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

On Thu, Sep 15, 2022 at 3:17 AM Sughosh Ganu sughosh.ganu@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.