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

[Jan Beulich <jbeulich@xxxxxxxx>]

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