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

[Jassi Brar]

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.

.....
>
> +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?

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.

....
> @@ -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;
               }

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


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