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

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

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

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

-- 
2.25.1

[PATCH v2 1/3] hw/smbios: code cleanup - use macro definitions for table base addresses

[Ani Sinha]

This is a minor cleanup. Using macro definitions makes the code more
readable. It is at once clear which tables use which starting base addresses.
It also makes it easy to calculate the gaps between the addresses and modify
them if needed.

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

Re: [PATCH v2 1/3] hw/smbios: code cleanup - use macro definitions for table base addresses

[Igor Mammedov]

On Mon,  7 Feb 2022 17:01:27 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> This is a minor cleanup. Using macro definitions makes the code more
> readable. It is at once clear which tables use which starting base addresses.
> It also makes it easy to calculate the gaps between the addresses and modify
> them if needed.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

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

> ---
>  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;
>  }
>  

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

[Ani Sinha]

With the current smbios table assignment code, we can have only 512 DIMM slots
(each DIMM of 16 GiB in size) before tables 17 and 19 conflict with their
addresses. 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 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 between the tables
in order to accomodate more DIMM devices without the table memories 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 memory space is reserved and
table addresses remain as before.

Since table addresses 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(-)

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 56b412ce35..d7de740363 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 so that they do not overlap. For example,
+         * for a VM with larger than 8 TB guest memory and DIMM size 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

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

[Igor Mammedov]

On Mon,  7 Feb 2022 17:01:28 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> With the current smbios table assignment code, we can have only 512 DIMM slots
it's a bit confusing, since it's not DIMM slots in QEMU sense (we do not expose
DIMM devices via SMBIOS/E820). So maybe clarify here that initial RAM is split
into 16GB (with 'DIMM' type ) chunks/entries when it's described in SMBIOS table 17.

> (each DIMM of 16 GiB in size) before tables 17 and 19 conflict with their
> addresses.

Are you sure it's addresses that are wrong? 

> 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 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 between the tables
> in order to accomodate more DIMM devices without the table memories colliding.

s/DIMM devices/DIMM entries/

not sure what 'table memories colliding' is, maybe rephrase and
be more accurate about what happens here.

> 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 memory space is reserved and
> table addresses remain as before.
> 
> Since table addresses 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(-)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 56b412ce35..d7de740363 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 so that they do not overlap. For example,

it's not tables that overlap, but something else

> +         * for a VM with larger than 8 TB guest memory and DIMM size 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);
>          }
>  

other than used language/terminology, the rest looks fine tom

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

[Ani Sinha]

On Mon, Feb 14, 2022 at 6:21 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Mon,  7 Feb 2022 17:01:28 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > With the current smbios table assignment code, we can have only 512 DIMM slots
> it's a bit confusing, since it's not DIMM slots in QEMU sense (we do not expose
> DIMM devices via SMBIOS/E820). So maybe clarify here that initial RAM is split
> into 16GB (with 'DIMM' type ) chunks/entries when it's described in SMBIOS table 17.
>
> > (each DIMM of 16 GiB in size) before tables 17 and 19 conflict with their
> > addresses.
>
> Are you sure it's addresses that are wrong?

I don't know why I had this pre conception of memory corruption and
overlapping addresses! Even the BZ says table handles overlap. Grr ...
doing too much multiplexing these days :(

[PATCH v2 3/3] hw/smbios: add an assertion to ensure 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 memory space between tables 17 and 19 for large VMs.
This may cause table 19 to collide with table 32 for those VMs. This change
adds an assertion to make sure table 19 does not extend into the memory used
by table 32.

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

diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index d7de740363..800a35e9a5 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -1048,6 +1048,9 @@ void smbios_get_tables(MachineState *ms,
                                        mem_array[i].length);
         }
 
+        /* we need to make sure table 19 and table 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 v2 3/3] hw/smbios: add an assertion to ensure tables 19 and 32 do not collide

[Igor Mammedov]

On Mon,  7 Feb 2022 17:01:29 +0530
Ani Sinha <ani@anisinha.ca> wrote:

> Since change b3cddba9c14b034 ("hw/smbios: fix table memory corruption with large memory vms")
> we reserve additional memory space between tables 17 and 19 for large VMs.
> This may cause table 19 to collide with table 32 for those VMs. This change
> adds an assertion to make sure table 19 does not extend into the memory used
> by table 32.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  hw/smbios/smbios.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index d7de740363..800a35e9a5 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -1048,6 +1048,9 @@ void smbios_get_tables(MachineState *ms,
>                                         mem_array[i].length);
>          }
>  
> +        /* we need to make sure table 19 and table 32 do not overlap */
same as in 2/3 (here and commit message), tables 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 v2 0/3] hw/smbios: fix table memory corruption with large memory vms

[Michael S. Tsirkin]

On Mon, Feb 07, 2022 at 05:01:26PM +0530, Ani Sinha wrote:
> 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.


Ani, typically it is best to copy everyone on the cover letter.
There are exceptions e.g. when the patchset is huge and the
CC list would grow too big, but that is the rule. Thanks!

> Ani Sinha (3):
>   hw/smbios: code cleanup - use macro definitions for table base
>     addresses
>   hw/smbios: fix table memory corruption with large memory vms
>   hw/smbios: add an assertion to ensure tables 19 and 32 do not collide
> 
>  hw/smbios/smbios.c | 58 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 15 deletions(-)
> 
> -- 
> 2.25.1
> 
> 
> 

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

[Ani Sinha]

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

On Mon, Feb 14, 2022 at 20:04 Michael S. Tsirkin <mst@redhat.com> wrote:

>
> On Mon, Feb 07, 2022 at 05:01:26PM +0530, Ani Sinha wrote:
> > 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.
>
>
> Ani, typically it is best to copy everyone on the cover letter.
> There are exceptions e.g. when the patchset is huge and the
> CC list would grow too big, but that is the rule. Thanks!
>
>
Yes that was not intentional. I overlooked the fact that the git
 send-email was not adding you and Igor both. Sorry about that. With v3
which I sent out earlier, I’ve explicitly added you both.


>
>

[-- Attachment #2: Type: text/html, Size: 1703 bytes --]