Ilias Apalodimas <ilias.apalodi...@linaro.org> writes:
> Hi Jonathan,
>
> The patch looks correct but there's a few more things to address.
>
> On Thu, Feb 13, 2025 at 01:53:50PM -0600, Jonathan Humphreys wrote:
>> The current implementation of EFI capsule update uses set_dfu_alt_info() to
>> set the dfu_alt_info environment variable with the settings it requires.
>> However, set_dfu_alt_info() is doing this for all DFU operations, even
>> those unrelated to capsule update.
>>
>> Thus other uses of DFU, such as DFU boot which sets its own value for the
>> dfu_alt_info environment variable, will have that setting overwritten with
>> the capsule update setting. Similarly, any user defined value for the
>> dfu_alt_info environment variable would get overwritten when any DFU
>> operation was performed, including simply performing a "dfu 0 list"
>> command.
>>
>> The solution is stop using the set_dfu_alt_info() mechanism to set the
>> dfu_alt_info environment variable and instead explicitly set it to the
>> capsule update's setting just before performing the capsule update's DFU
>> operation, and then restore the environment variable back to its original
>> value.
>>
>> This patch implements the explicit setting and restoring of the
>> dfu_alt_info environment variable as part of the EFI capsule update
>> operation.
>>
>> The fix is fully implemented in a subsequent patch that removes the capsule
>> update dfu_alt_info support in set_dfu_alt_info().
>>
>> Signed-off-by: Jonathan Humphreys <j-humphr...@ti.com>
>> ---
>> lib/efi_loader/efi_firmware.c | 39 ++++++++++++++++++++++++++++++++---
>> 1 file changed, 36 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
>> index 5a754c9cd03..1a1cf3b55e1 100644
>> --- a/lib/efi_loader/efi_firmware.c
>> +++ b/lib/efi_loader/efi_firmware.c
>> @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>> efi_status_t (*progress)(efi_uintn_t completion),
>> u16 **abort_reason)
>> {
>> + int ret;
>> efi_status_t status;
>> struct fmp_state state = { 0 };
>> + char *orig_dfu_env;
>>
>> EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>> image_size, vendor_code, progress, abort_reason);
>> @@ -663,7 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>> if (status != EFI_SUCCESS)
>> return EFI_EXIT(status);
>>
>> - if (fit_update(image))
>> + orig_dfu_env = strdup(env_get("dfu_alt_info"));
>
> strdup might fail, we need to check that the orig_dfu_env is !NULL
>
>> + if (env_set("dfu_alt_info", update_info.dfu_string)) {
>> + log_err("unable to set env variable \"dfu_alt_info\"!\n");
>> + free(orig_dfu_env);
>> + return EFI_EXIT(EFI_DEVICE_ERROR);
>> + }
>> +
>> + ret = fit_update(image);
>
>> +
>> + if (env_set("dfu_alt_info", orig_dfu_env)) {
>> + log_err("unable to set env variable \"dfu_alt_info\"!\n");
>> + ret = 1;
>
> This will work but for consistency just use an EFI defined error e.g
> ret = EFI_DEVICE_ERROR;
>
>> + }
>> + free(orig_dfu_env);
>> +
>> + if (ret)
>
> I am not 100% sure this exiting here is useful. If fit_update has succeeded
> we have updated our firmware. So I think we should just add a warning here,
> that resetting the dfu to its original value failed, and any further dfu
> ops will fail.
>
Hi Ilias, thanks for the review.
The exit here is for either fit_update() failing OR we cannot restore the
original value for the dfu_alt_info variable, so it is needed, at least in
the fit_update() failure case.
I was trying to keep the control flow simpler by handling the 2 failure
events together. Are you wanting to separate the 2 failure conditions, and
return EFI_DEVICE_ERROR if fit_update() fails, and emit a warning and
return EFI_SUCCESS if restoring the env variable fails?
Jon
>> return EFI_EXIT(EFI_DEVICE_ERROR);
>>
>> efi_firmware_set_fmp_state_var(&state, image_index);
>> @@ -717,6 +734,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>> u8 dfu_alt_num;
>> efi_status_t status;
>> struct fmp_state state = { 0 };
>> + char *orig_dfu_env;
>>
>> EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>> image_size, vendor_code, progress, abort_reason);
>> @@ -747,8 +765,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>> }
>> }
>>
>> - if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
>> - NULL, NULL))
>> + orig_dfu_env = strdup(env_get("dfu_alt_info"));
>> + if (env_set("dfu_alt_info", update_info.dfu_string)) {
>> + log_err("unable to set env variable \"dfu_alt_info\"!\n");
>> + free(orig_dfu_env);
>> + return EFI_EXIT(EFI_DEVICE_ERROR);
>> + }
>> +
>> + ret = dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
>> + NULL, NULL);
>> +
>> + if (env_set("dfu_alt_info", orig_dfu_env)) {
>> + log_err("unable to set env variable \"dfu_alt_info\"!\n");
>> + ret = 1;
>> + }
>> + free(orig_dfu_env);
>> +
>> + if (ret)
>> return EFI_EXIT(EFI_DEVICE_ERROR);
>>
>> efi_firmware_set_fmp_state_var(&state, image_index);
>> --
>> 2.34.1
>>
>
> Thanks
> /Ilias
