[PATCH v3 0/3] hw/smbios: fix table memory corruption with large memory vms

[PATCH v3 0/3] hw/smbios: fix table memory corruption with large memory vms

[Ani Sinha]

This patchset tries to address the issue raised in
https://bugzilla.redhat.com/show_bug.cgi?id=2023977 .

Patch 2 and patch 3 are dependent on patch 1.
Patch 1 makes some code cleanup to make thing more readable and also is
a preparatory patch for patches 2 and 3. Patch 2 is the actual fix.
Patch 3 adds an additional assertion because of changes introduced in
patch 2.

changelog:

v3: addressed Igor's comment on v2. reworded the commit log and the
comments in code to more accurately reflect the fix.
v2: split up patches into separate patches to keep things clean.

Ani Sinha (3):
  hw/smbios: code cleanup - use macro definitions for table header
    handles
  hw/smbios: fix table memory corruption with large memory vms
  hw/smbios: add assertion to ensure handles of tables 19 and 32 do not
    collide

 hw/smbios/smbios.c | 61 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] hw/smbios: code cleanup - use macro definitions for table header handles

[Ani Sinha]

This is a minor cleanup. Using macro definitions makes the code more
readable. It is at once clear which tables use which handle numbers in their
header. It also makes it easy to calculate the gaps between the numbers and
update them if needed.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/smbios/smbios.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 6013df1698..56b412ce35 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -549,9 +549,23 @@ bool smbios_skip_table(uint8_t type, bool required_table)
     return true;
 }
 
+#define T0_BASE 0x000
+#define T1_BASE 0x100
+#define T2_BASE 0x200
+#define T3_BASE 0x300
+#define T4_BASE 0x400
+#define T11_BASE 0xe00
+
+#define T16_BASE 0x1000
+#define T17_BASE 0x1100
+#define T19_BASE 0x1300
+#define T32_BASE 0x2000
+#define T41_BASE 0x2900
+#define T127_BASE 0x7F00
+
 static void smbios_build_type_0_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(0, 0x000, false); /* optional, leave up to BIOS */
+    SMBIOS_BUILD_TABLE_PRE(0, T0_BASE, false); /* optional, leave up to BIOS */
 
     SMBIOS_TABLE_SET_STR(0, vendor_str, type0.vendor);
     SMBIOS_TABLE_SET_STR(0, bios_version_str, type0.version);
@@ -599,7 +613,7 @@ static void smbios_encode_uuid(struct smbios_uuid *uuid, QemuUUID *in)
 
 static void smbios_build_type_1_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(1, 0x100, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(1, T1_BASE, true); /* required */
 
     SMBIOS_TABLE_SET_STR(1, manufacturer_str, type1.manufacturer);
     SMBIOS_TABLE_SET_STR(1, product_name_str, type1.product);
@@ -619,7 +633,7 @@ static void smbios_build_type_1_table(void)
 
 static void smbios_build_type_2_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(2, 0x200, false); /* optional */
+    SMBIOS_BUILD_TABLE_PRE(2, T2_BASE, false); /* optional */
 
     SMBIOS_TABLE_SET_STR(2, manufacturer_str, type2.manufacturer);
     SMBIOS_TABLE_SET_STR(2, product_str, type2.product);
@@ -637,7 +651,7 @@ static void smbios_build_type_2_table(void)
 
 static void smbios_build_type_3_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(3, 0x300, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(3, T3_BASE, true); /* required */
 
     SMBIOS_TABLE_SET_STR(3, manufacturer_str, type3.manufacturer);
     t->type = 0x01; /* Other */
@@ -662,7 +676,7 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
 {
     char sock_str[128];
 
-    SMBIOS_BUILD_TABLE_PRE(4, 0x400 + instance, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(4, T4_BASE + instance, true); /* required */
 
     snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
     SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
@@ -702,7 +716,7 @@ static void smbios_build_type_11_table(void)
         return;
     }
 
-    SMBIOS_BUILD_TABLE_PRE(11, 0xe00, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(11, T11_BASE, true); /* required */
 
     snprintf(count_str, sizeof(count_str), "%zu", type11.nvalues);
     t->count = type11.nvalues;
@@ -722,7 +736,7 @@ static void smbios_build_type_16_table(unsigned dimm_cnt)
 {
     uint64_t size_kb;
 
-    SMBIOS_BUILD_TABLE_PRE(16, 0x1000, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(16, T16_BASE, true); /* required */
 
     t->location = 0x01; /* Other */
     t->use = 0x03; /* System memory */
@@ -749,7 +763,7 @@ static void smbios_build_type_17_table(unsigned instance, uint64_t size)
     char loc_str[128];
     uint64_t size_mb;
 
-    SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(17, T17_BASE + instance, true); /* required */
 
     t->physical_memory_array_handle = cpu_to_le16(0x1000); /* Type 16 above */
     t->memory_error_information_handle = cpu_to_le16(0xFFFE); /* Not provided */
@@ -790,7 +804,7 @@ static void smbios_build_type_19_table(unsigned instance,
 {
     uint64_t end, start_kb, end_kb;
 
-    SMBIOS_BUILD_TABLE_PRE(19, 0x1300 + instance, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + instance, true); /* required */
 
     end = start + size - 1;
     assert(end > start);
@@ -814,7 +828,7 @@ static void smbios_build_type_19_table(unsigned instance,
 
 static void smbios_build_type_32_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(32, 0x2000, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(32, T32_BASE, true); /* required */
 
     memset(t->reserved, 0, 6);
     t->boot_status = 0; /* No errors detected */
@@ -828,7 +842,7 @@ static void smbios_build_type_41_table(Error **errp)
     struct type41_instance *t41;
 
     QTAILQ_FOREACH(t41, &type41, next) {
-        SMBIOS_BUILD_TABLE_PRE(41, 0x2900 + instance, true);
+        SMBIOS_BUILD_TABLE_PRE(41, T41_BASE + instance, true);
 
         SMBIOS_TABLE_SET_STR(41, reference_designation_str, t41->designation);
         t->device_type = t41->kind;
@@ -871,7 +885,7 @@ static void smbios_build_type_41_table(Error **errp)
 
 static void smbios_build_type_127_table(void)
 {
-    SMBIOS_BUILD_TABLE_PRE(127, 0x7F00, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(127, T127_BASE, true); /* required */
     SMBIOS_BUILD_TABLE_POST;
 }
 
-- 
2.25.1

[PATCH v3 2/3] hw/smbios: fix table memory corruption with large memory vms

[Ani Sinha]

The current smbios table implementation splits the main memory in 16 GiB
(DIMM like) chunks. With the current smbios table assignment code, we can have
only 512 such chunks before the 16 bit handle numbers in the header for tables
17 and 19 conflict. A guest with more than 8 TiB of memory will hit this
limitation and would fail with the following assertion in isa-debugcon:

ASSERT_EFI_ERROR (Status = Already started)
ASSERT /builddir/build/BUILD/edk2-ca407c7246bf/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c(125): !EFI_ERROR (Status)

This change adds an additional offset between tables 17 and 19 handle numbers
when configuring VMs larger than 8 TiB of memory. The value of the offset is
calculated to be equal to the additional space required to be reserved
in order to accomodate more DIMM entries without the table handles colliding.
In normal cases where the VM memory is smaller or equal to 8 TiB, this offset
value is 0. Hence in this case, no additional handle numbers are reserved and
table handle values remain as before.

As table handles are altered for large memory VMs, this change can break
migration in those cases. However, in those situations, qemu crashes anyway
without this fix and hence we do not preserve the old bug by introducing
compat knobs/machine types.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/smbios/smbios.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

changelog:
v3: reworded the commit log and comment in code.

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 56b412ce35..44c53797a4 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -799,12 +799,13 @@ static void smbios_build_type_17_table(unsigned instance, uint64_t size)
     SMBIOS_BUILD_TABLE_POST;
 }
 
-static void smbios_build_type_19_table(unsigned instance,
+static void smbios_build_type_19_table(unsigned instance, unsigned offset,
                                        uint64_t start, uint64_t size)
 {
     uint64_t end, start_kb, end_kb;
 
-    SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + instance, true); /* required */
+    SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + offset + instance,
+                           true); /* required */
 
     end = start + size - 1;
     assert(end > start);
@@ -996,7 +997,7 @@ void smbios_get_tables(MachineState *ms,
                        uint8_t **anchor, size_t *anchor_len,
                        Error **errp)
 {
-    unsigned i, dimm_cnt;
+    unsigned i, dimm_cnt, offset;
 
     if (smbios_legacy) {
         *tables = *anchor = NULL;
@@ -1026,6 +1027,16 @@ void smbios_get_tables(MachineState *ms,
 
         dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
 
+        /*
+         * The offset determines if we need to keep additional space betweeen
+         * table 17 and table 19 header handle numbers so that they do
+         * not overlap. For example, for a VM with larger than 8 TB guest
+         * memory and DIMM like chunks of 16 GiB, the default space between
+         * the two tables (T19_BASE - T17_BASE = 512) is not enough.
+         */
+        offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
+                 dimm_cnt - (T19_BASE - T17_BASE) : 0;
+
         smbios_build_type_16_table(dimm_cnt);
 
         for (i = 0; i < dimm_cnt; i++) {
@@ -1033,7 +1044,7 @@ void smbios_get_tables(MachineState *ms,
         }
 
         for (i = 0; i < mem_array_size; i++) {
-            smbios_build_type_19_table(i, mem_array[i].address,
+            smbios_build_type_19_table(i, offset, mem_array[i].address,
                                        mem_array[i].length);
         }
 
-- 
2.25.1

[PATCH v3 3/3] hw/smbios: add assertion to ensure handles of tables 19 and 32 do not collide

[Ani Sinha]

Since change b3cddba9c14b034 ("hw/smbios: fix table memory corruption with large memory vms")
we reserve additional space between handle numbers of tables 17 and 19 for
large VMs. This may cause table 19 to collide with table 32 in their handle
numbers for those large VMs. This change adds an assertion to ensure numbers
do not collide. If they do, qemu crashes with useful debug information for
taking additional steps.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/smbios/smbios.c | 6 ++++++
 1 file changed, 6 insertions(+)

changelog:
v3: reworded the commit log and comment in code.

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 44c53797a4..1f8d5c252f 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -1048,6 +1048,12 @@ void smbios_get_tables(MachineState *ms,
                                        mem_array[i].length);
         }
 
+        /*
+         * make sure 16 bit handle numbers in the headers of tables 19
+         * and 32 do not overlap.
+         */
+        assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
+
         smbios_build_type_32_table();
         smbios_build_type_38_table();
         smbios_build_type_41_table(errp);
-- 
2.25.1

Re: [PATCH v3 2/3] hw/smbios: fix table memory corruption with large memory vms

[Igor Mammedov]

On Mon, 14 Feb 2022 19:42:35 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> The current smbios table implementation splits the main memory in 16 GiB
> (DIMM like) chunks. With the current smbios table assignment code, we can have
> only 512 such chunks before the 16 bit handle numbers in the header for tables
> 17 and 19 conflict. A guest with more than 8 TiB of memory will hit this
> limitation and would fail with the following assertion in isa-debugcon:
> 
> ASSERT_EFI_ERROR (Status = Already started)
> ASSERT /builddir/build/BUILD/edk2-ca407c7246bf/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c(125): !EFI_ERROR (Status)
> 
> This change adds an additional offset between tables 17 and 19 handle numbers
> when configuring VMs larger than 8 TiB of memory. The value of the offset is
> calculated to be equal to the additional space required to be reserved
> in order to accomodate more DIMM entries without the table handles colliding.
> In normal cases where the VM memory is smaller or equal to 8 TiB, this offset
> value is 0. Hence in this case, no additional handle numbers are reserved and
> table handle values remain as before.
> 
> As table handles are altered for large memory VMs, this change can break
> migration in those cases. However, in those situations, qemu crashes anyway
1. pls explain how migration breaks
2. it's not QEMU which crashes

> without this fix and hence we do not preserve the old bug by introducing
> compat knobs/machine types.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977
s/buglink/Resolves/

> Signed-off-by: Ani Sinha <ani@anisinha.ca>

with above fixed

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/smbios/smbios.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> changelog:
> v3: reworded the commit log and comment in code.
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 56b412ce35..44c53797a4 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -799,12 +799,13 @@ static void smbios_build_type_17_table(unsigned instance, uint64_t size)
>      SMBIOS_BUILD_TABLE_POST;
>  }
>  
> -static void smbios_build_type_19_table(unsigned instance,
> +static void smbios_build_type_19_table(unsigned instance, unsigned offset,
>                                         uint64_t start, uint64_t size)
>  {
>      uint64_t end, start_kb, end_kb;
>  
> -    SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + instance, true); /* required */
> +    SMBIOS_BUILD_TABLE_PRE(19, T19_BASE + offset + instance,
> +                           true); /* required */
>  
>      end = start + size - 1;
>      assert(end > start);
> @@ -996,7 +997,7 @@ void smbios_get_tables(MachineState *ms,
>                         uint8_t **anchor, size_t *anchor_len,
>                         Error **errp)
>  {
> -    unsigned i, dimm_cnt;
> +    unsigned i, dimm_cnt, offset;
>  
>      if (smbios_legacy) {
>          *tables = *anchor = NULL;
> @@ -1026,6 +1027,16 @@ void smbios_get_tables(MachineState *ms,
>  
>          dimm_cnt = QEMU_ALIGN_UP(current_machine->ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
>  
> +        /*
> +         * The offset determines if we need to keep additional space betweeen
> +         * table 17 and table 19 header handle numbers so that they do
> +         * not overlap. For example, for a VM with larger than 8 TB guest
> +         * memory and DIMM like chunks of 16 GiB, the default space between
> +         * the two tables (T19_BASE - T17_BASE = 512) is not enough.
> +         */
> +        offset = (dimm_cnt > (T19_BASE - T17_BASE)) ? \
> +                 dimm_cnt - (T19_BASE - T17_BASE) : 0;
> +
>          smbios_build_type_16_table(dimm_cnt);
>  
>          for (i = 0; i < dimm_cnt; i++) {
> @@ -1033,7 +1044,7 @@ void smbios_get_tables(MachineState *ms,
>          }
>  
>          for (i = 0; i < mem_array_size; i++) {
> -            smbios_build_type_19_table(i, mem_array[i].address,
> +            smbios_build_type_19_table(i, offset, mem_array[i].address,
>                                         mem_array[i].length);
>          }
>  

Re: [PATCH v3 3/3] hw/smbios: add assertion to ensure handles of tables 19 and 32 do not collide

[Igor Mammedov]

On Mon, 14 Feb 2022 19:42:36 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> Since change b3cddba9c14b034 ("hw/smbios: fix table memory corruption with large memory vms")
> we reserve additional space between handle numbers of tables 17 and 19 for
> large VMs. This may cause table 19 to collide with table 32 in their handle
> numbers for those large VMs. This change adds an assertion to ensure numbers
> do not collide. If they do, qemu crashes with useful debug information for
> taking additional steps.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/smbios/smbios.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> changelog:
> v3: reworded the commit log and comment in code.
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 44c53797a4..1f8d5c252f 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -1048,6 +1048,12 @@ void smbios_get_tables(MachineState *ms,
>                                         mem_array[i].length);
>          }
>  
> +        /*
> +         * make sure 16 bit handle numbers in the headers of tables 19
> +         * and 32 do not overlap.
> +         */
> +        assert((mem_array_size + offset) < (T32_BASE - T19_BASE));
> +
>          smbios_build_type_32_table();
>          smbios_build_type_38_table();
>          smbios_build_type_41_table(errp);

Re: [PATCH v3 2/3] hw/smbios: fix table memory corruption with large memory vms

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]



On Thu, 17 Feb 2022, Igor Mammedov wrote:

> On Mon, 14 Feb 2022 19:42:35 +0530

> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977
> s/buglink/Resolves/
>

OK I am curious about this one.

Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html ,

If your patch fixes a bug in the gitlab bug tracker, please add a line
with “Resolves: <URL-of-the-bug>” to the commit message, too. Gitlab can
close bugs automatically once commits with the “Resolved:” keyword get
merged into the master branch of the project. And if your patch addresses
a bug in another public bug tracker, you can also use a line with
“Buglink: <URL-of-the-bug>” for reference here, too.

So I considered redhar BZ as a public bug tracker as well. Does the BZ
also automatically close bugs when we use "Resolves:"? Should we update
the instructions here?

Re: [PATCH v3 2/3] hw/smbios: fix table memory corruption with large memory vms

[Igor Mammedov]

On Thu, 17 Feb 2022 19:02:05 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Thu, 17 Feb 2022, Igor Mammedov wrote:
> 
> > On Mon, 14 Feb 2022 19:42:35 +0530  
> 
> > >
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2023977  
> > s/buglink/Resolves/
> >  
> 
> OK I am curious about this one.
> 
> Per https://www.qemu.org/docs/master/devel/submitting-a-patch.html ,
> 
> If your patch fixes a bug in the gitlab bug tracker, please add a line
> with “Resolves: <URL-of-the-bug>” to the commit message, too. Gitlab can
> close bugs automatically once commits with the “Resolved:” keyword get
> merged into the master branch of the project. And if your patch addresses
> a bug in another public bug tracker, you can also use a line with
> “Buglink: <URL-of-the-bug>” for reference here, too.
> 
> So I considered redhar BZ as a public bug tracker as well. Does the BZ
> also automatically close bugs when we use "Resolves:"? Should we update
> the instructions here?

I might have been wrong suggesting "Resolves" tag, it will/should not affect bugzilla.
Looking at history, shows a zoo of used tags in bugzilla case,
so I'd guess Buglink is fine as any other.