ACK: [PATCH][V2] acpi: acpidump: check for short length headers (LP: #1375300)
Keng-Yu Lin
keng-yu.lin at canonical.com
Fri Oct 3 07:13:25 UTC 2014
On Fri, Oct 3, 2014 at 11:22 AM, Alex Hung <alex.hung at canonical.com> wrote:
> 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>
>
Acked-by: Keng-Yu Lin <kengyu at canonical.com>
More information about the fwts-devel
mailing list