Hi Heinrich,
thanks for the review,
Heinrich Schuchardt <xypron.g...@gmx.de> writes:
> On 5/3/23 10:06, Ilias Apalodimas wrote:
>> On Tue, 2 May 2023 at 16:12, Rui Miguel Silva <rui.si...@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.si...@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?
Agree.
>
>>>
>>> /**
>>> * 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
Agree.
>
>>>
>>> /**
>>> * 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.
but efi_guid_t is defined as 8 aligned, right?
Even though, I think that this type of data written to disk/flash
without guaranteeing (trusting natural picketing) packed and endianness
is never a good idea.
Cheers,
Rui
>
> 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.apalodi...@linaro.org>
