Re: [PATCH 1/6] fwu_metadata: make sure structures are packed

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 5/3/23 10:06, Ilias Apalodimas wrote:
> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>
> Hi Rui,
>>
>> The fwu metadata in the metadata partitions
>> should/are packed to guarantee that the info is
>> correct in all platforms. Also the size of them
>> are used to calculate the crc32 and that is important
>> to get it right.
>>
>> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
>> ---
>>   include/fwu_mdata.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/fwu_mdata.h b/include/fwu_mdata.h
>> index 8fda4f4ac225..c61221a91735 100644
>> --- a/include/fwu_mdata.h
>> +++ b/include/fwu_mdata.h
>> @@ -22,7 +22,7 @@ struct fwu_image_bank_info {
>>          efi_guid_t  image_uuid;
>>          uint32_t accepted;
>>          uint32_t reserved;
>> -};
>> +} __packed;

This structure is naturally packed. Why should adding a __packed
attribute make a difference?

>>
>>   /**
>>    * struct fwu_image_entry - information for a particular type of image
>> @@ -38,7 +38,7 @@ struct fwu_image_entry {
>>          efi_guid_t image_type_uuid;
>>          efi_guid_t location_uuid;
>>          struct fwu_image_bank_info img_bank_info[CONFIG_FWU_NUM_BANKS];
>> -};
>> +} __packed;

ditto

>>
>>   /**
>>    * struct fwu_mdata - FWU metadata structure for multi-bank updates
>> @@ -62,6 +62,6 @@ struct fwu_mdata {
>>          uint32_t previous_active_index;
>>
>>          struct fwu_image_entry img_entry[CONFIG_FWU_NUM_IMAGES_PER_BANK];
>> -};
>> +} __packed;

Depending on the alignment of efi_guid_t __packed might make a
difference here. But as efi_guid_t is defined as 4 aligned I can't see
an impact here either.

Best regards

Heinrich

>>
>>   #endif /* _FWU_MDATA_H_ */
>> --
>> 2.40.0
>>
>
> Yep, that's a good catch.  The spec defines 'just' the offsets and not
> the C representation, so those should be indeed packed.
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>