[PATCH v2 2/2] efi_loader: support fmp versioning for multi bank update

[Masahisa Kojima]

On Wed, 10 Jan 2024 at 18:10, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 10 Jan 2024 at 02:53, Masahisa Kojima
> <masahisa.kojima at linaro.org> wrote:
> >
> > On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima <masahisa.kojima at linaro.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas
> > > > <ilias.apalodimas at linaro.org> wrote:
> > > > >
> > > > > Kojima-san
> > > > >
> > > > > On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
> > > > > > This commit stores the firmware version into the array
> > > > > > of fmp_state structure to support the fmp versioning
> > > > > > for multi bank update. The index of the array is identified
> > > > > > by the bank index.
> > > > >
> > > > > Why do we all of them? Can't we just always store/use the version of the active
> > > > > bank?
> > > >
> > > > Sorry for the late reply.
> > > > When the capsule update is reverted, the active bank is back to the
> > > > previous active bank.
> > > > So I think versions for all banks should be stored.
> > >
> > > But even in that case, the active bank should point to the correct
> > > version when the ESRT tables are generated. Wouldn't it be simpler to
> > > just set the FmpState variable accordingly when that happens?
> >
> > The fw_version in FmpState variable comes from UEFI Capsule Header and
> > FmpState variable is set when the UEFI capsule update is performed.
> > If we only store the fw_version of the active bank, the fw_version of
> > the previous active bank is overwritten. When the capsule is reverted,
> > we can not get the fw_version of the previous active bank.
> >
> > Thanks,
> > Masahisa Kojima
>
> Ah yes, that makes sense.  In that case, the patch looks fine, apart
> from the return value of the getvariable call which is never checked
>
>
> > > > > >
> > > > > >       image_type_id = efi_firmware_get_image_type_id(image_index);
> > > > > >       if (!image_type_id)
> > > > > > @@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
> > > > > >       efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > > > >                               image_index);
> > > > > >
> > > > > > +     if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> > > > > > +             ret = fwu_plat_get_update_index(&update_bank);
> > > > > > +             if (ret)
> > > > > > +                     return EFI_INVALID_PARAMETER;
> > > > > > +
> > > > > > +             num_banks = CONFIG_FWU_NUM_BANKS;
> > > > > > +     }
> > > > > > +
> > > > > > +     size = num_banks * sizeof(*var_state);
> > > > > > +     var_state = calloc(1, size);
> > > > > > +     if (!var_state)
> > > > > > +             return EFI_OUT_OF_RESOURCES;
> > > > > > +
> > > > > > +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> > > > > > +                                var_state, NULL);
> > > > > > +
>
> We should be checking the return value here.

I will fix it.

Thanks,
Masahisa Kojima

>
> > > > > >       /*
> > > > > >        * Only the fw_version is set here.
> > > > > >        * lowest_supported_version in FmpState variable is ignored since
> > > > > >        * it can be tampered if the file based EFI variable storage is used.
> > > > > >        */
> > > > > > -     var_state.fw_version = state->fw_version;
> > > > > > +     var_state[update_bank].fw_version = state->fw_version;
> > > > > >
> > > > > > +     size = num_banks * sizeof(*var_state);
> > > > > >       ret = efi_set_variable_int(varname, image_type_id,
> > > > > >                                  EFI_VARIABLE_READ_ONLY |
> > > > > >                                  EFI_VARIABLE_NON_VOLATILE |
> > > > > >                                  EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > > > >                                  EFI_VARIABLE_RUNTIME_ACCESS,
> > > > > > -                                sizeof(var_state), &var_state, false);
> > > > > > +                                size, var_state, false);
> > > > > > +
> > > > > > +     free(var_state);
> > > > > >
> > > > > >       return ret;
> > > > > >  }
> > > > > > --
> > > > > > 2.34.1
> > > > > >
>
> Thanks
> /Ilias