Re: [PATCH v3 2/3] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Jon Humphreys <j-humphreys@ti.com>]

Ilias Apalodimas <ilias.apalodimas@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-humphreys@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