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

[Mattijs Korpershoek]

Hi Jon,

Sorry for the (very) late reply. I had some long holidays in between and
since this is a difficult topic for me, I kept pushing this to the end
of my backlog.

On mer., déc. 18, 2024 at 17:09, Jon Humphreys <j-humphreys at ti.com> wrote:

> 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.

I understand that duplicating logic for every board you maintain is not
optimal, however, it gives each vendor the freedom of implementing their
policy.

I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.

Heinrich, Ilias, Sugosh, do you have any opinion on this patch?

>
>>
>> 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.

I agree that the above behaviour is confusing and I'm reconsidering to
take up this patch. I'd like some buy-in from either Heinrich, Ilias or
Sughosh on this since I'm not 100% confortable with the "policy change"

>
> 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 do as well. I have taken over maintaince on this subsystem a year ago
and have not had much patches/work done on the subsystem. Therefore I'm
not as knowledgeable as I would have liked to be. I'm sorry about that.

>
> 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