On Fri, 14 Oct 2022 at 13:29, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
+}
+static __maybe_unused efi_status_t fwu_empty_capsule_process(
struct efi_capsule_header *capsule)+{
int status;u32 active_idx;efi_status_t ret;efi_guid_t *image_guid;if (!guidcmp(&capsule->capsule_guid,&fwu_guid_os_request_fw_revert)) {/** One of the previously updated image has* failed the OS acceptance test. OS has* requested to revert back to the earlier* boot index*/status = fwu_revert_boot_index();ret = fwu_to_efi_error(status);if (ret == EFI_SUCCESS)log_info("Reverted the FWU active_index. Recommend rebooting the system\n");elselog_err("Failed to revert the FWU boot index\n");} else {We should be explicit on the GUID checking here. IOW if someone hands you over and empty capsule with a guid !fwu_guid_os_request_fw_revert you'll accept the capsule
This won't happen since this function is called only for empty capsules. Will handle the other two review comments.
I am not sure I am following. Why can't it happen? If someone create an empty capsule with an invalid (by invalid I mean none of the 2 GUIDs the spec expects), you'll end up trying to accept the firmware no?
That will be checked in the fwu_empty_capsule(). Only if this function returns true, will the fwu_empty_capsule_process() get called. So the capsule_guid should either be one for accept or a revert capsule.
-sughosh
Right in this case then why don't we pass an extra argument to fwu_empty_capsule_process() instead and skip the extra guidcmp?
Yes, I can do that, but in any case I need to pass the capsule header for getting the image_guid for the accept capsule. Hence the check in the function. Moreover, if this is to be passed as an argument, it will have to be determined first, whether it is an accept or a revert capsule, and that will have to be done in fwu_empty_capsule(), which is basically being used only for identification of the empty capsule, since it is also called in fwu_post_update_checks(). If you have a strong opinion on this, I will change the fwu_empty_capsule() implementation.
-sughosh
Thanks /Ilias
Thanks /Ilias
-sughosh
/** Image accepted by the OS. Set the acceptance* status for the image.*/image_guid = (void *)(char *)capsule +capsule->header_size;status = fwu_get_active_index(&active_idx);ret = fwu_to_efi_error(status);if (ret != EFI_SUCCESS) {log_err("Unable to get the active_index from the FWU metadata\n");return ret;}status = fwu_accept_image(image_guid, active_idx);ret = fwu_to_efi_error(status);if (ret != EFI_SUCCESS)log_err("Unable to set the Accept bit for the image %pUs\n",image_guid);}return ret;+}
+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)This line should fold to the upper one
*fw_accept_os =capsule->flags & FW_ACCEPT_OS ? true : false;+}
+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_set_active_index(update_index);ret = fwu_to_efi_error(status);if (ret != EFI_SUCCESS) {log_err("Failed to update FWU metadata index values\n");} else {log_debug("Successfully updated the active_index\n");[...]
Thanks /Ilias