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

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

Dov Murik posted 1 patch 3 years, 11 months ago

Diff against v2
Download mbox

Test checkpatch passed

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220214120857.1147288-1-dovmurik@linux.ibm.com

Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eduardo Habkost <eduardo@habkost.net>, Paolo Bonzini <pbonzini@redhat.com>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>

There is a newer version of this series

hw/i386/pc_sysfw_ovmf.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

Expand all Fold all

[PATCH] 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.

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..0663f3f54a 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 > flash_size - 50) {
+        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] hw/i386: Improve bounds checking in OVMF table parsing

Posted by Philippe Mathieu-Daudé via 3 years, 11 months ago

On 14/2/22 13:08, 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.
> 
> 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..0663f3f54a 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 > flash_size - 50) {

Please use a definition instead of this magic '50' number.

> +        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