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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2025 21:34, Petr Beneš wrote:
> 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)".

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