diff mbox

[V2] acpi: acpidump: check for short length headers (LP: #1375300)

Message ID 1412154493-12773-1-git-send-email-colin.king@canonical.com
State Accepted
Headers show

Commit Message

Colin Ian King Oct. 1, 2014, 9:08 a.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

There are a bunch of places in fwts acpidump that takes the
given table header sizes and sub-table sizes as given,
however, firmware can get this wrong.

So bail out early on bad looking header sizes rather than
believe the data and fall off the end of the table.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/acpidump/acpidump.c | 51 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

Comments

Alex Hung Oct. 3, 2014, 3:22 a.m. UTC | #1 
On 10/01/2014 05:08 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> There are a bunch of places in fwts acpidump that takes the
> given table header sizes and sub-table sizes as given,
> however, firmware can get this wrong.
> 
> So bail out early on bad looking header sizes rather than
> believe the data and fall off the end of the table.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpi/acpidump/acpidump.c | 51 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/src/acpi/acpidump/acpidump.c b/src/acpi/acpidump/acpidump.c
> index e3fe4b1..5c1fcc1 100644
> --- a/src/acpi/acpidump/acpidump.c
> +++ b/src/acpi/acpidump/acpidump.c
> @@ -1055,6 +1055,13 @@ static void acpidump_slit(fwts_framework *fw, const fwts_acpi_table_info *table)
>  	uint64_t n = table->length - sizeof(fwts_acpi_table_slit);
>  	const uint8_t *entry;
>  
> +	if (table->length < sizeof(fwts_acpi_table_slit)) {
> +		fwts_log_info_verbatum(fw, "SLIT header length too short, expected %zu "
> +			"bytes, got %" PRIu64 " bytes instead. Aborting SLIT table dump.",
> +			sizeof(fwts_acpi_table_slit), table->length);
> +		return;
> +	}
> +
>  	fwts_log_info_verbatum(fw, "# Sys Localities: 0x%" PRIx64 "(%" PRIu64 ")",
>  		slit->num_of_system_localities, slit->num_of_system_localities);
>  	if (n < slit->num_of_system_localities * slit->num_of_system_localities) {
> @@ -1250,6 +1257,13 @@ static void acpidump_asf(fwts_framework *fw, const fwts_acpi_table_info *table)
>  		uint8_t i;
>  		uint8_t *asf_ptr = ptr;
>  
> +		/* Minimal header check */
> +		if (hdr->length < sizeof(fwts_acpi_table_asf_header)) {
> +			fwts_log_info_verbatum(fw, "ASF header length too short, expected %zu "
> +				"bytes, got %" PRIu16 " bytes instead. Aborting ASF table dump.",
> +				sizeof(fwts_acpi_table_asf_header), hdr->length);
> +			break;
> +		}
>  		fwts_log_nl(fw);
>  		__acpi_dump_table_fields(fw, asf_ptr, asf_header_fields, asf_ptr - data);
>  
> @@ -1333,18 +1347,25 @@ static void acpidump_dmar_device_scope(
>  
>  	/* Parse through multiple device scope entries */
>  	while (length > 0) {
> -		unsigned int i;
> +		ssize_t i, len;
>  
>  		fwts_acpi_table_dmar_device_scope *device_scope_entry =
>  			(fwts_acpi_table_dmar_device_scope *)device_scope;
>  		__acpi_dump_table_fields(fw, device_scope, dmar_device_scope_fields, device_scope - data);
> +		len = device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope);
> +		/* Something not good about the data */
> +		if (len <= 0) {
> +			fwts_log_info_verbatum(fw, "DMAR device scope entry length "
> +				"too short. Aborting device scope dump.");
> +			break;
> +		}
>  		/*
>  		 *  The device scope has a variable length path,
>  		 *  so just dump this raw data out for now.
>  		 */
> -		for (i = 0; i < device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope); i++) {
> +		for (i = 0; i < len; i++) {
>  			uint8_t val8 = device_scope_entry->path[i];
> -			fwts_log_info_verbatum(fw, "%s 0x%2.2x [%d]", acpi_dump_field_info("Path", 1,
> +			fwts_log_info_verbatum(fw, "%s 0x%2.2x [%zd]", acpi_dump_field_info("Path", 1,
>  				(device_scope - data) + sizeof(fwts_acpi_table_dmar_device_scope) + i),
>  				val8, i);
>  		}
> @@ -1407,6 +1428,14 @@ static void acpidump_dmar(fwts_framework *fw, const fwts_acpi_table_info *table)
>  			(fwts_acpi_table_dmar_header *)ptr;
>  
>  		fwts_log_nl(fw);
> +		/* Something not good with the data */
> +		if (header->length < sizeof(fwts_acpi_table_dmar_header)) {
> +			fwts_log_info_verbatum(fw, "DMAR header length "
> +				"too short, expected %zu bytes, got %" PRIu16
> +				" bytes instead. Aborting DMAR dump.",
> +				sizeof(fwts_acpi_table_dmar_header), header->length);
> +			break;
> +		}
>  
>  		switch (header->type) {
>  		case 0:
> @@ -1644,7 +1673,7 @@ static void acpidump_fpdt(fwts_framework *fw, const fwts_acpi_table_info *table)
>  		/* fpdt not long enough, bail out early */
>  		if (fpdt->length < 16) {
>  			size_t offset = ptr - data;
> -			fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %" 
> +			fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %"
>  				PRIu8 " less than 16 bytes. Data:", fpdt->length);
>  			acpi_dump_raw_data(fw, ptr, table->length - offset, offset);
>  			break;
> @@ -1748,6 +1777,13 @@ static void acpidump_pcct(fwts_framework *fw, const fwts_acpi_table_info *table)
>  		fwts_acpi_table_pcct_subspace_header *header =
>  			(fwts_acpi_table_pcct_subspace_header *)ptr;
>  
> +		if (header->length < sizeof(fwts_acpi_table_pcct_subspace_header)) {
> +			fwts_log_info_verbatum(fw, "PCCT subspace header length too short, expected %zu "
> +				"bytes, got %" PRIu8 " bytes instead. Aborting PCCT table dump.",
> +				sizeof(fwts_acpi_table_pcct_subspace_header), header->length);
> +			break;
> +		}
> +
>  		/* Currently just type 0 is supported */
>  		switch (header->type) {
>  		case 0: 
> @@ -1887,6 +1923,13 @@ static void acpidump_dbg2(fwts_framework *fw, const fwts_acpi_table_info *table)
>  
>  		__acpi_dump_table_fields(fw, table->data + offset, dbg2_info_fields, offset);
>  
> +		if (dbg2_info->length < sizeof(fwts_acpi_table_dbg2_info)) {
> +			fwts_log_info_verbatum(fw, "DBG2 info header length too short, expected %zu "
> +				"bytes, got %" PRIu16 " bytes instead. Aborting PCCT table dump.",
> +				sizeof(fwts_acpi_table_dbg2_info), dbg2_info->length);
> +			break;
> +		}
> +
>  		if (dbg2_info->number_of_regs) {
>  			/* Dump out the register GAS and sizes */
>  			for (j = 0; j < dbg2_info->number_of_regs; j++) {
> 

Acked-by: Alex Hung <alex.hung@canonical.com>
Keng-Yu Lin Oct. 3, 2014, 7:13 a.m. UTC | #2 
On Fri, Oct 3, 2014 at 11:22 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On 10/01/2014 05:08 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There are a bunch of places in fwts acpidump that takes the
>> given table header sizes and sub-table sizes as given,
>> however, firmware can get this wrong.
>>
>> So bail out early on bad looking header sizes rather than
>> believe the data and fall off the end of the table.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  src/acpi/acpidump/acpidump.c | 51 ++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/acpi/acpidump/acpidump.c b/src/acpi/acpidump/acpidump.c
>> index e3fe4b1..5c1fcc1 100644
>> --- a/src/acpi/acpidump/acpidump.c
>> +++ b/src/acpi/acpidump/acpidump.c
>> @@ -1055,6 +1055,13 @@ static void acpidump_slit(fwts_framework *fw, const fwts_acpi_table_info *table)
>>       uint64_t n = table->length - sizeof(fwts_acpi_table_slit);
>>       const uint8_t *entry;
>>
>> +     if (table->length < sizeof(fwts_acpi_table_slit)) {
>> +             fwts_log_info_verbatum(fw, "SLIT header length too short, expected %zu "
>> +                     "bytes, got %" PRIu64 " bytes instead. Aborting SLIT table dump.",
>> +                     sizeof(fwts_acpi_table_slit), table->length);
>> +             return;
>> +     }
>> +
>>       fwts_log_info_verbatum(fw, "# Sys Localities: 0x%" PRIx64 "(%" PRIu64 ")",
>>               slit->num_of_system_localities, slit->num_of_system_localities);
>>       if (n < slit->num_of_system_localities * slit->num_of_system_localities) {
>> @@ -1250,6 +1257,13 @@ static void acpidump_asf(fwts_framework *fw, const fwts_acpi_table_info *table)
>>               uint8_t i;
>>               uint8_t *asf_ptr = ptr;
>>
>> +             /* Minimal header check */
>> +             if (hdr->length < sizeof(fwts_acpi_table_asf_header)) {
>> +                     fwts_log_info_verbatum(fw, "ASF header length too short, expected %zu "
>> +                             "bytes, got %" PRIu16 " bytes instead. Aborting ASF table dump.",
>> +                             sizeof(fwts_acpi_table_asf_header), hdr->length);
>> +                     break;
>> +             }
>>               fwts_log_nl(fw);
>>               __acpi_dump_table_fields(fw, asf_ptr, asf_header_fields, asf_ptr - data);
>>
>> @@ -1333,18 +1347,25 @@ static void acpidump_dmar_device_scope(
>>
>>       /* Parse through multiple device scope entries */
>>       while (length > 0) {
>> -             unsigned int i;
>> +             ssize_t i, len;
>>
>>               fwts_acpi_table_dmar_device_scope *device_scope_entry =
>>                       (fwts_acpi_table_dmar_device_scope *)device_scope;
>>               __acpi_dump_table_fields(fw, device_scope, dmar_device_scope_fields, device_scope - data);
>> +             len = device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope);
>> +             /* Something not good about the data */
>> +             if (len <= 0) {
>> +                     fwts_log_info_verbatum(fw, "DMAR device scope entry length "
>> +                             "too short. Aborting device scope dump.");
>> +                     break;
>> +             }
>>               /*
>>                *  The device scope has a variable length path,
>>                *  so just dump this raw data out for now.
>>                */
>> -             for (i = 0; i < device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope); i++) {
>> +             for (i = 0; i < len; i++) {
>>                       uint8_t val8 = device_scope_entry->path[i];
>> -                     fwts_log_info_verbatum(fw, "%s 0x%2.2x [%d]", acpi_dump_field_info("Path", 1,
>> +                     fwts_log_info_verbatum(fw, "%s 0x%2.2x [%zd]", acpi_dump_field_info("Path", 1,
>>                               (device_scope - data) + sizeof(fwts_acpi_table_dmar_device_scope) + i),
>>                               val8, i);
>>               }
>> @@ -1407,6 +1428,14 @@ static void acpidump_dmar(fwts_framework *fw, const fwts_acpi_table_info *table)
>>                       (fwts_acpi_table_dmar_header *)ptr;
>>
>>               fwts_log_nl(fw);
>> +             /* Something not good with the data */
>> +             if (header->length < sizeof(fwts_acpi_table_dmar_header)) {
>> +                     fwts_log_info_verbatum(fw, "DMAR header length "
>> +                             "too short, expected %zu bytes, got %" PRIu16
>> +                             " bytes instead. Aborting DMAR dump.",
>> +                             sizeof(fwts_acpi_table_dmar_header), header->length);
>> +                     break;
>> +             }
>>
>>               switch (header->type) {
>>               case 0:
>> @@ -1644,7 +1673,7 @@ static void acpidump_fpdt(fwts_framework *fw, const fwts_acpi_table_info *table)
>>               /* fpdt not long enough, bail out early */
>>               if (fpdt->length < 16) {
>>                       size_t offset = ptr - data;
>> -                     fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %"
>> +                     fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %"
>>                               PRIu8 " less than 16 bytes. Data:", fpdt->length);
>>                       acpi_dump_raw_data(fw, ptr, table->length - offset, offset);
>>                       break;
>> @@ -1748,6 +1777,13 @@ static void acpidump_pcct(fwts_framework *fw, const fwts_acpi_table_info *table)
>>               fwts_acpi_table_pcct_subspace_header *header =
>>                       (fwts_acpi_table_pcct_subspace_header *)ptr;
>>
>> +             if (header->length < sizeof(fwts_acpi_table_pcct_subspace_header)) {
>> +                     fwts_log_info_verbatum(fw, "PCCT subspace header length too short, expected %zu "
>> +                             "bytes, got %" PRIu8 " bytes instead. Aborting PCCT table dump.",
>> +                             sizeof(fwts_acpi_table_pcct_subspace_header), header->length);
>> +                     break;
>> +             }
>> +
>>               /* Currently just type 0 is supported */
>>               switch (header->type) {
>>               case 0:
>> @@ -1887,6 +1923,13 @@ static void acpidump_dbg2(fwts_framework *fw, const fwts_acpi_table_info *table)
>>
>>               __acpi_dump_table_fields(fw, table->data + offset, dbg2_info_fields, offset);
>>
>> +             if (dbg2_info->length < sizeof(fwts_acpi_table_dbg2_info)) {
>> +                     fwts_log_info_verbatum(fw, "DBG2 info header length too short, expected %zu "
>> +                             "bytes, got %" PRIu16 " bytes instead. Aborting PCCT table dump.",
>> +                             sizeof(fwts_acpi_table_dbg2_info), dbg2_info->length);
>> +                     break;
>> +             }
>> +
>>               if (dbg2_info->number_of_regs) {
>>                       /* Dump out the register GAS and sizes */
>>                       for (j = 0; j < dbg2_info->number_of_regs; j++) {
>>
>
> Acked-by: Alex Hung <alex.hung@canonical.com>
>

Acked-by: Keng-Yu Lin <kengyu@canonical.com>
diff mbox

Patch

diff --git a/src/acpi/acpidump/acpidump.c b/src/acpi/acpidump/acpidump.c
index e3fe4b1..5c1fcc1 100644
--- a/src/acpi/acpidump/acpidump.c
+++ b/src/acpi/acpidump/acpidump.c
@@ -1055,6 +1055,13 @@  static void acpidump_slit(fwts_framework *fw, const fwts_acpi_table_info *table)
 	uint64_t n = table->length - sizeof(fwts_acpi_table_slit);
 	const uint8_t *entry;
 
+	if (table->length < sizeof(fwts_acpi_table_slit)) {
+		fwts_log_info_verbatum(fw, "SLIT header length too short, expected %zu "
+			"bytes, got %" PRIu64 " bytes instead. Aborting SLIT table dump.",
+			sizeof(fwts_acpi_table_slit), table->length);
+		return;
+	}
+
 	fwts_log_info_verbatum(fw, "# Sys Localities: 0x%" PRIx64 "(%" PRIu64 ")",
 		slit->num_of_system_localities, slit->num_of_system_localities);
 	if (n < slit->num_of_system_localities * slit->num_of_system_localities) {
@@ -1250,6 +1257,13 @@  static void acpidump_asf(fwts_framework *fw, const fwts_acpi_table_info *table)
 		uint8_t i;
 		uint8_t *asf_ptr = ptr;
 
+		/* Minimal header check */
+		if (hdr->length < sizeof(fwts_acpi_table_asf_header)) {
+			fwts_log_info_verbatum(fw, "ASF header length too short, expected %zu "
+				"bytes, got %" PRIu16 " bytes instead. Aborting ASF table dump.",
+				sizeof(fwts_acpi_table_asf_header), hdr->length);
+			break;
+		}
 		fwts_log_nl(fw);
 		__acpi_dump_table_fields(fw, asf_ptr, asf_header_fields, asf_ptr - data);
 
@@ -1333,18 +1347,25 @@  static void acpidump_dmar_device_scope(
 
 	/* Parse through multiple device scope entries */
 	while (length > 0) {
-		unsigned int i;
+		ssize_t i, len;
 
 		fwts_acpi_table_dmar_device_scope *device_scope_entry =
 			(fwts_acpi_table_dmar_device_scope *)device_scope;
 		__acpi_dump_table_fields(fw, device_scope, dmar_device_scope_fields, device_scope - data);
+		len = device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope);
+		/* Something not good about the data */
+		if (len <= 0) {
+			fwts_log_info_verbatum(fw, "DMAR device scope entry length "
+				"too short. Aborting device scope dump.");
+			break;
+		}
 		/*
 		 *  The device scope has a variable length path,
 		 *  so just dump this raw data out for now.
 		 */
-		for (i = 0; i < device_scope_entry->length - sizeof(fwts_acpi_table_dmar_device_scope); i++) {
+		for (i = 0; i < len; i++) {
 			uint8_t val8 = device_scope_entry->path[i];
-			fwts_log_info_verbatum(fw, "%s 0x%2.2x [%d]", acpi_dump_field_info("Path", 1,
+			fwts_log_info_verbatum(fw, "%s 0x%2.2x [%zd]", acpi_dump_field_info("Path", 1,
 				(device_scope - data) + sizeof(fwts_acpi_table_dmar_device_scope) + i),
 				val8, i);
 		}
@@ -1407,6 +1428,14 @@  static void acpidump_dmar(fwts_framework *fw, const fwts_acpi_table_info *table)
 			(fwts_acpi_table_dmar_header *)ptr;
 
 		fwts_log_nl(fw);
+		/* Something not good with the data */
+		if (header->length < sizeof(fwts_acpi_table_dmar_header)) {
+			fwts_log_info_verbatum(fw, "DMAR header length "
+				"too short, expected %zu bytes, got %" PRIu16
+				" bytes instead. Aborting DMAR dump.",
+				sizeof(fwts_acpi_table_dmar_header), header->length);
+			break;
+		}
 
 		switch (header->type) {
 		case 0:
@@ -1644,7 +1673,7 @@  static void acpidump_fpdt(fwts_framework *fw, const fwts_acpi_table_info *table)
 		/* fpdt not long enough, bail out early */
 		if (fpdt->length < 16) {
 			size_t offset = ptr - data;
-			fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %" 
+			fwts_log_info_verbatum(fw, "Cannot decode FPDT header, size %"
 				PRIu8 " less than 16 bytes. Data:", fpdt->length);
 			acpi_dump_raw_data(fw, ptr, table->length - offset, offset);
 			break;
@@ -1748,6 +1777,13 @@  static void acpidump_pcct(fwts_framework *fw, const fwts_acpi_table_info *table)
 		fwts_acpi_table_pcct_subspace_header *header =
 			(fwts_acpi_table_pcct_subspace_header *)ptr;
 
+		if (header->length < sizeof(fwts_acpi_table_pcct_subspace_header)) {
+			fwts_log_info_verbatum(fw, "PCCT subspace header length too short, expected %zu "
+				"bytes, got %" PRIu8 " bytes instead. Aborting PCCT table dump.",
+				sizeof(fwts_acpi_table_pcct_subspace_header), header->length);
+			break;
+		}
+
 		/* Currently just type 0 is supported */
 		switch (header->type) {
 		case 0: 
@@ -1887,6 +1923,13 @@  static void acpidump_dbg2(fwts_framework *fw, const fwts_acpi_table_info *table)
 
 		__acpi_dump_table_fields(fw, table->data + offset, dbg2_info_fields, offset);
 
+		if (dbg2_info->length < sizeof(fwts_acpi_table_dbg2_info)) {
+			fwts_log_info_verbatum(fw, "DBG2 info header length too short, expected %zu "
+				"bytes, got %" PRIu16 " bytes instead. Aborting PCCT table dump.",
+				sizeof(fwts_acpi_table_dbg2_info), dbg2_info->length);
+			break;
+		}
+
 		if (dbg2_info->number_of_regs) {
 			/* Dump out the register GAS and sizes */
 			for (j = 0; j < dbg2_info->number_of_regs; j++) {