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

[Colin Ian King]

On 01/10/14 04:08, Alex Hung wrote:
> 
> 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?
> 
Well, since this is a utility and not a pass/fail test I will add some
information feedback rather than a fail. But, yes, silently exiting is
probably not sufficient. I will fix that

Colin