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

[Masahisa Kojima]

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.

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