[PATCH v3 1/2] hw/i386: Improve bounds checking in OVMF table parsing

Dov Murik posted 2 patches 3 years, 11 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Eduardo Habkost <eduardo@habkost.net>, Paolo Bonzini <pbonzini@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
[PATCH v3 1/2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Dov Murik 3 years, 11 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>
---
 hw/i386/pc_sysfw_ovmf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index f4dd92c588..df15c9737b 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "hw/i386/pc.h"
 #include "cpu.h"
 
@@ -66,7 +67,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 > (ptr - flash_ptr)) {
+        error_report("OVMF table has invalid size %d", tot_len);
+        return;
+    }
+
+    if (tot_len == 0) {
+        /* no entries in the OVMF table */
         return;
     }
 
-- 
2.25.1


Re: [PATCH v3 1/2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Dr. David Alan Gilbert 3 years, 11 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>

Thanks,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/i386/pc_sysfw_ovmf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
> index f4dd92c588..df15c9737b 100644
> --- a/hw/i386/pc_sysfw_ovmf.c
> +++ b/hw/i386/pc_sysfw_ovmf.c
> @@ -24,6 +24,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "hw/i386/pc.h"
>  #include "cpu.h"
>  
> @@ -66,7 +67,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 > (ptr - flash_ptr)) {
> +        error_report("OVMF table has invalid size %d", tot_len);
> +        return;
> +    }
> +
> +    if (tot_len == 0) {
> +        /* no entries in the OVMF table */
>          return;
>      }
>  
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH v3 1/2] hw/i386: Improve bounds checking in OVMF table parsing
Posted by Daniel P. Berrangé 3 years, 11 months ago
On Tue, Feb 22, 2022 at 07:19:05AM +0000, Dov Murik 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>
> ---
>  hw/i386/pc_sysfw_ovmf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|