diff mbox

[1/2] lib: make acpidump parser more robust (LP: #1471202)

Message ID 1435928230-21604-2-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King July 3, 2015, 12:57 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

When given data that contains more than one space between each hex
byte the parser has a conflict between a fixed format input and less
than 16 bytes being scanned.  Since the data being allocated on
a short row is less than 16 bytes and the offsets always increment
by 16 bytes we get a mismatch between where the parser is writing
the data and the size of the allocated table.

Make the parser more robust:

1. Only parse rows that match the acpidump format exactly
2. Abort if offsets don't increment as we expect
3. Treat short rows of less than 16 bytes as end-of-table

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/lib/src/fwts_acpi_tables.c | 95 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 15 deletions(-)

Comments

Alex Hung July 6, 2015, 3:19 a.m. UTC | #1 
On 07/03/2015 08:57 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> When given data that contains more than one space between each hex
> byte the parser has a conflict between a fixed format input and less
> than 16 bytes being scanned.  Since the data being allocated on
> a short row is less than 16 bytes and the offsets always increment
> by 16 bytes we get a mismatch between where the parser is writing
> the data and the size of the allocated table.
> 
> Make the parser more robust:
> 
> 1. Only parse rows that match the acpidump format exactly
> 2. Abort if offsets don't increment as we expect
> 3. Treat short rows of less than 16 bytes as end-of-table
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/lib/src/fwts_acpi_tables.c | 95 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 15 deletions(-)
> 
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index 6bb69fd..1592791 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -31,6 +31,7 @@
>  #include <dirent.h>
>  #include <fcntl.h>
>  #include <errno.h>
> +#include <ctype.h>
>  
>  #include "fwts.h"
>  
> @@ -520,9 +521,14 @@ static uint32_t fwts_fake_physical_addr(const size_t size)
>   *  fwts_acpi_load_table_from_acpidump()
>   *	Load an ACPI table from the output of acpidump or fwts --dump
>   */
> -static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_t *addr, size_t *size)
> +static uint8_t *fwts_acpi_load_table_from_acpidump(
> +	fwts_framework *fw,
> +	FILE *fp,
> +	char *name,
> +	uint64_t *addr,
> +	size_t *size)
>  {
> -	uint32_t offset;
> +	uint32_t offset, expected_offset = 0;
>  	uint8_t  data[16];
>  	char buffer[128];
>  	uint8_t *table;
> @@ -567,34 +573,93 @@ static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
>  	if (strncmp(name, "RSD PTR", 7) == 0)
>  		strcpy(name, "RSDP");
>  
> -	/* Pull in 16 bytes at a time */
> +	/*
> +	 *  Pull in 16 bytes at a time, data MUST be conforming to the
> +	 *  acpidump format, e.g.:
> +	 *   0000: 46 41 43 50 f4 00 00 00 03 f9 41 4d 44 20 20 20  FACP......AMD
> +	 *
> +	 *  This parser expects hex data to be separated by one space,
> +	 *  anything not conforming to this rigid format will be prematurely
> +	 *  aborted
> +	 */
>  	while (fgets(buffer, sizeof(buffer), fp) ) {
>  		uint8_t *new_tmp;
> +		char *ptr;
>  		int n;
> -		buffer[56] = '\0';	/* truncate */
> -		if ((n = sscanf(buffer,"  %x: %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx",
> -			&offset,
> -			&data[0], &data[1], &data[2], &data[3],
> -			&data[4], &data[5], &data[6], &data[7],
> -			&data[8], &data[9], &data[10], &data[11],
> -			&data[12], &data[13], &data[14], &data[15])) < 1)
> +
> +		/* Get offset */
> +		if (sscanf(buffer,"  %" SCNx32 ": ", &offset) < 1)
> +			break;
> +
> +		/* Offset are not correct, abort with truncated table */
> +		if (offset != expected_offset) {
> +			fwts_log_error(fw, "ACPI dump offsets in table '%s'"
> +				" are not incrementing by 16 bytes per row. "
> +				" Table truncated prematurely due to bad acpidump data.",
> +				name);
> +			break;
> +		}
> +
> +		expected_offset += 16;
> +
> +		/* Data follows the colon, abort if not found */
> +		ptr = strstr(buffer, ": ");
> +		if (!ptr) {
> +			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
> +				"any data, expecting at least 1 hex byte of data per row.",
> +				name);
> +			break;
> +		}
> +
> +		ptr += 2;
> +		/* Now expect 16 lots of 2 hex digits and a space */
> +		for (n = 0; n < 16; n++) {
> +			/*
> +			 *  Need to be 100% sure 2 hex digits. Maybe a short row
> +		 	 *  because it is the end of the table, so assume it is the
> +			 *  end of the table if not hex digits.
> +			 */
> +			if (!isxdigit(*ptr) || !isxdigit(*(ptr + 1))) {
> +				break;
> +			}
> +			if (sscanf(ptr, "%2" SCNx8, &data[n]) != 1) {
> +				/* Bug in parsing! should never happen! */
> +				fwts_log_error(fw, "ACPI dump in table '%s' at hex byte position "
> +					"%d did parse correctly, got "
> +					"'%2.2s' which could not be parsed as a hex value.",
> +					name, n, ptr);
> +				break;
> +			}
> +			ptr += 3;
> +		}
> +
> +		/* Got no data? */
> +		if (n == 0) {
> +			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
> +				"any data, expecting at least 1 hex byte of data per row.",
> +				name);
>  			break;
> +		}
>  
> -		len += (n - 1);
> -		if (len == 0)
> -			break;	/* No data, must be corrupt input */
> +		len += n;
>  		if ((new_tmp = realloc(tmp, len)) == NULL) {
>  			free(tmp);
> +			fwts_log_error(fw, "ACPI table parser run out of memory parsing table '%s'.", name);
>  			return NULL;
>  		} else
>  			tmp = new_tmp;
>  
> -		memcpy(tmp + offset, data, n-1);
> +		memcpy(tmp + offset, data, n);
> +
> +		/* Treat less than a full row as last one */
> +		if (n != 16)
> +			break;
>  	}
>  
>  	/* Allocate the table using low 32 bit memory */
>  	if ((table = fwts_low_malloc(len)) == NULL) {
>  		free(tmp);
> +		fwts_log_error(fw, "ACPI table parser run out of 32 bit memory parsing table '%s'.", name);
>  		return NULL;
>  	}
>  	memcpy(table, tmp, len);
> @@ -633,7 +698,7 @@ static int fwts_acpi_load_tables_from_acpidump(fwts_framework *fw)
>  		size_t length;
>  		char name[16];
>  
> -		if ((table = fwts_acpi_load_table_from_acpidump(fp, name, &addr, &length)) != NULL)
> +		if ((table = fwts_acpi_load_table_from_acpidump(fw, fp, name, &addr, &length)) != NULL)
>  			fwts_acpi_add_table(name, table, addr, length, FWTS_ACPI_TABLE_FROM_FILE);
>  	}
>  
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu July 6, 2015, 3:55 a.m. UTC | #2 
On 2015年07月03日 20:57, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> When given data that contains more than one space between each hex
> byte the parser has a conflict between a fixed format input and less
> than 16 bytes being scanned.  Since the data being allocated on
> a short row is less than 16 bytes and the offsets always increment
> by 16 bytes we get a mismatch between where the parser is writing
> the data and the size of the allocated table.
>
> Make the parser more robust:
>
> 1. Only parse rows that match the acpidump format exactly
> 2. Abort if offsets don't increment as we expect
> 3. Treat short rows of less than 16 bytes as end-of-table
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/lib/src/fwts_acpi_tables.c | 95 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index 6bb69fd..1592791 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -31,6 +31,7 @@
>   #include <dirent.h>
>   #include <fcntl.h>
>   #include <errno.h>
> +#include <ctype.h>
>   
>   #include "fwts.h"
>   
> @@ -520,9 +521,14 @@ static uint32_t fwts_fake_physical_addr(const size_t size)
>    *  fwts_acpi_load_table_from_acpidump()
>    *	Load an ACPI table from the output of acpidump or fwts --dump
>    */
> -static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_t *addr, size_t *size)
> +static uint8_t *fwts_acpi_load_table_from_acpidump(
> +	fwts_framework *fw,
> +	FILE *fp,
> +	char *name,
> +	uint64_t *addr,
> +	size_t *size)
>   {
> -	uint32_t offset;
> +	uint32_t offset, expected_offset = 0;
>   	uint8_t  data[16];
>   	char buffer[128];
>   	uint8_t *table;
> @@ -567,34 +573,93 @@ static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
>   	if (strncmp(name, "RSD PTR", 7) == 0)
>   		strcpy(name, "RSDP");
>   
> -	/* Pull in 16 bytes at a time */
> +	/*
> +	 *  Pull in 16 bytes at a time, data MUST be conforming to the
> +	 *  acpidump format, e.g.:
> +	 *   0000: 46 41 43 50 f4 00 00 00 03 f9 41 4d 44 20 20 20  FACP......AMD
> +	 *
> +	 *  This parser expects hex data to be separated by one space,
> +	 *  anything not conforming to this rigid format will be prematurely
> +	 *  aborted
> +	 */
>   	while (fgets(buffer, sizeof(buffer), fp) ) {
>   		uint8_t *new_tmp;
> +		char *ptr;
>   		int n;
> -		buffer[56] = '\0';	/* truncate */
> -		if ((n = sscanf(buffer,"  %x: %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx",
> -			&offset,
> -			&data[0], &data[1], &data[2], &data[3],
> -			&data[4], &data[5], &data[6], &data[7],
> -			&data[8], &data[9], &data[10], &data[11],
> -			&data[12], &data[13], &data[14], &data[15])) < 1)
> +
> +		/* Get offset */
> +		if (sscanf(buffer,"  %" SCNx32 ": ", &offset) < 1)
> +			break;
> +
> +		/* Offset are not correct, abort with truncated table */
> +		if (offset != expected_offset) {
> +			fwts_log_error(fw, "ACPI dump offsets in table '%s'"
> +				" are not incrementing by 16 bytes per row. "
> +				" Table truncated prematurely due to bad acpidump data.",
> +				name);
> +			break;
> +		}
> +
> +		expected_offset += 16;
> +
> +		/* Data follows the colon, abort if not found */
> +		ptr = strstr(buffer, ": ");
> +		if (!ptr) {
> +			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
> +				"any data, expecting at least 1 hex byte of data per row.",
> +				name);
> +			break;
> +		}
> +
> +		ptr += 2;
> +		/* Now expect 16 lots of 2 hex digits and a space */
> +		for (n = 0; n < 16; n++) {
> +			/*
> +			 *  Need to be 100% sure 2 hex digits. Maybe a short row
> +		 	 *  because it is the end of the table, so assume it is the
> +			 *  end of the table if not hex digits.
> +			 */
> +			if (!isxdigit(*ptr) || !isxdigit(*(ptr + 1))) {
> +				break;
> +			}
> +			if (sscanf(ptr, "%2" SCNx8, &data[n]) != 1) {Re
> +				/* Bug in parsing! should never happen! */
> +				fwts_log_error(fw, "ACPI dump in table '%s' at hex byte position "
> +					"%d did parse correctly, got "
> +					"'%2.2s' which could not be parsed as a hex value.",
> +					name, n, ptr);
> +				break;
> +			}
> +			ptr += 3;
> +		}
> +
> +		/* Got no data? */
> +		if (n == 0) {
> +			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
> +				"any data, expecting at least 1 hex byte of data per row.",
> +				name);
>   			break;
> +		}
>   
> -		len += (n - 1);
> -		if (len == 0)
> -			break;	/* No data, must be corrupt input */
> +		len += n;
>   		if ((new_tmp = realloc(tmp, len)) == NULL) {
>   			free(tmp);
> +			fwts_log_error(fw, "ACPI table parser run out of memory parsing table '%s'.", name);
>   			return NULL;
>   		} else
>   			tmp = new_tmp;
>   
> -		memcpy(tmp + offset, data, n-1);
> +		memcpy(tmp + offset, data, n);
> +
> +		/* Treat less than a full row as last one */
> +		if (n != 16)
> +			break;
>   	}
>   
>   	/* Allocate the table using low 32 bit memory */
>   	if ((table = fwts_low_malloc(len)) == NULL) {
>   		free(tmp);
> +		fwts_log_error(fw, "ACPI table parser run out of 32 bit memory parsing table '%s'.", name);
>   		return NULL;
>   	}
>   	memcpy(table, tmp, len);
> @@ -633,7 +698,7 @@ static int fwts_acpi_load_tables_from_acpidump(fwts_framework *fw)
>   		size_t length;
>   		char name[16];
>   
> -		if ((table = fwts_acpi_load_table_from_acpidump(fp, name, &addr, &length)) != NULL)
> +		if ((table = fwts_acpi_load_table_from_acpidump(fw, fp, name, &addr, &length)) != NULL)
>   			fwts_acpi_add_table(name, table, addr, length, FWTS_ACPI_TABLE_FROM_FILE);
>   	}
>   
Acked-by: Ivan Hu<ivan.hu@canonical.com>
diff mbox

Patch

diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
index 6bb69fd..1592791 100644
--- a/src/lib/src/fwts_acpi_tables.c
+++ b/src/lib/src/fwts_acpi_tables.c
@@ -31,6 +31,7 @@ 
 #include <dirent.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <ctype.h>
 
 #include "fwts.h"
 
@@ -520,9 +521,14 @@  static uint32_t fwts_fake_physical_addr(const size_t size)
  *  fwts_acpi_load_table_from_acpidump()
  *	Load an ACPI table from the output of acpidump or fwts --dump
  */
-static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_t *addr, size_t *size)
+static uint8_t *fwts_acpi_load_table_from_acpidump(
+	fwts_framework *fw,
+	FILE *fp,
+	char *name,
+	uint64_t *addr,
+	size_t *size)
 {
-	uint32_t offset;
+	uint32_t offset, expected_offset = 0;
 	uint8_t  data[16];
 	char buffer[128];
 	uint8_t *table;
@@ -567,34 +573,93 @@  static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
 	if (strncmp(name, "RSD PTR", 7) == 0)
 		strcpy(name, "RSDP");
 
-	/* Pull in 16 bytes at a time */
+	/*
+	 *  Pull in 16 bytes at a time, data MUST be conforming to the
+	 *  acpidump format, e.g.:
+	 *   0000: 46 41 43 50 f4 00 00 00 03 f9 41 4d 44 20 20 20  FACP......AMD
+	 *
+	 *  This parser expects hex data to be separated by one space,
+	 *  anything not conforming to this rigid format will be prematurely
+	 *  aborted
+	 */
 	while (fgets(buffer, sizeof(buffer), fp) ) {
 		uint8_t *new_tmp;
+		char *ptr;
 		int n;
-		buffer[56] = '\0';	/* truncate */
-		if ((n = sscanf(buffer,"  %x: %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx",
-			&offset,
-			&data[0], &data[1], &data[2], &data[3],
-			&data[4], &data[5], &data[6], &data[7],
-			&data[8], &data[9], &data[10], &data[11],
-			&data[12], &data[13], &data[14], &data[15])) < 1)
+
+		/* Get offset */
+		if (sscanf(buffer,"  %" SCNx32 ": ", &offset) < 1)
+			break;
+
+		/* Offset are not correct, abort with truncated table */
+		if (offset != expected_offset) {
+			fwts_log_error(fw, "ACPI dump offsets in table '%s'"
+				" are not incrementing by 16 bytes per row. "
+				" Table truncated prematurely due to bad acpidump data.",
+				name);
+			break;
+		}
+
+		expected_offset += 16;
+
+		/* Data follows the colon, abort if not found */
+		ptr = strstr(buffer, ": ");
+		if (!ptr) {
+			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
+				"any data, expecting at least 1 hex byte of data per row.",
+				name);
+			break;
+		}
+
+		ptr += 2;
+		/* Now expect 16 lots of 2 hex digits and a space */
+		for (n = 0; n < 16; n++) {
+			/*
+			 *  Need to be 100% sure 2 hex digits. Maybe a short row
+		 	 *  because it is the end of the table, so assume it is the
+			 *  end of the table if not hex digits.
+			 */
+			if (!isxdigit(*ptr) || !isxdigit(*(ptr + 1))) {
+				break;
+			}
+			if (sscanf(ptr, "%2" SCNx8, &data[n]) != 1) {
+				/* Bug in parsing! should never happen! */
+				fwts_log_error(fw, "ACPI dump in table '%s' at hex byte position "
+					"%d did parse correctly, got "
+					"'%2.2s' which could not be parsed as a hex value.",
+					name, n, ptr);
+				break;
+			}
+			ptr += 3;
+		}
+
+		/* Got no data? */
+		if (n == 0) {
+			fwts_log_error(fw, "ACPI dump in table '%s' did not contain "
+				"any data, expecting at least 1 hex byte of data per row.",
+				name);
 			break;
+		}
 
-		len += (n - 1);
-		if (len == 0)
-			break;	/* No data, must be corrupt input */
+		len += n;
 		if ((new_tmp = realloc(tmp, len)) == NULL) {
 			free(tmp);
+			fwts_log_error(fw, "ACPI table parser run out of memory parsing table '%s'.", name);
 			return NULL;
 		} else
 			tmp = new_tmp;
 
-		memcpy(tmp + offset, data, n-1);
+		memcpy(tmp + offset, data, n);
+
+		/* Treat less than a full row as last one */
+		if (n != 16)
+			break;
 	}
 
 	/* Allocate the table using low 32 bit memory */
 	if ((table = fwts_low_malloc(len)) == NULL) {
 		free(tmp);
+		fwts_log_error(fw, "ACPI table parser run out of 32 bit memory parsing table '%s'.", name);
 		return NULL;
 	}
 	memcpy(table, tmp, len);
@@ -633,7 +698,7 @@  static int fwts_acpi_load_tables_from_acpidump(fwts_framework *fw)
 		size_t length;
 		char name[16];
 
-		if ((table = fwts_acpi_load_table_from_acpidump(fp, name, &addr, &length)) != NULL)
+		if ((table = fwts_acpi_load_table_from_acpidump(fw, fp, name, &addr, &length)) != NULL)
 			fwts_acpi_add_table(name, table, addr, length, FWTS_ACPI_TABLE_FROM_FILE);
 	}