diff mbox series

uefi: uefidump: add some guarding on loop iteration

Message ID 20171110102303.27357-1-colin.king@canonical.com
State Accepted
Headers show
Series uefi: uefidump: add some guarding on loop iteration | expand

Commit Message

Colin Ian King Nov. 10, 2017, 10:23 a.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Add some sanity checking to size boundaries. Cleans up a static
analysis warning.

Detected by CoverityScan, CID#1338583 ("Insecure data handling")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/uefi/uefidump/uefidump.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Alex Hung Nov. 13, 2017, 3:28 a.m. UTC | #1 
On 2017-11-10 06:23 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Add some sanity checking to size boundaries. Cleans up a static
> analysis warning.
> 
> Detected by CoverityScan, CID#1338583 ("Insecure data handling")
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/uefi/uefidump/uefidump.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
> index 55ce7f23..80ea2a99 100644
> --- a/src/uefi/uefidump/uefidump.c
> +++ b/src/uefi/uefidump/uefidump.c
> @@ -554,15 +554,22 @@ static char *uefidump_build_dev_path(char *path, fwts_uefi_dev_path *dev_path, c
>   			break;
>   		case FWTS_UEFI_DNS_DEVICE_PATH_SUBTYPE:
>   			if (dev_path_len > sizeof(fwts_uefi_dns_dev_path)) {
> -				size_t i;
>   				fwts_uefi_dns_dev_path *d = (fwts_uefi_dns_dev_path *)dev_path;
>   				uint16_t len = d->dev_path.length[0] | (((uint16_t)d->dev_path.length[1]) << 8);
> +				ssize_t i, sz = (ssize_t)len - sizeof(fwts_uefi_dns_dev_path);
>   				path = uefidump_vprintf(path, "\\DNS(0x%" PRIx8 ",", d->isipv6);
>   
> -				/* dump one or more DNS server address */
> -				for (i = 0; i < (len - sizeof(fwts_uefi_dns_dev_path)); i++)
> -					path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
> -				path = uefidump_vprintf(path, ")");
> +				/*
> +				 *  Add a little more sanity checking, but we can never be sure
> +				 *  we will not overrun d->dns_addr as we have to trust the given
> +				 *  length.
> +				 */
> +				if ((sz > 0) && (sz <= 0xffff)) {
> +					/* dump one or more DNS server address */
> +					for (i = 0; i < sz; i++)
> +						path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
> +					path = uefidump_vprintf(path, ")");
> +				}
>   			}
>   			break;
>   		default:
> 

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu Nov. 14, 2017, 10:13 a.m. UTC | #2 
On 11/10/2017 06:23 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Add some sanity checking to size boundaries. Cleans up a static
> analysis warning.
>
> Detected by CoverityScan, CID#1338583 ("Insecure data handling")
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/uefi/uefidump/uefidump.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
> index 55ce7f23..80ea2a99 100644
> --- a/src/uefi/uefidump/uefidump.c
> +++ b/src/uefi/uefidump/uefidump.c
> @@ -554,15 +554,22 @@ static char *uefidump_build_dev_path(char *path, fwts_uefi_dev_path *dev_path, c
>   			break;
>   		case FWTS_UEFI_DNS_DEVICE_PATH_SUBTYPE:
>   			if (dev_path_len > sizeof(fwts_uefi_dns_dev_path)) {
> -				size_t i;
>   				fwts_uefi_dns_dev_path *d = (fwts_uefi_dns_dev_path *)dev_path;
>   				uint16_t len = d->dev_path.length[0] | (((uint16_t)d->dev_path.length[1]) << 8);
> +				ssize_t i, sz = (ssize_t)len - sizeof(fwts_uefi_dns_dev_path);
>   				path = uefidump_vprintf(path, "\\DNS(0x%" PRIx8 ",", d->isipv6);
>   
> -				/* dump one or more DNS server address */
> -				for (i = 0; i < (len - sizeof(fwts_uefi_dns_dev_path)); i++)
> -					path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
> -				path = uefidump_vprintf(path, ")");
> +				/*
> +				 *  Add a little more sanity checking, but we can never be sure
> +				 *  we will not overrun d->dns_addr as we have to trust the given
> +				 *  length.
> +				 */
> +				if ((sz > 0) && (sz <= 0xffff)) {
> +					/* dump one or more DNS server address */
> +					for (i = 0; i < sz; i++)
> +						path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
> +					path = uefidump_vprintf(path, ")");
> +				}
>   			}
>   			break;
>   		default:
Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff mbox series

Patch

diff --git a/src/uefi/uefidump/uefidump.c b/src/uefi/uefidump/uefidump.c
index 55ce7f23..80ea2a99 100644
--- a/src/uefi/uefidump/uefidump.c
+++ b/src/uefi/uefidump/uefidump.c
@@ -554,15 +554,22 @@  static char *uefidump_build_dev_path(char *path, fwts_uefi_dev_path *dev_path, c
 			break;
 		case FWTS_UEFI_DNS_DEVICE_PATH_SUBTYPE:
 			if (dev_path_len > sizeof(fwts_uefi_dns_dev_path)) {
-				size_t i;
 				fwts_uefi_dns_dev_path *d = (fwts_uefi_dns_dev_path *)dev_path;
 				uint16_t len = d->dev_path.length[0] | (((uint16_t)d->dev_path.length[1]) << 8);
+				ssize_t i, sz = (ssize_t)len - sizeof(fwts_uefi_dns_dev_path);
 				path = uefidump_vprintf(path, "\\DNS(0x%" PRIx8 ",", d->isipv6);
 
-				/* dump one or more DNS server address */
-				for (i = 0; i < (len - sizeof(fwts_uefi_dns_dev_path)); i++)
-					path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
-				path = uefidump_vprintf(path, ")");
+				/*
+				 *  Add a little more sanity checking, but we can never be sure
+				 *  we will not overrun d->dns_addr as we have to trust the given
+				 *  length.
+				 */
+				if ((sz > 0) && (sz <= 0xffff)) {
+					/* dump one or more DNS server address */
+					for (i = 0; i < sz; i++)
+						path = uefidump_vprintf(path, "%02" PRIx8 , d->dns_addr[i]);
+					path = uefidump_vprintf(path, ")");
+				}
 			}
 			break;
 		default: