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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2025 09:03, Jan Beulich wrote:
> On 30.07.2025 11:56, Petr Beneš wrote:
>> --- a/tools/firmware/hvmloader/smbios_types.h
>> +++ b/tools/firmware/hvmloader/smbios_types.h
>> @@ -90,13 +90,13 @@ struct smbios_type_2 {
>>      uint8_t product_name_str;
>>      uint8_t version_str;
>>      uint8_t serial_number_str;
>> -    uint8_t asset_tag_str;
>> -    uint8_t feature_flags;
>> -    uint8_t location_in_chassis_str;
>> -    uint16_t chassis_handle;
>> -    uint8_t board_type;
>> -    uint8_t contained_handle_count;
>> -    uint16_t contained_handles[];
>> +    uint8_t asset_tag_str;                  /* Optional */
>> +    uint8_t feature_flags;                  /* Optional */
>> +    uint8_t location_in_chassis_str;        /* Optional */
>> +    uint16_t chassis_handle;                /* Optional */
>> +    uint8_t board_type;                     /* Optional */
>> +    uint8_t contained_handle_count;         /* Optional */
>> +    uint16_t contained_handles[];           /* Optional */
>>  } __attribute__ ((packed));
>>  
>>  /* System Enclosure - Contained Elements */
>> @@ -118,12 +118,12 @@ struct smbios_type_3 {
>>      uint8_t power_supply_state;
>>      uint8_t thermal_state;
>>      uint8_t security_status;
>> -    uint32_t oem_specific;
>> -    uint8_t height;
>> -    uint8_t number_of_power_cords;
>> -    uint8_t contained_element_count;
>> -    uint8_t contained_element_length;
>> -    struct smbios_contained_element contained_elements[];
>> +    uint32_t oem_specific;                  /* Optional */
>> +    uint8_t height;                         /* Optional */
>> +    uint8_t number_of_power_cords;          /* Optional */
>> +    uint8_t contained_element_count;        /* Optional */
>> +    uint8_t contained_element_length;       /* Optional */
>> +    struct smbios_contained_element contained_elements[]; /* Optional */
>>  } __attribute__ ((packed));
>>  
>>  /* SMBIOS type 4 - Processor Information */
>> @@ -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));
>>  
>>  /* SMBIOS type 127 -- End-of-table */
> 
> I regret now that I didn't look more closely before offering my ack. May I
> ask where these "Optional" (and also the ones in patch 3) are taken from?
> While looking at Teddy's patch to bump the version to 2.6 I considered it
> necessary to check whether other adjustments (fields becoming non-optional
> for example) would be required, and to my surprise and couldn't find proof
> of any of the above being optional in 2.4.

I take this back. This is written down in the spec, just not as clearly and
uniformly as one might wish.

Jan