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

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