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