On Mon, 26 Sept 2022 at 08:25, Jassi Brar <jassisinghb...@gmail.com> wrote: > > On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu <sughosh.g...@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.