[PATCH v10 10/15] FWU: Add support for the FWU Multi Bank Update feature

[Sughosh Ganu]

On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>
> On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> ....
> > +
> > +static __maybe_unused void fwu_post_update_checks(
> > +       struct efi_capsule_header *capsule,
> > +       bool *fw_accept_os, bool *capsule_update)
> > +{
> > +       if (fwu_empty_capsule(capsule))
> > +               *capsule_update = false;
> > +       else
> > +               if (!*fw_accept_os)
> > +                       *fw_accept_os =
> > +                               capsule->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> >
> True and False, instead of 1 and 0 for consistency.

Okay

>
> .....
> >
> > +static __maybe_unused efi_status_t fwu_post_update_process(bool fw_accept_os)
> > +{
> > +       int status;
> > +       u32 update_index;
> > +       efi_status_t ret;
> > +
> > +       status = fwu_plat_get_update_index(&update_index);
> > +       if (status < 0) {
> > +               log_err("Failed to get the FWU update_index value\n");
> > +               return EFI_DEVICE_ERROR;
> > +       }
> > +
> > +       /*
> > +        * All the capsules have been updated successfully,
> > +        * update the FWU metadata.
> > +        */
> > +       log_debug("Update Complete. Now updating active_index to %u\n",
> > +                 update_index);
> > +       status = fwu_update_active_index(update_index);
> >
> Do we want to check if all images in the bank are updated via capsules
> before switching the bank?

This function does get called only when the update status for every
capsule is a success. Even if one of the capsules does not get
updated, the active index will not get updated.

>
> A developer will make sure all images are provided in one go, so that
> the switch is successful.
> But a malicious user may force some old vulnerable image back into use
> by updating all but that image.

That I believe is to be handled through a combination of implementing
a rollback protection mechanism, along with capsule authentication.
These are separate to the implementation of the multi bank updates
that these patches are aiming for.

>
> ....
> > @@ -410,7 +544,35 @@ static efi_status_t efi_capsule_update_firmware(
> >         int item;
> >         struct efi_firmware_management_protocol *fmp;
> >         u16 *abort_reason;
> > +       efi_guid_t image_type_id;
> >         efi_status_t ret = EFI_SUCCESS;
> > +       int status;
> > +       u8 image_index;
> > +       u32 update_index;
> > +       bool fw_accept_os, image_index_check;
> > +
> > +       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>
>
> > +               if (!fwu_empty_capsule(capsule_data) &&
> > +                   !fwu_update_checks_pass()) {
> > +                       log_err("FWU checks failed. Cannot start update\n");
> > +                       return EFI_INVALID_PARAMETER;
> > +               }
> > +
> > +               if (fwu_empty_capsule(capsule_data))
> > +                       return fwu_empty_capsule_process(capsule_data);
> > +
> This could be simplified as
>                if (fwu_empty_capsule(capsule_data))
>                        return fwu_empty_capsule_process(capsule_data);
>
>                 if (!fwu_update_checks_pass()) {
>                        log_err("FWU checks failed. Cannot start update\n");
>                        return EFI_INVALID_PARAMETER;
>                }

Yes, will change.

>
> ....
> > @@ -1151,7 +1374,15 @@ efi_status_t efi_launch_capsules(void)
> >                         log_err("Deleting capsule %ls failed\n",
> >                                 files[i]);
> >         }
> > +
> >         efi_capsule_scan_done();
> > +       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> >
> missing newline before the if

Will add.

-sughosh

>
>
> > +               if (update_status == true && capsule_update == true) {
> > +                       ret = fwu_post_update_process(fw_accept_os);
> > +               } else if (capsule_update == true && update_status == false) {
> > +                       log_err("All capsules were not updated. Not updating FWU metadata\n");
> > +               }
> nit:  maybe keep the order of capsule_update and update_status same in
> both clauses.
>
> cheers.