[PATCH v4 1/5] efi_loader: store firmware version into FmpState variable

[Ilias Apalodimas]

Kojima-san

On Wed, 29 Mar 2023 at 10:34, Masahisa Kojima
<masahisa.kojima at linaro.org> wrote:
>
> Hi Ilias,
>
> On Tue, 28 Mar 2023 at 22:02, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Kojima-san
> > Apologies for the late review comments but I have some concerns about
> > the design and storing the info as EFI variables.  If the backing
> > storage is a file we can't trust any of that information since anyone
> > can tamper with the file, although the variables are defined as RO.
> >
> > The new struct defines
> > struct fmp_state {
> >        u32 fw_version;
> >        u32 lowest_supported_version;
> >        u32 last_attempt_version;
> >        u32 last_attempt_status;
> > }
> >
> > I think we should change the approach a bit.
> > fw_version -> this should go into the signature.dts we use to store
> > the EFI capsule certificates
> > lowest_supported_version -> same here, put it into the .dts file
>
> OK, I will add these two values into .dts file.

I think you can keep most of the code as-is.  IOW keep the struct and
just fill in the values from the .dts.  I think that would make the
abstraction easier when we try to add similar functionality to A/B
updates.

Regards
/Ilias
>
> > last_attempt_version -> This is essentially the fw_version *after* the
> > upgrade is successful
> > last_attempt_status -> CapsuleXXXX holds that result for us and we
> > already have code to parse it.
>
> To fill ESRT, I think these two values are not very important.
> I will omit these values in my next series.
>
> >
> > This shields further against someone trying to manipulate
> > lowest_supported_version, in case someone uses this for rollback
> > protection checks.
> >
> > Can we modify this patch accordingly?
>
> Thank you for your comment.
> I will revise this series.
>
> Regards,
> Masahisa Kojima
>
> >
> > Regards
> > /Ilias
> >
> >
> >
> >
> > On Tue, 28 Mar 2023 at 14:53, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Kojima-san,
> > >
> > > On Thu, Mar 23, 2023 at 08:09:01PM +0900, Masahisa Kojima wrote:
> > > > Firmware version management is not implemented in the current
> > > > FMP protocol.
> > > > EDK II reference implementation capsule generation script inserts
> > > > the FMP Payload Header right before the payload, it contains the
> > > > firmware version and lowest supported version.
> > > >
> > > > This commit utilizes the FMP Payload Header, reads the header and
> > > > stores the firmware version, lowest supported version,
> > > > last attempt version and last attempt status into "FmpStateXXXX"
> > > > EFI non-volatile variable. XXXX indicates the image index,
> > > > since FMP protocol handles multiple image indexes.
> > > >
> > > > This change is compatible with the existing FMP implementation.
> > > > This change does not mandate the FMP Payload Header.
> > > > If no FMP Payload Header is found in the capsule file, fw_version,
> > > > lowest supported version, last attempt version and last attempt
> > > > status is 0 and this is the same behavior as existing FMP
> > > > implementation.
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > > > ---