[PATCH v2 0/3] hvmloader: add new SMBIOS tables (7,8,9,26,27,28)

[PATCH v2 0/3] hvmloader: add new SMBIOS tables (7,8,9,26,27,28)

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Changes since v1:
- Swapped the order of S-o-b in the last commit message.

Resubmitting patch from Anton Belousov and addressing review comments
from Jan: https://old-list-archives.xen.org/archives/html/xen-devel/2022-01/msg00725.html

Original message:
> SMBIOS tables like 7,8,9,26,27,28 are neccessary to prevent sandbox detection
> by malware using WMI-queries. New tables can be mapped to memory from binary
> file specified in "smbios_firmware" parameter of domain configuration.
> If particular table is absent in binary file, then it will not be mapped to
> memory. This method works for Windows domains as tables 7,8,9,26,27,28 are not
> critical for OS boot and runtime. Also if "smbios_firmware" parameter is not
> provided, these tables will be skipped in write_smbios_tables function.

Further explanation:
Some malware samples are known to check presence of various hardware components
(like CPU fan, CPU temperature sensor, etc.) by WMI queries. If these components
are not present, then malware can assume that it is running in a sandbox and
will not execute its payload.

This patch will allow security researchers to create a custom SMBIOS
firmware binary file that contains these tables.

Petr Beneš (3):
  hvmloader: clean up SMBIOS code style and formatting
  hvmloader: fix SMBIOS table length checks
  hvmloader: add new SMBIOS tables (7, 8, 9, 26, 27, 28)

 tools/firmware/hvmloader/smbios.c       | 354 +++++++++++++++---------
 tools/firmware/hvmloader/smbios_types.h |  83 +++++-
 2 files changed, 299 insertions(+), 138 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] hvmloader: clean up SMBIOS code style and formatting

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

* Removed trailing whitespaces
* Removed unnecessary pointer casts
* Added whitespaces around &&
* Removed superfluous parentheses
* Use variables in sizeof() where applicable

No functional change.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/firmware/hvmloader/smbios.c | 162 +++++++++++++++---------------
 1 file changed, 81 insertions(+), 81 deletions(-)

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 97a054e9e3..077c88c41c 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -67,7 +67,7 @@ static void *
 smbios_type_0_init(void *start, const char *xen_version,
                    uint32_t xen_major_version, uint32_t xen_minor_version);
 static void *
-smbios_type_1_init(void *start, const char *xen_version, 
+smbios_type_1_init(void *start, const char *xen_version,
                    uint8_t uuid[16]);
 static void *
 smbios_type_2_init(void *start);
@@ -242,8 +242,8 @@ get_memsize(void)
         sz += (((uint64_t)hvm_info->high_mem_pgend << PAGE_SHIFT) - GB(4));
 
     /*
-     * Round up to the nearest MB.  The user specifies domU pseudo-physical 
-     * memory in megabytes, so not doing this could easily lead to reporting 
+     * Round up to the nearest MB.  The user specifies domU pseudo-physical
+     * memory in megabytes, so not doing this could easily lead to reporting
      * one less MB than the user specified.
      */
     return (sz + MB(1) - 1) >> 20;
@@ -378,24 +378,24 @@ static void *
 smbios_type_0_init(void *start, const char *xen_version,
                    uint32_t xen_major_version, uint32_t xen_minor_version)
 {
-    struct smbios_type_0 *p = (struct smbios_type_0 *)start;
+    struct smbios_type_0 *p = start;
     static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
     void *pts;
     uint32_t length;
 
     pts = get_smbios_pt_struct(0, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE0;
-        return (start + length);
+        return start + length;
     }
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 0;
-    p->header.length = sizeof(struct smbios_type_0);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE0;
 
     p->vendor_str = 1;
@@ -411,12 +411,12 @@ smbios_type_0_init(void *start, const char *xen_version,
     /* Extended Characteristics: Enable Targeted Content Distribution. */
     p->characteristics_extension_bytes[1] = 0x04;
 
-    p->major_release = (uint8_t) xen_major_version;
-    p->minor_release = (uint8_t) xen_minor_version;
+    p->major_release = (uint8_t)xen_major_version;
+    p->minor_release = (uint8_t)xen_minor_version;
     p->embedded_controller_major = 0xff;
     p->embedded_controller_minor = 0xff;
 
-    start += sizeof(struct smbios_type_0);
+    start += sizeof(*p);
     s = xenstore_read(HVM_XS_BIOS_VENDOR, "Xen");
     strcpy((char *)start, s);
     start += strlen(s) + 1;
@@ -434,42 +434,42 @@ smbios_type_0_init(void *start, const char *xen_version,
 
 /* Type 1 -- System Information */
 static void *
-smbios_type_1_init(void *start, const char *xen_version, 
+smbios_type_1_init(void *start, const char *xen_version,
                    uint8_t uuid[16])
 {
     char uuid_str[37];
-    struct smbios_type_1 *p = (struct smbios_type_1 *)start;
+    struct smbios_type_1 *p = start;
     const char *s;
     void *pts;
     uint32_t length;
 
     pts = get_smbios_pt_struct(1, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE1;
-        return (start + length);
+        return start + length;
     }
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 1;
-    p->header.length = sizeof(struct smbios_type_1);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE1;
 
     p->manufacturer_str = 1;
     p->product_name_str = 2;
     p->version_str = 3;
     p->serial_number_str = 4;
-    
+
     memcpy(p->uuid, uuid, 16);
 
     p->wake_up_type = 0x06; /* power switch */
     p->sku_str = 0;
     p->family_str = 0;
 
-    start += sizeof(struct smbios_type_1);
-    
+    start += sizeof(*p);
+
     s = xenstore_read(HVM_XS_SYSTEM_MANUFACTURER, "Xen");
     strcpy((char *)start, s);
     start += strlen(s) + 1;
@@ -482,21 +482,21 @@ smbios_type_1_init(void *start, const char *xen_version,
     strcpy((char *)start, s);
     start += strlen(s) + 1;
 
-    uuid_to_string(uuid_str, uuid); 
+    uuid_to_string(uuid_str, uuid);
     s = xenstore_read(HVM_XS_SYSTEM_SERIAL_NUMBER, uuid_str);
     strcpy((char *)start, s);
     start += strlen(s) + 1;
 
     *((uint8_t *)start) = 0;
-    
-    return start+1; 
+
+    return start+1;
 }
 
 /* Type 2 -- System Board */
 static void *
 smbios_type_2_init(void *start)
 {
-    struct smbios_type_2 *p = (struct smbios_type_2 *)start;
+    struct smbios_type_2 *p = start;
     const char *s;
     uint8_t *ptr;
     void *pts;
@@ -504,7 +504,7 @@ smbios_type_2_init(void *start)
     unsigned int counter = 0;
 
     pts = get_smbios_pt_struct(2, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE2;
@@ -517,12 +517,12 @@ smbios_type_2_init(void *start)
                 *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
         }
 
-        return (start + length);
+        return start + length;
     }
 
     memset(p, 0, sizeof(*p));
     p->header.type = 2;
-    p->header.length = sizeof(struct smbios_type_2);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE2;
     p->feature_flags = 0x09; /* Board is a hosting board and replaceable */
     p->chassis_handle = SMBIOS_HANDLE_TYPE3;
@@ -530,7 +530,7 @@ smbios_type_2_init(void *start)
     start += sizeof(*p);
 
     s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -538,7 +538,7 @@ smbios_type_2_init(void *start)
     }
 
     s = xenstore_read(HVM_XS_BASEBOARD_PRODUCT_NAME, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -546,7 +546,7 @@ smbios_type_2_init(void *start)
     }
 
     s = xenstore_read(HVM_XS_BASEBOARD_VERSION, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -554,7 +554,7 @@ smbios_type_2_init(void *start)
     }
 
     s = xenstore_read(HVM_XS_BASEBOARD_SERIAL_NUMBER, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -562,7 +562,7 @@ smbios_type_2_init(void *start)
     }
 
     s = xenstore_read(HVM_XS_BASEBOARD_ASSET_TAG, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -570,7 +570,7 @@ smbios_type_2_init(void *start)
     }
 
     s = xenstore_read(HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -591,24 +591,24 @@ smbios_type_2_init(void *start)
 static void *
 smbios_type_3_init(void *start)
 {
-    struct smbios_type_3 *p = (struct smbios_type_3 *)start;
+    struct smbios_type_3 *p = start;
     const char *s;
     void *pts;
     uint32_t length;
     uint32_t counter = 0;
 
     pts = get_smbios_pt_struct(3, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE3;
-        return (start + length);
+        return start + length;
     }
     
     memset(p, 0, sizeof(*p));
 
     p->header.type = 3;
-    p->header.length = sizeof(struct smbios_type_3);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE3;
 
     p->manufacturer_str = ++counter;
@@ -621,22 +621,22 @@ smbios_type_3_init(void *start)
     p->thermal_state = 0x03; /* safe */
     p->security_status = 0x02; /* unknown */
 
-    start += sizeof(struct smbios_type_3);
-    
+    start += sizeof(*p);
+
     s = xenstore_read(HVM_XS_ENCLOSURE_MANUFACTURER, "Xen");
     strcpy((char *)start, s);
     start += strlen(s) + 1;
 
     /* No internal defaults for following ones if the value is not set */
     s = xenstore_read(HVM_XS_ENCLOSURE_SERIAL_NUMBER, NULL);
-    if ( (s != NULL)&&(*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy((char *)start, s);
         start += strlen(s) + 1;
         p->serial_number_str = ++counter;
     }
     s = xenstore_read(HVM_XS_ENCLOSURE_ASSET_TAG, NULL);
-    if ( (s != NULL) && (*s != '\0') )
+    if ( s != NULL && *s != '\0' )
     {
         strcpy(start, s);
         start += strlen(s) + 1;
@@ -652,14 +652,14 @@ static void *
 smbios_type_4_init(
     void *start, unsigned int cpu_number, char *cpu_manufacturer)
 {
-    char buf[80]; 
-    struct smbios_type_4 *p = (struct smbios_type_4 *)start;
+    char buf[80];
+    struct smbios_type_4 *p = start;
     uint32_t eax, ebx, ecx, edx;
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 4;
-    p->header.length = sizeof(struct smbios_type_4);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE4 + cpu_number;
 
     p->socket_designation_str = 1;
@@ -684,7 +684,7 @@ smbios_type_4_init(
     p->l2_cache_handle = 0xffff; /* No cache information structure provided. */
     p->l3_cache_handle = 0xffff; /* No cache information structure provided. */
 
-    start += sizeof(struct smbios_type_4);
+    start += sizeof(*p);
 
     strncpy(buf, "CPU ", sizeof(buf));
     if ( (sizeof(buf) - strlen("CPU ")) >= 3 )
@@ -702,9 +702,9 @@ smbios_type_4_init(
 
 /* Type 11 -- OEM Strings */
 static void *
-smbios_type_11_init(void *start) 
+smbios_type_11_init(void *start)
 {
-    struct smbios_type_11 *p = (struct smbios_type_11 *)start;
+    struct smbios_type_11 *p = start;
     char path[20];
     const char *s;
     int i;
@@ -712,20 +712,20 @@ smbios_type_11_init(void *start)
     uint32_t length;
 
     pts = get_smbios_pt_struct(11, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE11;
-        return (start + length);
+        return start + length;
     }
 
     p->header.type = 11;
-    p->header.length = sizeof(struct smbios_type_11);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE11;
 
     p->count = 0;
 
-    start += sizeof(struct smbios_type_11);
+    start += sizeof(*p);
 
     /* Pull out as many oem-* strings we find in xenstore */
     for ( i = 1; i < 100; i++ )
@@ -737,7 +737,7 @@ smbios_type_11_init(void *start)
         start += strlen(s) + 1;
         p->count++;
     }
-    
+
     /* Make sure there's at least one type-11 string */
     if ( p->count == 0 )
     {
@@ -754,14 +754,14 @@ smbios_type_11_init(void *start)
 static void *
 smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
 {
-    struct smbios_type_16 *p = (struct smbios_type_16*)start;
+    struct smbios_type_16 *p = start;
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 16;
     p->header.handle = SMBIOS_HANDLE_TYPE16;
-    p->header.length = sizeof(struct smbios_type_16);
-    
+    p->header.length = sizeof(*p);
+
     p->location = 0x01; /* other */
     p->use = 0x03; /* system memory */
     p->error_correction = 0x06; /* Multi-bit ECC to make Microsoft happy */
@@ -769,7 +769,7 @@ smbios_type_16_init(void *start, uint32_t memsize, int nr_mem_devs)
     p->memory_error_information_handle = 0xfffe; /* none provided */
     p->number_of_memory_devices = nr_mem_devs;
 
-    start += sizeof(struct smbios_type_16);
+    start += sizeof(*p);
     *((uint16_t *)start) = 0;
     return start + 2;
 }
@@ -779,12 +779,12 @@ static void *
 smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 {
     char buf[16];
-    struct smbios_type_17 *p = (struct smbios_type_17 *)start;
-    
+    struct smbios_type_17 *p = start;
+
     memset(p, 0, sizeof(*p));
 
     p->header.type = 17;
-    p->header.length = sizeof(struct smbios_type_17);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE17 + instance;
 
     p->physical_memory_array_handle = 0x1000;
@@ -799,7 +799,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
     p->memory_type = 0x07; /* RAM */
     p->type_detail = 0;
 
-    start += sizeof(struct smbios_type_17);
+    start += sizeof(*p);
     strcpy(start, "DIMM ");
     start += strlen("DIMM ");
     itoa(buf, instance);
@@ -814,12 +814,12 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 static void *
 smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 {
-    struct smbios_type_19 *p = (struct smbios_type_19 *)start;
-    
+    struct smbios_type_19 *p = start;
+
     memset(p, 0, sizeof(*p));
 
     p->header.type = 19;
-    p->header.length = sizeof(struct smbios_type_19);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE19 + instance;
 
     p->starting_address = instance << 24;
@@ -827,7 +827,7 @@ smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
     p->memory_array_handle = 0x1000;
     p->partition_width = 1;
 
-    start += sizeof(struct smbios_type_19);
+    start += sizeof(*p);
     *((uint16_t *)start) = 0;
     return start + 2;
 }
@@ -836,12 +836,12 @@ smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 static void *
 smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
 {
-    struct smbios_type_20 *p = (struct smbios_type_20 *)start;
+    struct smbios_type_20 *p = start;
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 20;
-    p->header.length = sizeof(struct smbios_type_20);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE20 + instance;
 
     p->starting_address = instance << 24;
@@ -852,7 +852,7 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
     p->interleave_position = 0;
     p->interleaved_data_depth = 0;
 
-    start += sizeof(struct smbios_type_20);
+    start += sizeof(*p);
 
     *((uint16_t *)start) = 0;
     return start+2;
@@ -862,18 +862,18 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance)
 static void *
 smbios_type_22_init(void *start)
 {
-    struct smbios_type_22 *p = (struct smbios_type_22 *)start;
+    struct smbios_type_22 *p = start;
     static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
     void *pts;
     uint32_t length;
 
     pts = get_smbios_pt_struct(22, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE22;
-        return (start + length);
+        return start + length;
     }
 
     s = xenstore_read(HVM_XS_SMBIOS_DEFAULT_BATTERY, "0");
@@ -883,7 +883,7 @@ smbios_type_22_init(void *start)
     memset(p, 0, sizeof(*p));
 
     p->header.type = 22;
-    p->header.length = sizeof(struct smbios_type_22);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE22;
 
     p->location_str = 1;
@@ -902,7 +902,7 @@ smbios_type_22_init(void *start)
     p->design_capacity_multiplier = 0;
     p->oem_specific = 0;
 
-    start += sizeof(struct smbios_type_22);
+    start += sizeof(*p);
 
     strcpy((char *)start, "Primary");
     start += strlen("Primary") + 1;
@@ -920,24 +920,24 @@ smbios_type_22_init(void *start)
 
     *((uint8_t *)start) = 0;
 
-    return start+1; 
+    return start+1;
 }
 
 /* Type 32 -- System Boot Information */
 static void *
 smbios_type_32_init(void *start)
 {
-    struct smbios_type_32 *p = (struct smbios_type_32 *)start;
+    struct smbios_type_32 *p = start;
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 32;
-    p->header.length = sizeof(struct smbios_type_32);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE32;
     memset(p->reserved, 0, 6);
     p->boot_status = 0; /* no errors detected */
-    
-    start += sizeof(struct smbios_type_32);
+
+    start += sizeof(*p);
     *((uint16_t *)start) = 0;
     return start+2;
 }
@@ -946,16 +946,16 @@ smbios_type_32_init(void *start)
 static void *
 smbios_type_39_init(void *start)
 {
-    struct smbios_type_39 *p = (struct smbios_type_39 *)start;
+    struct smbios_type_39 *p = start;
     void *pts;
     uint32_t length;
 
     pts = get_smbios_pt_struct(39, &length);
-    if ( (pts != NULL)&&(length > 0) )
+    if ( pts != NULL && length > 0 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE39;
-        return (start + length);
+        return start + length;
     }
 
     /* Only present when passed in */
@@ -998,15 +998,15 @@ smbios_type_vendor_oem_init(void *start)
 static void *
 smbios_type_127_init(void *start)
 {
-    struct smbios_type_127 *p = (struct smbios_type_127 *)start;
+    struct smbios_type_127 *p = start;
 
     memset(p, 0, sizeof(*p));
 
     p->header.type = 127;
-    p->header.length = sizeof(struct smbios_type_127);
+    p->header.length = sizeof(*p);
     p->header.handle = SMBIOS_HANDLE_TYPE127;
 
-    start += sizeof(struct smbios_type_127);
+    start += sizeof(*p);
     *((uint16_t *)start) = 0;
     return start + 2;
 }
-- 
2.34.1

[PATCH v2 2/3] hvmloader: fix SMBIOS table length checks

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

SMBIOS specification dictates that tables should have a minimal length.
This commit introduces further validation for user-input SMBIOS tables.

As per SMBIOS Reference Specification:
* Type 0: For version 2.3 and later implementations, the length is at least 14h
* Type 1: 1Bh for 2.4 and later
* Type 2: at least 08h
* Type 3: 0Dh for version 2.1 and later
* Type 11: 5h (+ strings)
* Type 22: 1Ah (+ strings)
* Type 39: a minimum of 10h

Notably, this also fixes previously incorrect check for chassis handle in smbios_type_2_init.
Chassis handle is a WORD, therefore, the condition now correctly checks for >= 13 instead of > 13.

hvmloader currently implements version 2.4

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 tools/firmware/hvmloader/smbios.c       | 135 ++++++++++++------------
 tools/firmware/hvmloader/smbios_types.h |   6 +-
 2 files changed, 69 insertions(+), 72 deletions(-)

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 077c88c41c..93bfea3e6e 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -47,6 +47,8 @@ static void
 smbios_pt_init(void);
 static void*
 get_smbios_pt_struct(uint8_t type, uint32_t *length_out);
+static void *
+smbios_pt_copy(void *start, uint8_t type, uint16_t handle, size_t table_size);
 static void
 get_cpu_manufacturer(char *buf, int len);
 static int
@@ -154,6 +156,25 @@ get_smbios_pt_struct(uint8_t type, uint32_t *length_out)
     return NULL;
 }
 
+static void *
+smbios_pt_copy(void *start, uint8_t type, uint16_t handle, size_t table_size)
+{
+    struct smbios_structure_header *header = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(type, &length);
+    if ( pts != NULL && length >= table_size )
+    {
+        memcpy(start, pts, length);
+        header->handle = handle;
+        return start + length;
+    }
+
+    return start;
+}
+
 static void
 get_cpu_manufacturer(char *buf, int len)
 {
@@ -381,16 +402,11 @@ smbios_type_0_init(void *start, const char *xen_version,
     struct smbios_type_0 *p = start;
     static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
-    void *pts;
-    uint32_t length;
+    void* next;
 
-    pts = get_smbios_pt_struct(0, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE0;
-        return start + length;
-    }
+    next = smbios_pt_copy(start, 0, SMBIOS_HANDLE_TYPE0, sizeof(*p));
+    if ( next != start )
+        return next;
 
     memset(p, 0, sizeof(*p));
 
@@ -440,16 +456,11 @@ smbios_type_1_init(void *start, const char *xen_version,
     char uuid_str[37];
     struct smbios_type_1 *p = start;
     const char *s;
-    void *pts;
-    uint32_t length;
+    void* next;
 
-    pts = get_smbios_pt_struct(1, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE1;
-        return start + length;
-    }
+    next = smbios_pt_copy(start, 1, SMBIOS_HANDLE_TYPE1, sizeof(*p));
+    if ( next != start )
+        return next;
 
     memset(p, 0, sizeof(*p));
 
@@ -499,25 +510,27 @@ smbios_type_2_init(void *start)
     struct smbios_type_2 *p = start;
     const char *s;
     uint8_t *ptr;
-    void *pts;
-    uint32_t length;
+    void *next;
     unsigned int counter = 0;
 
-    pts = get_smbios_pt_struct(2, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE2;
+    /*
+     * Specification says Type 2 table has length of at least 08h,
+     * which corresponds with "Asset Tag" field offset.
+     */
 
+    next = smbios_pt_copy(start, 2, SMBIOS_HANDLE_TYPE2, sizeof(*p));
+    if ( next != start )
+    {
         /* Set current chassis handle if present */
-        if ( p->header.length > 13 )
+        if ( p->header.length >= offsetof(struct smbios_type_2, board_type) )
         {
-            ptr = ((uint8_t*)start) + 11;            
+            ptr = ((uint8_t*)start) + offsetof(struct smbios_type_2,
+                                               chassis_handle);
             if ( *((uint16_t*)ptr) != 0 )
                 *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
         }
 
-        return start + length;
+        return next;
     }
 
     memset(p, 0, sizeof(*p));
@@ -593,18 +606,18 @@ smbios_type_3_init(void *start)
 {
     struct smbios_type_3 *p = start;
     const char *s;
-    void *pts;
-    uint32_t length;
+    void *next;
     uint32_t counter = 0;
 
-    pts = get_smbios_pt_struct(3, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE3;
-        return start + length;
-    }
-    
+    /*
+     * Specification says Type 3 table has length of at least 0Dh (for v2.1+),
+     * which corresponds with "OEM-defined" field offset.
+     */
+
+    next = smbios_pt_copy(start, 3, SMBIOS_HANDLE_TYPE3, sizeof(*p));
+    if ( next != start )
+        return next;
+
     memset(p, 0, sizeof(*p));
 
     p->header.type = 3;
@@ -707,17 +720,12 @@ smbios_type_11_init(void *start)
     struct smbios_type_11 *p = start;
     char path[20];
     const char *s;
+    void *next;
     int i;
-    void *pts;
-    uint32_t length;
 
-    pts = get_smbios_pt_struct(11, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE11;
-        return start + length;
-    }
+    next = smbios_pt_copy(start, 11, SMBIOS_HANDLE_TYPE11, sizeof(*p));
+    if ( next != start )
+        return next;
 
     p->header.type = 11;
     p->header.length = sizeof(*p);
@@ -865,16 +873,11 @@ smbios_type_22_init(void *start)
     struct smbios_type_22 *p = start;
     static const char *smbios_release_date = __SMBIOS_DATE__;
     const char *s;
-    void *pts;
-    uint32_t length;
+    void *next;
 
-    pts = get_smbios_pt_struct(22, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE22;
-        return start + length;
-    }
+    next = smbios_pt_copy(start, 22, SMBIOS_HANDLE_TYPE22, sizeof(*p));
+    if ( next != start )
+        return next;
 
     s = xenstore_read(HVM_XS_SMBIOS_DEFAULT_BATTERY, "0");
     if ( strncmp(s, "1", 1) != 0 )
@@ -946,20 +949,14 @@ smbios_type_32_init(void *start)
 static void *
 smbios_type_39_init(void *start)
 {
-    struct smbios_type_39 *p = start;
-    void *pts;
-    uint32_t length;
-
-    pts = get_smbios_pt_struct(39, &length);
-    if ( pts != NULL && length > 0 )
-    {
-        memcpy(start, pts, length);
-        p->header.handle = SMBIOS_HANDLE_TYPE39;
-        return start + length;
-    }
+    /*
+     * Specification says Type 39 table has length of at least 10h,
+     * which corresponds with "Input Voltage Probe Handle" offset.
+     */
 
-    /* Only present when passed in */
-    return start;
+    return smbios_pt_copy(start, 39, SMBIOS_HANDLE_TYPE39,
+                          offsetof(struct smbios_type_39,
+                                   input_voltage_probe_handle));
 }
 
 static void *
diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h
index 7c648ece71..656b2a51ad 100644
--- a/tools/firmware/hvmloader/smbios_types.h
+++ b/tools/firmware/hvmloader/smbios_types.h
@@ -252,9 +252,9 @@ struct smbios_type_39 {
     uint8_t revision_level_str;
     uint16_t max_capacity;
     uint16_t characteristics;
-    uint16_t input_voltage_probe_handle;
-    uint16_t cooling_device_handle;
-    uint16_t input_current_probe_handle;
+    uint16_t input_voltage_probe_handle;    /* Optional */
+    uint16_t cooling_device_handle;         /* Optional */
+    uint16_t input_current_probe_handle;    /* Optional */
 } __attribute__ ((packed));
 
 /* SMBIOS type 127 -- End-of-table */
-- 
2.34.1

[PATCH v2 3/3] hvmloader: add new SMBIOS tables (7, 8, 9, 26, 27, 28)

[Petr Beneš]

From: Petr Beneš <w1benny@gmail.com>

Some SMBIOS tables are used by certain malware families to detect virtualized
environments via WMI queries.

To improve stealth for sandboxing purposes, this patch adds support
for populating these SMBIOS tables from an external binary specified
via the "smbios_firmware" domain config option:

* 7 - Cache Info
* 8 - Port Connector
* 9 - System Slots
* 26 - Voltage Probe
* 27 - Cooling Device
* 28 - Temperature Probe

If particular table is absent in binary file, then it will not be mapped to
memory. This method works for Windows domains as tables 7,8,9,26,27,28 are not
critical for OS boot and runtime. Also if "smbios_firmware" parameter is not
provided, these tables will be skipped in write_smbios_tables function.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
Signed-off-by: Anton Belousov <blsvntn@outlook.com>
---
 tools/firmware/hvmloader/smbios.c       | 87 +++++++++++++++++++++++++
 tools/firmware/hvmloader/smbios_types.h | 77 ++++++++++++++++++++++
 2 files changed, 164 insertions(+)

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 93bfea3e6e..c67336cffa 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -33,12 +33,18 @@
 #define SMBIOS_HANDLE_TYPE2   0x0200
 #define SMBIOS_HANDLE_TYPE3   0x0300
 #define SMBIOS_HANDLE_TYPE4   0x0400
+#define SMBIOS_HANDLE_TYPE7   0x0700
+#define SMBIOS_HANDLE_TYPE8   0x0800
+#define SMBIOS_HANDLE_TYPE9   0x0900
 #define SMBIOS_HANDLE_TYPE11  0x0B00
 #define SMBIOS_HANDLE_TYPE16  0x1000
 #define SMBIOS_HANDLE_TYPE17  0x1100
 #define SMBIOS_HANDLE_TYPE19  0x1300
 #define SMBIOS_HANDLE_TYPE20  0x1400
 #define SMBIOS_HANDLE_TYPE22  0x1600
+#define SMBIOS_HANDLE_TYPE26  0x1A00
+#define SMBIOS_HANDLE_TYPE27  0x1B00
+#define SMBIOS_HANDLE_TYPE28  0x1C00
 #define SMBIOS_HANDLE_TYPE32  0x2000
 #define SMBIOS_HANDLE_TYPE39  0x2700
 #define SMBIOS_HANDLE_TYPE127 0x7f00
@@ -79,6 +85,12 @@ static void *
 smbios_type_4_init(void *start, unsigned int cpu_number,
                    char *cpu_manufacturer);
 static void *
+smbios_type_7_init(void *start);
+static void *
+smbios_type_8_init(void *start);
+static void *
+smbios_type_9_init(void *start);
+static void *
 smbios_type_11_init(void *start);
 static void *
 smbios_type_16_init(void *start, uint32_t memory_size_mb, int nr_mem_devs);
@@ -91,6 +103,12 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, int instance);
 static void *
 smbios_type_22_init(void *start);
 static void *
+smbios_type_26_init(void *start);
+static void *
+smbios_type_27_init(void *start);
+static void *
+smbios_type_28_init(void *start);
+static void *
 smbios_type_32_init(void *start);
 static void *
 smbios_type_39_init(void *start);
@@ -226,6 +244,9 @@ write_smbios_tables(void *ep, void *start,
     do_struct(smbios_type_3_init(p));
     for ( cpu_num = 1; cpu_num <= vcpus; cpu_num++ )
         do_struct(smbios_type_4_init(p, cpu_num, cpu_manufacturer));
+    do_struct(smbios_type_7_init(p));
+    do_struct(smbios_type_8_init(p));
+    do_struct(smbios_type_9_init(p));
     do_struct(smbios_type_11_init(p));
 
     /* Each 'memory device' covers up to 16GB of address space. */
@@ -242,6 +263,9 @@ write_smbios_tables(void *ep, void *start,
     }
 
     do_struct(smbios_type_22_init(p));
+    do_struct(smbios_type_26_init(p));
+    do_struct(smbios_type_27_init(p));
+    do_struct(smbios_type_28_init(p));
     do_struct(smbios_type_32_init(p));
     do_struct(smbios_type_39_init(p));
     do_struct(smbios_type_vendor_oem_init(p));
@@ -713,6 +737,30 @@ smbios_type_4_init(
     return start+1;
 }
 
+/* Type 7 -- Cache Information */
+static void *
+smbios_type_7_init(void *start)
+{
+    return smbios_pt_copy(start, 7, SMBIOS_HANDLE_TYPE7,
+                          sizeof(struct smbios_type_7));
+}
+
+/* Type 8 -- Port Connector Information */
+static void *
+smbios_type_8_init(void *start)
+{
+    return smbios_pt_copy(start, 8, SMBIOS_HANDLE_TYPE8,
+                          sizeof(struct smbios_type_8));
+}
+
+/* Type 9 -- System Slots */
+static void *
+smbios_type_9_init(void *start)
+{
+    return smbios_pt_copy(start, 9, SMBIOS_HANDLE_TYPE9,
+                          sizeof(struct smbios_type_9));
+}
+
 /* Type 11 -- OEM Strings */
 static void *
 smbios_type_11_init(void *start)
@@ -926,6 +974,45 @@ smbios_type_22_init(void *start)
     return start+1;
 }
 
+/* Type 26 -- Voltage Probe */
+static void *
+smbios_type_26_init(void *start)
+{
+    /*
+     * Specification says Type 26 table has length of at least 14h,
+     * which corresponds with "Nominal Value" offset.
+     */
+
+    return smbios_pt_copy(start, 26, SMBIOS_HANDLE_TYPE26,
+                          offsetof(struct smbios_type_26, nominal_value));
+}
+
+/* Type 27 -- Cooling Device */
+static void *
+smbios_type_27_init(void *start)
+{
+    /*
+     * Specification says Type 27 table has length of at least 0Ch,
+     * which corresponds with "Nominal Speed" offset.
+     */
+
+    return smbios_pt_copy(start, 27, SMBIOS_HANDLE_TYPE27,
+                          offsetof(struct smbios_type_27, nominal_speed));
+}
+
+/* Type 28 -- Temperature Probe */
+static void *
+smbios_type_28_init(void *start)
+{
+    /*
+     * Specification says Type 28 table has length of at least 14h,
+     * which corresponds with "Nominal Value" offset.
+     */
+
+    return smbios_pt_copy(start, 28, SMBIOS_HANDLE_TYPE28,
+                          offsetof(struct smbios_type_28, nominal_value));
+}
+
 /* Type 32 -- System Boot Information */
 static void *
 smbios_type_32_init(void *start)
diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h
index 656b2a51ad..1a24ccc55f 100644
--- a/tools/firmware/hvmloader/smbios_types.h
+++ b/tools/firmware/hvmloader/smbios_types.h
@@ -149,6 +149,44 @@ struct smbios_type_4 {
     uint8_t part_number_str;
 } __attribute__ ((packed));
 
+/* SMBIOS type 7 - Cache Information */
+struct smbios_type_7 {
+    struct smbios_structure_header header;
+    uint8_t socket_designation_str;
+    uint16_t cache_configuration;
+    uint16_t maximum_cache_size;
+    uint16_t installed_size;
+    uint16_t supported_SRAM_type;
+    uint16_t current_SRAM_type;
+    uint8_t cache_speed;
+    uint8_t error_connection_type;
+    uint8_t system_cache_type;
+    uint8_t associativity;
+} __attribute__ ((packed));
+
+/* SMBIOS type 8 - Port Connector Information */
+struct smbios_type_8 {
+    struct smbios_structure_header header;
+    uint8_t internal_reference_designator_str;
+    uint8_t internal_connector_type;
+    uint8_t external_reference_designator_str;
+    uint8_t external_connector_type;
+    uint8_t port_type;
+} __attribute__ ((packed));
+
+/* SMBIOS type 9 - System Slots */
+struct smbios_type_9 {
+    struct smbios_structure_header header;
+    uint8_t slot_designation_str;
+    uint8_t slot_type;
+    uint8_t slot_data_bus_width;
+    uint8_t current_usage;
+    uint8_t slot_length;
+    uint16_t slot_id;
+    uint8_t slot_characteristics_1;
+    uint8_t slot_characteristics_2;
+} __attribute__ ((packed));
+
 /* SMBIOS type 11 - OEM Strings */
 struct smbios_type_11 {
     struct smbios_structure_header header;
@@ -232,6 +270,45 @@ struct smbios_type_22 {
     uint32_t oem_specific;
 } __attribute__ ((packed));
 
+/* SMBIOS type 26 - Voltage Probe */
+struct smbios_type_26 {
+    struct smbios_structure_header header;
+    uint8_t description_str;
+    uint8_t location_and_status;
+    uint16_t maximum_value;
+    uint16_t minimum_value;
+    uint16_t resolution;
+    uint16_t tolerance;
+    uint16_t accuracy;
+    uint32_t oem_defined;
+    uint16_t nominal_value;     /* Optional */
+} __attribute__ ((packed));
+
+/* SMBIOS type 27 - Cooling Device */
+struct smbios_type_27 {
+    struct smbios_structure_header header;
+    uint16_t temperature_probe_handle;
+    uint8_t device_type_and_status;
+    uint8_t cooling_unit_group;
+    uint32_t oem_defined;
+    uint16_t nominal_speed;     /* Optional */
+    uint8_t description_str;    /* Optional */
+} __attribute__ ((packed));
+
+/* SMBIOS type 28 - Temperature Probe */
+struct smbios_type_28 {
+    struct smbios_structure_header header;
+    uint8_t description_str;
+    uint8_t location_and_status;
+    uint16_t maximum_value;
+    uint16_t minimum_value;
+    uint16_t resolution;
+    uint16_t tolerance;
+    uint16_t accuracy;
+    uint32_t oem_defined;
+    uint16_t nominal_value;     /* Optional */
+} __attribute__ ((packed));
+
 /* SMBIOS type 32 - System Boot Information */
 struct smbios_type_32 {
     struct smbios_structure_header header;
-- 
2.34.1

Re: [PATCH v2 1/3] hvmloader: clean up SMBIOS code style and formatting

[Jan Beulich]

On 15.07.2025 00:49, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> * Removed trailing whitespaces
> * Removed unnecessary pointer casts
> * Added whitespaces around &&
> * Removed superfluous parentheses
> * Use variables in sizeof() where applicable
> 
> No functional change.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with a few more adjustment, which I may take the liberty to do while
committing:

> @@ -411,12 +411,12 @@ smbios_type_0_init(void *start, const char *xen_version,
>      /* Extended Characteristics: Enable Targeted Content Distribution. */
>      p->characteristics_extension_bytes[1] = 0x04;
>  
> -    p->major_release = (uint8_t) xen_major_version;
> -    p->minor_release = (uint8_t) xen_minor_version;
> +    p->major_release = (uint8_t)xen_major_version;
> +    p->minor_release = (uint8_t)xen_minor_version;

These casts are pointless, too. Since you touch the lines, you could as
well have purged them at the same time.

> @@ -482,21 +482,21 @@ smbios_type_1_init(void *start, const char *xen_version,
>      strcpy((char *)start, s);
>      start += strlen(s) + 1;
>  
> -    uuid_to_string(uuid_str, uuid); 
> +    uuid_to_string(uuid_str, uuid);
>      s = xenstore_read(HVM_XS_SYSTEM_SERIAL_NUMBER, uuid_str);
>      strcpy((char *)start, s);
>      start += strlen(s) + 1;
>  
>      *((uint8_t *)start) = 0;
> -    
> -    return start+1; 
> +
> +    return start+1;

Add the missing blanks here as well while touching the line?

> @@ -920,24 +920,24 @@ smbios_type_22_init(void *start)
>  
>      *((uint8_t *)start) = 0;
>  
> -    return start+1; 
> +    return start+1;

Same here.

Jan

Re: [PATCH v2 2/3] hvmloader: fix SMBIOS table length checks

[Jan Beulich]

On 15.07.2025 00:49, Petr Beneš wrote:
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -47,6 +47,8 @@ static void
>  smbios_pt_init(void);
>  static void*
>  get_smbios_pt_struct(uint8_t type, uint32_t *length_out);
> +static void *
> +smbios_pt_copy(void *start, uint8_t type, uint16_t handle, size_t table_size);

This new helper function isn't mentioned at all in the description. Its
connection to the purpose of the change also is unclear to me. Should
its introduction have been a separate change? And then here only the
length checks be adjusted? (I wouldn't insist on splitting, but the
description at least wants to reflect this addition and in particular
its purpose.)

> @@ -154,6 +156,25 @@ get_smbios_pt_struct(uint8_t type, uint32_t *length_out)
>      return NULL;
>  }
>  
> +static void *
> +smbios_pt_copy(void *start, uint8_t type, uint16_t handle, size_t table_size)
> +{
> +    struct smbios_structure_header *header = start;
> +
> +    void *pts;
> +    uint32_t length;

Nit: Excess blank line in the middle of declarations.

> @@ -381,16 +402,11 @@ smbios_type_0_init(void *start, const char *xen_version,
>      struct smbios_type_0 *p = start;
>      static const char *smbios_release_date = __SMBIOS_DATE__;
>      const char *s;
> -    void *pts;
> -    uint32_t length;
> +    void* next;

Nit: Misplaced *.

> @@ -440,16 +456,11 @@ smbios_type_1_init(void *start, const char *xen_version,
>      char uuid_str[37];
>      struct smbios_type_1 *p = start;
>      const char *s;
> -    void *pts;
> -    uint32_t length;
> +    void* next;

Again.

> @@ -499,25 +510,27 @@ smbios_type_2_init(void *start)
>      struct smbios_type_2 *p = start;
>      const char *s;
>      uint8_t *ptr;
> -    void *pts;
> -    uint32_t length;
> +    void *next;
>      unsigned int counter = 0;
>  
> -    pts = get_smbios_pt_struct(2, &length);
> -    if ( pts != NULL && length > 0 )
> -    {
> -        memcpy(start, pts, length);
> -        p->header.handle = SMBIOS_HANDLE_TYPE2;
> +    /*
> +     * Specification says Type 2 table has length of at least 08h,
> +     * which corresponds with "Asset Tag" field offset.
> +     */

This comment looks to be entirely unrelated to the code which follows.
What is this about? Did you mean to ...

> +    next = smbios_pt_copy(start, 2, SMBIOS_HANDLE_TYPE2, sizeof(*p));

... replace the sizeof() here, using offsetof() instead?

This applies elsewhere as well. Interestingly for type 39 you do use
offsetof(). Actually, for type 0 the descrpition also says "at least",
without that being reflected in the code.

> +    if ( next != start )
> +    {
>          /* Set current chassis handle if present */
> -        if ( p->header.length > 13 )
> +        if ( p->header.length >= offsetof(struct smbios_type_2, board_type) )

Comment and code don't fit together, unless one goes check that board_type
is the field immediately following chassis_handle.

>          {
> -            ptr = ((uint8_t*)start) + 11;            
> +            ptr = ((uint8_t*)start) + offsetof(struct smbios_type_2,
> +                                               chassis_handle);

The cast can also be dropped at the same time.

>              if ( *((uint16_t*)ptr) != 0 )
>                  *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;

Why not switch to p->chassis_handle, without any use of "ptr"? Yet then I
fear I don't really understand what is being done here. Why would an
arbitrary non-zero value be overwritten with a fixed value?

> @@ -946,20 +949,14 @@ smbios_type_32_init(void *start)
>  static void *
>  smbios_type_39_init(void *start)
>  {
> -    struct smbios_type_39 *p = start;
> -    void *pts;
> -    uint32_t length;
> -
> -    pts = get_smbios_pt_struct(39, &length);
> -    if ( pts != NULL && length > 0 )
> -    {
> -        memcpy(start, pts, length);
> -        p->header.handle = SMBIOS_HANDLE_TYPE39;
> -        return start + length;
> -    }
> +    /*
> +     * Specification says Type 39 table has length of at least 10h,
> +     * which corresponds with "Input Voltage Probe Handle" offset.
> +     */
>  
> -    /* Only present when passed in */
> -    return start;
> +    return smbios_pt_copy(start, 39, SMBIOS_HANDLE_TYPE39,
> +                          offsetof(struct smbios_type_39,
> +                                   input_voltage_probe_handle));
>  }

The other comment may want retaining, though.

> --- a/tools/firmware/hvmloader/smbios_types.h
> +++ b/tools/firmware/hvmloader/smbios_types.h
> @@ -252,9 +252,9 @@ struct smbios_type_39 {
>      uint8_t revision_level_str;
>      uint16_t max_capacity;
>      uint16_t characteristics;
> -    uint16_t input_voltage_probe_handle;
> -    uint16_t cooling_device_handle;
> -    uint16_t input_current_probe_handle;
> +    uint16_t input_voltage_probe_handle;    /* Optional */
> +    uint16_t cooling_device_handle;         /* Optional */
> +    uint16_t input_current_probe_handle;    /* Optional */
>  } __attribute__ ((packed));

Why not also mark other optional fields as such?

Jan

Re: [PATCH v2 3/3] hvmloader: add new SMBIOS tables (7, 8, 9, 26, 27, 28)

[Jan Beulich]

On 15.07.2025 00:49, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> Some SMBIOS tables are used by certain malware families to detect virtualized
> environments via WMI queries.
> 
> To improve stealth for sandboxing purposes, this patch adds support
> for populating these SMBIOS tables from an external binary specified
> via the "smbios_firmware" domain config option:
> 
> * 7 - Cache Info
> * 8 - Port Connector
> * 9 - System Slots
> * 26 - Voltage Probe
> * 27 - Cooling Device
> * 28 - Temperature Probe
> 
> If particular table is absent in binary file, then it will not be mapped to
> memory. This method works for Windows domains as tables 7,8,9,26,27,28 are not
> critical for OS boot and runtime. Also if "smbios_firmware" parameter is not
> provided, these tables will be skipped in write_smbios_tables function.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> Signed-off-by: Anton Belousov <blsvntn@outlook.com>

Once again - who's the original author of this patch? Anton, aiui. Hence his S-o-b
wants to be first (chronological order), and he wants to be tagged as the author
of the patch via From:.

The patch itself looks okay to me now. Yet I think we want a SUPPORT.md addition,
clarifying that passing in any such overrides is unsupported. After all a guest
can easily be presented bad information this way.

Jan

Re: [PATCH v2 2/3] hvmloader: fix SMBIOS table length checks

[Petr Beneš]

On Wed, Jul 16, 2025 at 12:27 PM Jan Beulich <jbeulich@suse.com> wrote:
> > +    if ( next != start )
> > +    {
> >          /* Set current chassis handle if present */
> > -        if ( p->header.length > 13 )
> > +        if ( p->header.length >= offsetof(struct smbios_type_2, board_type) )
>
> Comment and code don't fit together, unless one goes check that board_type
> is the field immediately following chassis_handle.

That's the tragedy of using offsetof in this situation. What is mostly
needed throughout this code is "offsetof(x) + sizeof(x)". Instead, I'm
mostly using offsetof(a-field-that-is-following-the-field-that-i-really-meant)
which leads to comments that seemingly don't make sense.

How should I ideally proceed? Should I introduce a new macro?

>
> >              if ( *((uint16_t*)ptr) != 0 )
> >                  *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
>
> Why not switch to p->chassis_handle, without any use of "ptr"? Yet then I
> fear I don't really understand what is being done here.

Right, that would make sense. I left the original code intact.

> Why would an arbitrary non-zero value be overwritten with a fixed value?

That's a question for the original author. FWIW, qemu does not coerce
these values.

But if I had to guess, the original author wanted to make sure that
the SMBIOS data do not reference nonsensical handles.

I would argue that if a user decided to fiddle with these values, they
know what they're doing and I would let them shoot in the foot if they
desire to do so (in other words, I would remove this coercion; but
that's not up to me to decide).

> The other comment may want retaining, though.

Which one do you mean? This one?

> -    /* Only present when passed in */

If so, I should probably add this comment to all the newly introduced
tables as well.



P.

Re: [PATCH v2 2/3] hvmloader: fix SMBIOS table length checks

[Jan Beulich]

On 16.07.2025 21:34, Petr Beneš wrote:
> On Wed, Jul 16, 2025 at 12:27 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> +    if ( next != start )
>>> +    {
>>>          /* Set current chassis handle if present */
>>> -        if ( p->header.length > 13 )
>>> +        if ( p->header.length >= offsetof(struct smbios_type_2, board_type) )
>>
>> Comment and code don't fit together, unless one goes check that board_type
>> is the field immediately following chassis_handle.
> 
> That's the tragedy of using offsetof in this situation. What is mostly
> needed throughout this code is "offsetof(x) + sizeof(x)".

Which is what, in the end, I was alluding to. Sorry if I didn't make that
clear enough.

> Instead, I'm
> mostly using offsetof(a-field-that-is-following-the-field-that-i-really-meant)
> which leads to comments that seemingly don't make sense.
> 
> How should I ideally proceed? Should I introduce a new macro?

Perhaps. Maybe something like endof_field(), along the lines of the
sizeof_field() that we have in the hypervisor.

>>>              if ( *((uint16_t*)ptr) != 0 )
>>>                  *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
>>
>> Why not switch to p->chassis_handle, without any use of "ptr"? Yet then I
>> fear I don't really understand what is being done here.
> 
> Right, that would make sense. I left the original code intact.
> 
>> Why would an arbitrary non-zero value be overwritten with a fixed value?
> 
> That's a question for the original author. FWIW, qemu does not coerce
> these values.
> 
> But if I had to guess, the original author wanted to make sure that
> the SMBIOS data do not reference nonsensical handles.
> 
> I would argue that if a user decided to fiddle with these values, they
> know what they're doing and I would let them shoot in the foot if they
> desire to do so (in other words, I would remove this coercion; but
> that's not up to me to decide).
> 
>> The other comment may want retaining, though.
> 
> Which one do you mean? This one?
> 
>> -    /* Only present when passed in */

Yes.

> If so, I should probably add this comment to all the newly introduced
> tables as well.

I was indeed wondering ...

Jan