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
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
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.
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
© 2016 - 2025 Red Hat, Inc.