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

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

[Petr Beneš]

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

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: fix code style violations
  hvmloader: fix SMBIOS table length checks
  hvmloader: add new SMBIOS tables (7,8,9,26,27,28)

 tools/firmware/hvmloader/smbios.c       | 204 ++++++++++++++++++++----
 tools/firmware/hvmloader/smbios_types.h |  83 +++++++++-
 2 files changed, 254 insertions(+), 33 deletions(-)

-- 
2.34.1

[PATCH 1/3] hvmloader: fix code style violations

[Petr Beneš]

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

Preparatory commit. No functional change.

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

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 97a054e9e3..fc3cdc9a25 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -378,18 +378,18 @@ 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));
@@ -438,17 +438,17 @@ 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));
@@ -496,7 +496,7 @@ smbios_type_1_init(void *start, const char *xen_version,
 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,7 +517,7 @@ smbios_type_2_init(void *start)
                 *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
         }
 
-        return (start + length);
+        return start + length;
     }
 
     memset(p, 0, sizeof(*p));
@@ -591,18 +591,18 @@ 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));
@@ -653,7 +653,7 @@ 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;
+    struct smbios_type_4 *p = start;
     uint32_t eax, ebx, ecx, edx;
 
     memset(p, 0, sizeof(*p));
@@ -704,7 +704,7 @@ smbios_type_4_init(
 static void *
 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,11 +712,11 @@ 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;
@@ -754,7 +754,7 @@ 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));
 
@@ -779,7 +779,7 @@ 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));
 
@@ -814,7 +814,7 @@ 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));
 
@@ -836,7 +836,7 @@ 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));
 
@@ -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");
@@ -927,7 +927,7 @@ smbios_type_22_init(void *start)
 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));
 
@@ -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,7 +998,7 @@ 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));
 
-- 
2.34.1

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

hvmloader currently implements version 2.4

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

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index fc3cdc9a25..2cd826768b 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -385,7 +385,7 @@ smbios_type_0_init(void *start, const char *xen_version,
     uint32_t length;
 
     pts = get_smbios_pt_struct(0, &length);
-    if ( pts != NULL && length > 0 )
+    if ( pts != NULL && length >= sizeof(struct smbios_type_0) )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE0;
@@ -444,7 +444,7 @@ smbios_type_1_init(void *start, const char *xen_version,
     uint32_t length;
 
     pts = get_smbios_pt_struct(1, &length);
-    if ( pts != NULL && length > 0 )
+    if ( pts != NULL && length >= sizeof(struct smbios_type_1) )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE1;
@@ -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 >= 8 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE2;
@@ -598,7 +598,7 @@ smbios_type_3_init(void *start)
     uint32_t counter = 0;
 
     pts = get_smbios_pt_struct(3, &length);
-    if ( pts != NULL && length > 0 )
+    if ( pts != NULL && length >= 13 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE3;
@@ -712,7 +712,7 @@ 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 >= sizeof(struct smbios_type_11) )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE11;
@@ -869,7 +869,7 @@ smbios_type_22_init(void *start)
     uint32_t length;
 
     pts = get_smbios_pt_struct(22, &length);
-    if ( pts != NULL && length > 0 )
+    if ( pts != NULL && length >= sizeof(struct smbios_type_22) )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE22;
@@ -951,7 +951,7 @@ smbios_type_39_init(void *start)
     uint32_t length;
 
     pts = get_smbios_pt_struct(39, &length);
-    if ( pts != NULL && length > 0 )
+    if ( pts != NULL && length >= 16 )
     {
         memcpy(start, pts, length);
         p->header.handle = SMBIOS_HANDLE_TYPE39;
-- 
2.34.1

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

[Petr Beneš]

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

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.

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

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 2cd826768b..2a799fdac7 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
@@ -77,6 +83,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);
@@ -89,6 +101,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);
@@ -205,6 +223,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. */
@@ -221,6 +242,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));
@@ -700,6 +724,66 @@ smbios_type_4_init(
     return start+1;
 }
 
+/* Type 7 -- Cache Information */
+static void *
+smbios_type_7_init(void *start)
+{
+    struct smbios_type_7 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(7, &length);
+    if ( pts != NULL && length >= sizeof(struct smbios_type_7) )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE7;
+        return start + length;
+    }
+
+    return start;
+}
+
+/* Type 8 -- Port Connector Information */
+static void *
+smbios_type_8_init(void *start)
+{
+    struct smbios_type_8 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(8, &length);
+    if ( pts != NULL && length >= sizeof(struct smbios_type_8) )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE8;
+        return start + length;
+    }
+
+    return start;
+}
+
+/* Type 9 -- System Slots */
+static void *
+smbios_type_9_init(void *start)
+{
+    struct smbios_type_9 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(9, &length);
+    if ( pts != NULL && length >= sizeof(struct smbios_type_9) )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE9;
+        return start + length;
+    }
+
+    return start;
+}
+
 /* Type 11 -- OEM Strings */
 static void *
 smbios_type_11_init(void *start) 
@@ -780,7 +864,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
 {
     char buf[16];
     struct smbios_type_17 *p = start;
-    
+
     memset(p, 0, sizeof(*p));
 
     p->header.type = 17;
@@ -815,7 +899,7 @@ static void *
 smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
 {
     struct smbios_type_19 *p = start;
-    
+
     memset(p, 0, sizeof(*p));
 
     p->header.type = 19;
@@ -923,6 +1007,66 @@ smbios_type_22_init(void *start)
     return start+1; 
 }
 
+/* Type 26 -- Voltage Probe */
+static void *
+smbios_type_26_init(void *start)
+{
+    struct smbios_type_26 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(26, &length);
+    if ( pts != NULL && length >= 20 )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE26;
+        return start + length;
+    }
+
+    return start;
+}
+
+/* Type 27 -- Cooling Device */
+static void *
+smbios_type_27_init(void *start)
+{
+    struct smbios_type_27 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(27, &length);
+    if ( pts != NULL && length >= 12 )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE27;
+        return start + length;
+    }
+
+    return start;
+}
+
+/* Type 28 -- Temperature Probe */
+static void *
+smbios_type_28_init(void *start)
+{
+    struct smbios_type_28 *p = start;
+
+    void *pts;
+    uint32_t length;
+
+    pts = get_smbios_pt_struct(28, &length);
+    if ( pts != NULL && length >= 20 )
+    {
+        memcpy(start, pts, length);
+        p->header.handle = SMBIOS_HANDLE_TYPE28;
+        return start + length;
+    }
+
+    return start;
+}
+
 /* 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 7c648ece71..3e5c5fa46a 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;
@@ -252,9 +329,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

Re: [PATCH 1/3] hvmloader: fix code style violations

[Frediano Ziglio]

On Wed, Jul 2, 2025 at 12:45 AM Petr Beneš <w1benny@gmail.com> wrote:
>
> From: Petr Beneš <w1benny@gmail.com>
>
> Preparatory commit. No functional change.
>

Can you describe which specific code styles this commit is supposed to fix?

> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> ---
>  tools/firmware/hvmloader/smbios.c | 56 +++++++++++++++----------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
> index 97a054e9e3..fc3cdc9a25 100644
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -378,18 +378,18 @@ 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));
> @@ -438,17 +438,17 @@ 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));
> @@ -496,7 +496,7 @@ smbios_type_1_init(void *start, const char *xen_version,
>  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,7 +517,7 @@ smbios_type_2_init(void *start)
>                  *((uint16_t*)ptr) = SMBIOS_HANDLE_TYPE3;
>          }
>
> -        return (start + length);
> +        return start + length;
>      }
>
>      memset(p, 0, sizeof(*p));
> @@ -591,18 +591,18 @@ 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));
> @@ -653,7 +653,7 @@ 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;
> +    struct smbios_type_4 *p = start;
>      uint32_t eax, ebx, ecx, edx;
>
>      memset(p, 0, sizeof(*p));
> @@ -704,7 +704,7 @@ smbios_type_4_init(
>  static void *
>  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,11 +712,11 @@ 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;
> @@ -754,7 +754,7 @@ 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));
>
> @@ -779,7 +779,7 @@ 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));
>
> @@ -814,7 +814,7 @@ 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));
>
> @@ -836,7 +836,7 @@ 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));
>
> @@ -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");
> @@ -927,7 +927,7 @@ smbios_type_22_init(void *start)
>  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));
>
> @@ -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,7 +998,7 @@ 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));
>

Frediano

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

[Jan Beulich]

On 02.07.2025 01:45, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> 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

In which case shouldn't this submission have a version number, explicitly
larger than 1?

Jan

> 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: fix code style violations
>   hvmloader: fix SMBIOS table length checks
>   hvmloader: add new SMBIOS tables (7,8,9,26,27,28)
> 
>  tools/firmware/hvmloader/smbios.c       | 204 ++++++++++++++++++++----
>  tools/firmware/hvmloader/smbios_types.h |  83 +++++++++-
>  2 files changed, 254 insertions(+), 33 deletions(-)
> 

Re: [PATCH 1/3] hvmloader: fix code style violations

[Jan Beulich]

On 02.07.2025 01:45, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
> 
> Preparatory commit. No functional change.
> 
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

There are style violations that you deal with here, yes. But you go beyond
that. At which point I agree with Frediano that you want to call out at
least what you do beyond bringing code (more?) in line with ./CODING_STYLE.
The subject may also want to reflect that's it's not just violations you
sort.

Jan

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

[Jan Beulich]

On 02.07.2025 01:45, Petr Beneš wrote:
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -385,7 +385,7 @@ smbios_type_0_init(void *start, const char *xen_version,
>      uint32_t length;
>  
>      pts = get_smbios_pt_struct(0, &length);
> -    if ( pts != NULL && length > 0 )
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_0) )

Please preferably use an actual variable that's available, i.e. sizeof(*p) in
cases like this one.

Also, patch 1 touched this very line. Adjusting style while touching a line
is quite fine, and typically even preferred over touching the same line twice
in close succession.

> @@ -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 >= 8 )

No way to have (even uncommented) literal numbers like this in the code.
This can be expressed using offsetof(), I expect, and hence wants expressing
that way. Seeing the respective member's name will then also aid reviewing.

Jan

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

[Jan Beulich]

On 02.07.2025 01:45, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>

This isn't in line with the first S-o-b, nor with the fact that in the cover
letter you say this was previously submitted (and hence authored?) by Anton.

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

Who or what says these are "necessary"? Also, to have at least a basic
understanding without consulting the spec (and without reading the patch in
its entirety), mentioning what the numbers actually stand for would be helpful,
I think. This way the connection to "necessary" or maybe merely "desirable"
would likely also be easier to make.

> @@ -700,6 +724,66 @@ smbios_type_4_init(
>      return start+1;
>  }
>  
> +/* Type 7 -- Cache Information */
> +static void *
> +smbios_type_7_init(void *start)
> +{
> +    struct smbios_type_7 *p = start;
> +

Nit: What use is this blank line?

> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(7, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_7) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE7;
> +        return start + length;
> +    }
> +
> +    return start;
> +}
> +
> +/* Type 8 -- Port Connector Information */
> +static void *
> +smbios_type_8_init(void *start)
> +{
> +    struct smbios_type_8 *p = start;
> +
> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(8, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_8) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE8;
> +        return start + length;
> +    }
> +
> +    return start;
> +}
> +
> +/* Type 9 -- System Slots */
> +static void *
> +smbios_type_9_init(void *start)
> +{
> +    struct smbios_type_9 *p = start;
> +
> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(9, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_9) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE9;
> +        return start + length;
> +    }
> +
> +    return start;
> +}

These all look largely identical with one another, and they also look to
match smbios_type_39_init(). Surely we could do with less redundancy by
having some common helper dealing with "Only present when passed in" cases,
as smbios_type_39_init() states it in a comment.

> @@ -780,7 +864,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, int instance)
>  {
>      char buf[16];
>      struct smbios_type_17 *p = start;
> -    
> +
>      memset(p, 0, sizeof(*p));
>  
>      p->header.type = 17;
> @@ -815,7 +899,7 @@ static void *
>  smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
>  {
>      struct smbios_type_19 *p = start;
> -    
> +
>      memset(p, 0, sizeof(*p));
>  
>      p->header.type = 19;

These two hunks look like they belong in patch 1. They're entirely unrelated
here (i.e. not even adjacent to code being touched).

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

Nit: We don't use C++ style comments in Xen.

> @@ -252,9 +329,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));

This again looks like an unrelated change. I don't mind it living here, but
such wants mentioning in the description. Then again this may better be done
in patch 2, to accompany the size checks (where it matters from what offset
onwards fields are optional).

Jan

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

[Petr Beneš]

On Wed, Jul 2, 2025 at 8:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.07.2025 01:45, Petr Beneš wrote:
> > From: Petr Beneš <w1benny@gmail.com>
> >
> > 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
>
> In which case shouldn't this submission have a version number, explicitly
> larger than 1?

Fair. I wasn't sure, since I'm the one who's posting the patch now.
What version number should I use next? Should I go with 2 or use
something else?

P.

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

[Petr Beneš]

On Wed, Jul 2, 2025 at 9:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 02.07.2025 01:45, Petr Beneš wrote:
> > From: Petr Beneš <w1benny@gmail.com>
>
> This isn't in line with the first S-o-b, nor with the fact that in the cover
> letter you say this was previously submitted (and hence authored?) by Anton.

Can you please point me to the right direction? I have no idea what
tags should I specify here.

P.

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

[Jan Beulich]

On 04.07.2025 23:15, Petr Beneš wrote:
> On Wed, Jul 2, 2025 at 8:47 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.07.2025 01:45, Petr Beneš wrote:
>>> From: Petr Beneš <w1benny@gmail.com>
>>>
>>> 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
>>
>> In which case shouldn't this submission have a version number, explicitly
>> larger than 1?
> 
> Fair. I wasn't sure, since I'm the one who's posting the patch now.
> What version number should I use next? Should I go with 2 or use
> something else?

Aiui this is effectively v2, so the next submission imo would want to be v3.

Jan

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

[Jan Beulich]

On 05.07.2025 01:48, Petr Beneš wrote:
> On Wed, Jul 2, 2025 at 9:15 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 02.07.2025 01:45, Petr Beneš wrote:
>>> From: Petr Beneš <w1benny@gmail.com>
>>
>> This isn't in line with the first S-o-b, nor with the fact that in the cover
>> letter you say this was previously submitted (and hence authored?) by Anton.
> 
> Can you please point me to the right direction? I have no idea what
> tags should I specify here.

Well, in the common case the original author would never change, and it would
be their S-o-b that remains first forever. Anything else would need explaining.

Jan

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

[Petr Beneš]

On Mon, Jul 7, 2025 at 8:56 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Well, in the common case the original author would never change, and it would
> be their S-o-b that remains first forever. Anything else would need explaining.

So, I should swap the two S-o-b lines, gotcha.

P.