[PATCH] dfu: Prevent set_dfu_alt_info() from overwriting a previous value

[Jon Humphreys]

Mattijs Korpershoek <mkorpershoek at baylibre.com> writes:

> Hi Jonathan,
>
> Thank you for the patch.
>
> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys at ti.com> wrote:
>
>> If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment
>> variable is dynamically set when initializing the DFU entities, which is
>> done as part of normal flow of a DFU operation.
>>
>> The USB DFU boot support will set it's own specific value for dfu_alt_info
>> before performing the DFU operation. This means that if
>> CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable
>> that the USB DFU boot path had set is overwritten, causing USB DFU boot to
>> fail.
>>
>> Likewise, if the user sets their own value for dfu_alt_info, say at the
>> U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
>>
>> This patch will first check that dfu_alt_info isn't already set before
>> calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
>
> To me, this is a policy change: before we could override the environment
> via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already
> set in the environment).
>
> Also, it seems that this change goes against the uefi doc which states:
>
> """
> A string is defined which is to be used for populating the
> dfu_alt_info variable. This string is used by the function
> set_dfu_alt_info. Instead of taking the variable from the environment,
> the capsule update feature requires that the variable be set through
> the function, since that is more robust. Allowing the user to change
> the location of the firmware updates is not a very secure
> practice. Getting this information from the firmware itself is more
> secure, assuming the firmware has been verified by a previous stage
> boot loader.
> """
>
> See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-update
>
> Moreover, looking at various boards that implement
> set_dfu_alt_info(), we can see different behaviours:
>
> Boards examples that won't override "dfu_alt_info" via
> set_dfu_alt_info() if "dfu_alt_info" is already set via environment
>
> * board/xilinx/zynq/board.c
> * board/emulation/common/qemu_dfu.c
>
> Boards examplesthat will always override the "dfu_alt_info" via
> set_dfu_alt_info():
>
> * board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
> * board/ti/am62px/evm.c
>
> Since set_dfu_alt_info() is a board specific callback, why can't this
> logic be implemented for boards that want this behaviour change?

Because I would then need to duplicate the same logic for every board that
wanted both USB DFU boot and EFI capsules to work.  And the paramters
passed in do not allow the function to know the use case (am I DFU booting
or updating EFI capsules?).  See more below.

>
> Regards,
>
> Mattijs
>
>>
>> Signed-off-by: Jonathan Humphreys <j-humphreys at ti.com>
>> ---
>>  drivers/dfu/dfu.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>> index 756569217bb..ab8abae1d89 100644
>> --- a/drivers/dfu/dfu.c
>> +++ b/drivers/dfu/dfu.c
>> @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
>>  	dfu_reinit_needed = false;
>>  	dfu_alt_info_changed = false;
>>  
>> +	str_env = env_get("dfu_alt_info");
>>  #ifdef CONFIG_SET_DFU_ALT_INFO
>> -	set_dfu_alt_info(interface, devstr);
>> +	if (!str_env) {
>> +		set_dfu_alt_info(interface, devstr);
>> +		str_env = env_get("dfu_alt_info");
>> +	}
>>  #endif
>> -	str_env = env_get("dfu_alt_info");
>>  	if (!str_env) {
>>  		pr_err("\"dfu_alt_info\" env variable not defined!\n");
>>  		return -EINVAL;
>> -- 
>> 2.34.1

Mattijs, thanks for the thorough reply.  I did wrestle a lot with how wide
of a fix to propose for this problem, and in the end, decided on the narrow
fix of simply preventing the overwriting of the variable.

Yes it is a policy change, but the policy is already unclear, inconsistent,
and confusing, IMO.

For example:
1) EFI capsule update wants to very strictly control the dfu alt values
   by setting it in set_dfu_alt_info(), but then any other DFU use
   case breaks.  USB DFU boot is now broken.
2) The behavior the user sees wrt the dfu_alt_info env variable is very
   confusing and non-intuitive. Take this example:

=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000"
=> env print dfu_alt_info
dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000
=> dfu 0 list
DFU alt settings list:
dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR
dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR
dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR
=> env print dfu_alt_info
dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000
=>

As you can see, the user set's the dfu_alt_info variable according to their
specific use case, then simply tries to list the DFU alt settings, and
because this code goes through the dfu_init_env_entities() path, it gets
changed to the EFI capsule settings.

I was hoping to get a simpler fix in now so we can get USB DFU boot working
again, and we can visit the overall policy design next.  As you suggest, I
could also push the testing of overwriting into the board specific
set_dfu_alt_info() function, but then I need to duplicate the code in 8
different places for the TI boards, and other vendors may still have the
problem.

Looking to the longer term solution, here are my thoughts.
1) We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules.  The only
   reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI
   capsule update is enabled.  Outside of a few legacy uses (I think - it
   appears they were introduced prior to supporting multi-interface dfu alt
   strings), I think this is true for other vendor's boards as well.
2) Have EFI capsule support do as USB DFU boot does today, and set the
   dfu alt string it wants used *before* initiating the DFU operation.  With
   CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get
   overridden.
3) Have the actual value of the dfu alt string used in the DFU operation be
   passed in, rather than read from the dfu_alt_info environment variable.
   The USB DFU and EFI capsule use case will pass in the dfu alt string
   they want. The standard 'dfu' command can pass in the value of the
   dfu_alt_info env variable. Note that this effectively decouples the dfu
   command from the alt settings that USB DFU boot and EFI capsules use,
   but I think this is what we want.

This then allows both USB DFU boot and EFI capsule use cases to work as
intended and allows the dfu command to operate on the user defined
dfu_alt_info value.

I welcome comments from those that have the history and intended behavior
background of the DFU support :).

I also welcome comments on how to proceed for 2025.01. Should we live with
USB DFU boot broken until we get the long term fix in, or ok with the patch
posted here. The patch posted here does allow for a user to change EFI
capsule's dfu alt settings, as Mattijs says, but especially given capsules
can be authenticated, I'm not sure how this would be exploited, and if that
risk is worse that broken DFU boot.

thank
Jon