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

[Alex Hung]

On 10/01/2014 05:08 PM, Colin King wrote:
> From: Colin Ian King <colin.king at 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 at 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 at canonical.com>