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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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