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

[Ilias Apalodimas]

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?
Am I missing anything?

Thanks
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> >
> > >
> > > This modification keeps the backward compatibility with
> > > the existing versioning feature.
> > >
> >
> > Thanks
> > /Ilias
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > > ---
> > >  lib/efi_loader/efi_firmware.c | 69 +++++++++++++++++++++++++++--------
> > >  1 file changed, 54 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > index 9b8630625f..5459f3d38d 100644
> > > --- a/lib/efi_loader/efi_firmware.c
> > > +++ b/lib/efi_loader/efi_firmware.c
> > > @@ -207,18 +207,10 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_
> > >  {
> > >       u16 varname[13]; /* u"FmpStateXXXX" */
> > >       efi_status_t ret;
> > > -     efi_uintn_t size;
> > > -     struct fmp_state var_state = { 0 };
> > > -
> > > -     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > -                             fw_array->image_index);
> > > -     size = sizeof(var_state);
> > > -     ret = efi_get_variable_int(varname, &fw_array->image_type_id,
> > > -                                NULL, &size, &var_state, NULL);
> > > -     if (ret == EFI_SUCCESS)
> > > -             image_info->version = var_state.fw_version;
> > > -     else
> > > -             image_info->version = 0;
> > > +     efi_uintn_t size, expected_size;
> > > +     uint num_banks = 1;
> > > +     uint active_index = 0;
> > > +     struct fmp_state *var_state;
> > >
> > >       efi_firmware_get_lsv_from_dtb(fw_array->image_index,
> > >                                     &fw_array->image_type_id,
> > > @@ -227,6 +219,31 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_
> > >       image_info->version_name = NULL; /* not supported */
> > >       image_info->last_attempt_version = 0;
> > >       image_info->last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > > +     image_info->version = 0;
> > > +
> > > +     /* get the fw_version */
> > > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > +                             fw_array->image_index);
> > > +     if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> > > +             ret = fwu_get_active_index(&active_index);
> > > +             if (ret)
> > > +                     return;
> > > +
> > > +             num_banks = CONFIG_FWU_NUM_BANKS;
> > > +     }
> > > +
> > > +     size = num_banks * sizeof(*var_state);
> > > +     expected_size = size;
> > > +     var_state = calloc(1, size);
> > > +     if (!var_state)
> > > +             return;
> > > +
> > > +     ret = efi_get_variable_int(varname, &fw_array->image_type_id,
> > > +                                NULL, &size, var_state, NULL);
> > > +     if (ret == EFI_SUCCESS && expected_size == size)
> > > +             image_info->version = var_state[active_index].fw_version;
> > > +
> > > +     free(var_state);
> > >  }
> > >
> > >  /**
> > > @@ -362,8 +379,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
> > >  {
> > >       u16 varname[13]; /* u"FmpStateXXXX" */
> > >       efi_status_t ret;
> > > +     uint num_banks = 1;
> > > +     uint update_bank = 0;
> > > +     efi_uintn_t size;
> > >       efi_guid_t *image_type_id;
> > > -     struct fmp_state var_state = { 0 };
> > > +     struct fmp_state *var_state;
> > >
> > >       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);
> > > +
> > >       /*
> > >        * 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
> > >