1. Patchew
  2. QEMU
  3. View series

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

Dov Murik posted 1 patch 2 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220216064024.153423-1-dovmurik@linux.ibm.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/i386/pc_sysfw_ovmf.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
[PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Dov Murik 2 years, 2 months ago 
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;
     }
@@ -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);
 
-    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
Re: [PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Dr. David Alan Gilbert 2 years, 2 months ago 
* 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;
>      }
> @@ -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.

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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [PATCH v2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Dov Murik 2 years, 2 months ago 
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
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.