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

[Rui Miguel Silva <rui.silva@linaro.org>]

Ilias Apalodimas <ilias.apalodimas@linaro.org> writes:

> Hi,
>
> On Mon, 29 May 2023 at 16:12, Rui Miguel Silva <rui.silva@linaro.org> wrote:
>>
>> Hi Heinrich,
>> thanks for the review,
>> Heinrich Schuchardt <xypron.glpk@gmx.de> writes:
>>
>> > 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?
>>
>> 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?

>
> No, we have it as 4byte aligned,

Sorry, was looking at an relative old branch. (only with this:
1dd705cf99 efi: use 32-bit alignment for efi_guid_t in v2023.04 it
changed to 4 aligned)

and, please note, that there is a definition of efi_guid_t
as aligned 8 in eficapsule tool:
https://elixir.bootlin.com/u-boot/latest/source/tools/eficapsule.h#L27

>
>> 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.
>
> I think we should overall define these as packed regardless.  The spec
> might change at some point and add a few fields ane even though it
> doesn't explicitly requires packing it only mentions offsets.  This is
> going to make a difference in code generation, but it's in a path
> noone cares about speed.

Agree. Thanks for the feedback.

Cheers,
  Rui
  
>
> Thanks
> /Ilias
>
>>
>> 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.apalodimas@linaro.org>