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

[Ilias Apalodimas]

Hi,

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

> 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.

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 at linaro.org>