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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2025 11:56, 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);
>  static void
>  get_cpu_manufacturer(char *buf, int len);
>  static int
> @@ -154,6 +156,24 @@ 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 )

With this, the function parameter may better be named "min_size" or "req_size".
(I was first irritated by ...

> @@ -381,16 +401,17 @@ 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;
> -    }
> +    /*
> +     * Specification says Type 0 table has length of at least 18h for 
> v2.4-3.0.
> +     */
> +
> +    BUILD_BUG_ON(sizeof(*p) != 24);

... there being != here, despite the comment saying "at least". And the
check here is ...

> +    next = smbios_pt_copy(start, 0, SMBIOS_HANDLE_TYPE0, sizeof(*p));

... for the sizeof() use here, aiui.)

> @@ -498,26 +517,30 @@ 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 the end of the "Serial Number" field.
> +     */
> +
> +    BUILD_BUG_ON(offsetof_end(struct smbios_type_2, serial_number_str) != 8);
>  
> +    next = smbios_pt_copy(start, 2, SMBIOS_HANDLE_TYPE2,
> +                          offsetof_end(struct smbios_type_3,

Was this meant to be smbios_type_2?

With the adjustments
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
The adjustments also look to be isolated enough to carry out while committing.
Provided of course that you agree with making them.

Jan