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

[Petr Beneš <w1benny@xxxxxxxxx>]

On Wed, Jul 16, 2025 at 12:27 PM Jan Beulich <jbeulich@xxxxxxxx> 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.