diff mbox series

[v3,2/3] hw/smbios: report error if table size is too large

Message ID 20200923133804.2089190-3-berrange@redhat.com
State Superseded
Headers show
Series Add support for loading SMBIOS OEM strings from a file | expand

Commit Message

Daniel P. Berrangé Sept. 23, 2020, 1:38 p.m. UTC
The SMBIOS 2.1 entry point uses a uint16 data type for reporting the
total length of the tables. If the user passes -smbios configuration to
QEMU that causes the table size to exceed this limit then various bad
behaviours result, including

 - firmware hangs in an infinite loop
 - firmware triggers a KVM crash on bad memory access
 - firmware silently discards user's SMBIOS data replacing it with
   a generic data set.

Limiting the size to 0xffff in QEMU avoids triggering most of these
problems. There is a remaining bug in SeaBIOS which tries to prepend its
own data for table 0, and does not check whether there is sufficient
space before attempting this.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/smbios/smbios.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Laszlo Ersek Sept. 24, 2020, 7:13 a.m. UTC | #1
On 09/23/20 15:38, Daniel P. Berrangé wrote:
> The SMBIOS 2.1 entry point uses a uint16 data type for reporting the
> total length of the tables. If the user passes -smbios configuration to
> QEMU that causes the table size to exceed this limit then various bad
> behaviours result, including
> 
>  - firmware hangs in an infinite loop
>  - firmware triggers a KVM crash on bad memory access
>  - firmware silently discards user's SMBIOS data replacing it with
>    a generic data set.
> 
> Limiting the size to 0xffff in QEMU avoids triggering most of these
> problems. There is a remaining bug in SeaBIOS which tries to prepend its
> own data for table 0, and does not check whether there is sufficient
> space before attempting this.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/smbios/smbios.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index d993448087..8b30906e50 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -365,6 +365,13 @@ static void smbios_register_config(void)
>  
>  opts_init(smbios_register_config);
>  
> +/*
> + * The SMBIOS 2.1 "structure table length" field in the
> + * entry point uses a 16-bit integer, so we're limited
> + * in total table size
> + */
> +#define SMBIOS_21_MAX_TABLES_LEN 0xffff
> +
>  static void smbios_validate_table(MachineState *ms)
>  {
>      uint32_t expect_t4_count = smbios_legacy ?
> @@ -375,6 +382,13 @@ static void smbios_validate_table(MachineState *ms)
>                       expect_t4_count, smbios_type4_count);
>          exit(1);
>      }
> +
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
> +        smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
> +        error_report("SMBIOS 2.1 table length %zu exceeds %d",
> +                     smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
> +        exit(1);
> +    }
>  }
>  
>  
> 

Tested-by: Laszlo Ersek <lersek@redhat.com>
diff mbox series

Patch

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d993448087..8b30906e50 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -365,6 +365,13 @@  static void smbios_register_config(void)
 
 opts_init(smbios_register_config);
 
+/*
+ * The SMBIOS 2.1 "structure table length" field in the
+ * entry point uses a 16-bit integer, so we're limited
+ * in total table size
+ */
+#define SMBIOS_21_MAX_TABLES_LEN 0xffff
+
 static void smbios_validate_table(MachineState *ms)
 {
     uint32_t expect_t4_count = smbios_legacy ?
@@ -375,6 +382,13 @@  static void smbios_validate_table(MachineState *ms)
                      expect_t4_count, smbios_type4_count);
         exit(1);
     }
+
+    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21 &&
+        smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) {
+        error_report("SMBIOS 2.1 table length %zu exceeds %d",
+                     smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN);
+        exit(1);
+    }
 }