[PATCH 2/2] acpi: acpidump: check for zero length headers to stop segfaulting (LP: #1375300)

[Alex Hung]

On 14-09-29 11:00 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 | 25 ++++++++++++++++++++++---
>   1 file changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/src/acpi/acpidump/acpidump.c b/src/acpi/acpidump/acpidump.c
> index b42363c..daad644 100644
> --- a/src/acpi/acpidump/acpidump.c
> +++ b/src/acpi/acpidump/acpidump.c
> @@ -1055,6 +1055,9 @@ 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))
> +		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) {
> @@ -1303,6 +1306,8 @@ static void acpidump_asf(fwts_framework *fw, const fwts_acpi_table_info *table)
>   			break;	/* Unknown! */
>   		}
>   
> +		if (hdr->length == 0)
> +			break;
>   		ptr += hdr->length;	/* Jump to next header */
>   
>   		if (type & 0x80)	/* Last record indicator, top bit of type field */
> @@ -1333,18 +1338,22 @@ 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)
> +			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);
>   		}
> @@ -1441,6 +1450,10 @@ static void acpidump_dmar(fwts_framework *fw, const fwts_acpi_table_info *table)
>   			break;
>   		}
>   
> +		/* Something not good with the data */
> +		if (header->length == 0)
> +			break;
> +
>   		ptr += header->length;
>   	}
>   }
> @@ -1515,6 +1528,8 @@ static void acpidump_slic(fwts_framework *fw, const fwts_acpi_table_info *table)
>   			break;
>   		}
>   
> +		if (header->length == 0)
> +			break;
>   		ptr += header->length;
>   	}
>   }
> @@ -1756,6 +1771,8 @@ static void acpidump_pcct(fwts_framework *fw, const fwts_acpi_table_info *table)
>   		default:
>   			break;
>   		}
> +		if (header->length == 0)
> +			break;
>   		ptr += header->length;
>   	}
>   }
> @@ -1911,6 +1928,8 @@ static void acpidump_dbg2(fwts_framework *fw, const fwts_acpi_table_info *table)
>   				(void *)oem_data - table->data);
>   		}
>   
> +		if (dbg2_info->length == 0)
> +			break;
>   		offset += dbg2_info->length;
>   		if (offset > table->length)
>   			break;

Will it be better if fwts output errors when length == 0?