Re: [PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing

[Dov Murik]

Thanks Dave for reviewing.


On 21/02/2022 21:44, Dr. David Alan Gilbert wrote:
> * Dov Murik (dovmurik@linux.ibm.com) wrote:
>> When pc_system_parse_ovmf_flash() parses the optional GUIDed table in
>> the end of the OVMF flash memory area, the table length field is checked
>> for sizes that are too small, but doesn't error on sizes that are too
>> big (bigger than the flash content itself).
>>
>> Add a check for maximal size of the OVMF table, and add an error report
>> in case the size is invalid.  In such a case, an error like this will be
>> displayed during launch:
>>
>>     qemu-system-x86_64: OVMF table has invalid size 4047
>>
>> and the table parsing is skipped.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>
>> ---
>>
>> v2:
>> - add error message example to commit description
>> - replace magic numbers 48 and 50 with size calculations (thanks Phil MD)
>> ---
>>  hw/i386/pc_sysfw_ovmf.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
>> index f4dd92c588..1c9a16e9e6 100644
>> --- a/hw/i386/pc_sysfw_ovmf.c
>> +++ b/hw/i386/pc_sysfw_ovmf.c
>> @@ -24,11 +24,14 @@
>>   */
>>  
>>  #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>  #include "hw/i386/pc.h"
>>  #include "cpu.h"
>>  
>>  #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d"
>>  
>> +static const int bytes_after_table_footer = 32;
>> +
>>  static bool ovmf_flash_parsed;
>>  static uint8_t *ovmf_table;
>>  static int ovmf_table_len;
>> @@ -38,6 +41,8 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t 
>> flash_size)
>>      uint8_t *ptr;
>>      QemuUUID guid;
>>      int tot_len;
>> +    int max_tot_len = flash_size - (bytes_after_table_footer +
>> +                                    sizeof(guid) + sizeof(uint16_t));
>>  
>>      /* should only be called once */
>>      if (ovmf_flash_parsed) {
>> @@ -52,12 +57,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, 
>> size_t flash_size)
>>  
>>      /*
>>       * if this is OVMF there will be a table footer
>> -     * guid 48 bytes before the end of the flash file.  If it's
>> -     * not found, silently abort the flash parsing.
>> +     * guid 48 bytes before the end of the flash file
>> +     * (= 32 bytes after the table + 16 bytes the GUID itself).
>> +     * If it's not found, silently abort the flash parsing.
>>       */
>>      qemu_uuid_parse(OVMF_TABLE_FOOTER_GUID, &guid);
>>      guid = qemu_uuid_bswap(guid); /* guids are LE */
>> -    ptr = flash_ptr + flash_size - 48;
>> +    ptr = flash_ptr + flash_size - (bytes_after_table_footer + 
>> sizeof(guid));
>>      if (!qemu_uuid_is_equal((QemuUUID *)ptr, &guid)) {
>>          return;
>>      }


I'll probably split the two hunks above (without max_tot_len) to a
separate patch "hw/i386: Replace magic number with field length
calculation".



>> @@ -66,7 +72,13 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, 
>> size_t flash_size)
>>      ptr -= sizeof(uint16_t);
>>      tot_len = le16_to_cpu(*(uint16_t *)ptr) - sizeof(guid) - 
>> sizeof(uint16_t);
> 
> Instead of the max_tot_len calculation above, is it actually:
>    max_tot_len = ptr - flash_ptr;
> 
> I think that works out the same and avoids doing the calculation in two
> places; it's also logically what you have - you can't read over the
> structure you just read.

Good call, it indeed gives the same result.


I'll change the condition below to:

    if (tot_len < 0 || tot_len > (ptr - flash_ptr))

and remove the max_tot_len variable, and put this in a separate patch
that only adds this condition to solve the overflow problem.


Thanks,
-Dov



> 
> Dave
> 
>> -    if (tot_len <= 0) {
>> +    if (tot_len < 0 || tot_len > max_tot_len) {
>> +        error_report("OVMF table has invalid size %d", tot_len);
>> +        return;
>> +    }
>> +
>> +    if (tot_len == 0) {
>> +        /* no entries in the OVMF table */
>>          return;
>>      }
>>  
>>
>> base-commit: 48033ad678ae2def43bf0d543a2c4c3d2a93feaf
>> -- 
>> 2.25.1
>>