On Mon, 26 Sept 2022 at 08:25, Jassi Brar jassisinghbrar@gmail.com wrote:
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;elseif (!*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.