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

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

[Jonathan Humphreys]

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.

Signed-off-by: Jonathan Humphreys <j-humphreys@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

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

[Siddharth Vadapalli]

On Tue, Dec 17, 2024 at 02:48:35PM -0600, Jonathan Humphreys wrote:

Hello Jon,

Thank you for posting this patch. I will drop the equivalent of this
patch when I post the v2 series for:
https://patchwork.ozlabs.org/project/uboot/cover/20241217131658.2920799-1-s-vadapalli@ti.com/

> 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

nitpick: s/it's/its

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

nitpick: s/get's/gets

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

Rather than referring to "patch", we could rephrase the above as:

Check that dfu_alt_info isn't already set before calling set_dfu_alt_info()
when CONFIG_SET_DFU_ALT_INFO is enabled, in order to avoid overwriting it.

> 
> Signed-off-by: Jonathan Humphreys <j-humphreys@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;

Regards,
Siddharth.

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

[Mattijs Korpershoek]

Hi Jonathan,

Thank you for the patch.

On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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?

Regards,

Mattijs

>
> Signed-off-by: Jonathan Humphreys <j-humphreys@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

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

[Jon Humphreys]

Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:

> Hi Jonathan,
>
> Thank you for the patch.
>
> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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

Re: [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@ti.com> wrote:

> Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
>
>> Hi Jonathan,
>>
>> Thank you for the patch.
>>
>> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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

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

[Ilias Apalodimas]

Hi Mattijs,

On Thu, 16 Jan 2025 at 10:37, Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> 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@ti.com> wrote:
>
> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
> >
> >> Hi Jonathan,
> >>
> >> Thank you for the patch.
> >>
> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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?

Looking at it quickly, this would break the capsule update
functionality if a user sets a dfu_alt_info (or if he has one defined
in their board env file).
A board is generally expected to reset after a capsule update, so
should we consider implementing something different?
set_dfu_alt_info() unconditionally runs when dfu_init_env_entities()
runs. We could teach the EFI subsystem to set the dfu_alt_info just
before a capsule update runs and store any existing values. Then we
can restore it after the capsule update finishes

Cheers
/Ilias

>
> >
> >>
> >> Regards,
> >>
> >> Mattijs
> >>
> >>>
> >>> Signed-off-by: Jonathan Humphreys <j-humphreys@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

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

[Sughosh Ganu]

On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> 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@ti.com> wrote:
>
> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
> >
> >> Hi Jonathan,
> >>
> >> Thank you for the patch.
> >>
> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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"

A little context here. The DFU driver already had this policy in place
where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string
would be set by U-Boot, instead of taking the user provided string. It
was decided to use this for EFI capsule updates, as getting the string
which determines the location of writing the update images from within
U-Boot is more resilient than taking some user provided string.


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

I think either of 2) or 3) above can be looked at. Although not sure
if 2) will be breaking the current DFU policy.

-sughosh

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

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

[Jon Humphreys]

Sughosh Ganu <sughosh.ganu@linaro.org> writes:

> On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> 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@ti.com> wrote:
>>
>> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
>> >
>> >> Hi Jonathan,
>> >>
>> >> Thank you for the patch.
>> >>
>> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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"
>
> A little context here. The DFU driver already had this policy in place
> where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string
> would be set by U-Boot, instead of taking the user provided string. It
> was decided to use this for EFI capsule updates, as getting the string
> which determines the location of writing the update images from within
> U-Boot is more resilient than taking some user provided string.
>
>
>>
>> >
>> > 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.
>
> I think either of 2) or 3) above can be looked at. Although not sure
> if 2) will be breaking the current DFU policy.
>
> -sughosh
>

Thanks for the comments.  I have looked into this a bit further and see 2
options we can take for the EFI capsule update use case:

1) stick with the more traditional approach and do as DFU BOOT does by
   setting the value of the dfu_alt_info env variable just before
   initiating the DFU operation. As Ilias sugggested, we could also add a
   save/restore so this is transparent to other DFU users.

2) move to a model where we explicitly pass in to the DFU operation the
   value of dfu_alt_info that we want used. In the normal/legacy DFU use
   cases, this would involve calling env_get() for the dfu_alt_info env
   variable and pass that value. I like this approach because it is
   cleaner and more explicit. However, there are many layers of function
   calls between the driver of the DFU operation (the one that would decide
   what dfu_alt_info value to use) and dfu_init_env_entities() where it is
   used, and passing this info across the function interfaces would
   involved lots of function interface updates.

I'm curious if there is apetite for 2) or should we go with the more
traditional approach in 1).

The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules.
For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that
if they override the dfu_alt_info env variable, they know what they are
doing.

Thanks
Jon

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

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

[Ilias Apalodimas]

Hi Jon,

On Fri, 17 Jan 2025 at 00:02, Jon Humphreys <j-humphreys@ti.com> wrote:
>
> Sughosh Ganu <sughosh.ganu@linaro.org> writes:
>
> > On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
> > <mkorpershoek@baylibre.com> wrote:
> >>
> >> 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@ti.com> wrote:
> >>
> >> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
> >> >
> >> >> Hi Jonathan,
> >> >>
> >> >> Thank you for the patch.
> >> >>
> >> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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"
> >
> > A little context here. The DFU driver already had this policy in place
> > where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string
> > would be set by U-Boot, instead of taking the user provided string. It
> > was decided to use this for EFI capsule updates, as getting the string
> > which determines the location of writing the update images from within
> > U-Boot is more resilient than taking some user provided string.
> >
> >
> >>
> >> >
> >> > 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.
> >
> > I think either of 2) or 3) above can be looked at. Although not sure
> > if 2) will be breaking the current DFU policy.
> >
> > -sughosh
> >
>
> Thanks for the comments.  I have looked into this a bit further and see 2
> options we can take for the EFI capsule update use case:
>
> 1) stick with the more traditional approach and do as DFU BOOT does by
>    setting the value of the dfu_alt_info env variable just before
>    initiating the DFU operation. As Ilias sugggested, we could also add a
>    save/restore so this is transparent to other DFU users.
>
> 2) move to a model where we explicitly pass in to the DFU operation the
>    value of dfu_alt_info that we want used. In the normal/legacy DFU use
>    cases, this would involve calling env_get() for the dfu_alt_info env
>    variable and pass that value. I like this approach because it is
>    cleaner and more explicit. However, there are many layers of function
>    calls between the driver of the DFU operation (the one that would decide
>    what dfu_alt_info value to use) and dfu_init_env_entities() where it is
>    used, and passing this info across the function interfaces would
>    involved lots of function interface updates.
>
> I'm curious if there is apetite for 2) or should we go with the more
> traditional approach in 1).
>
> The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules.
> For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that
> if they override the dfu_alt_info env variable, they know what they are
> doing.

Decoupling that should be pretty easy. I personally like 2) more since
it's much more scalable and doesn't involve saving.restoring values.
But I am not sure how big of a task it is

Cheers
/Ilias
>
> Thanks
> Jon
>
> >> >
> >> > 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

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

[Jon Humphreys]

Ilias Apalodimas <ilias.apalodimas@linaro.org> writes:

> Hi Jon,
>
> On Fri, 17 Jan 2025 at 00:02, Jon Humphreys <j-humphreys@ti.com> wrote:
>>
>> Sughosh Ganu <sughosh.ganu@linaro.org> writes:
>>
>> > On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
>> > <mkorpershoek@baylibre.com> wrote:
>> >>
>> >> 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@ti.com> wrote:
>> >>
>> >> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
>> >> >
>> >> >> Hi Jonathan,
>> >> >>
>> >> >> Thank you for the patch.
>> >> >>
>> >> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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"
>> >
>> > A little context here. The DFU driver already had this policy in place
>> > where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string
>> > would be set by U-Boot, instead of taking the user provided string. It
>> > was decided to use this for EFI capsule updates, as getting the string
>> > which determines the location of writing the update images from within
>> > U-Boot is more resilient than taking some user provided string.
>> >
>> >
>> >>
>> >> >
>> >> > 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.
>> >
>> > I think either of 2) or 3) above can be looked at. Although not sure
>> > if 2) will be breaking the current DFU policy.
>> >
>> > -sughosh
>> >
>>
>> Thanks for the comments.  I have looked into this a bit further and see 2
>> options we can take for the EFI capsule update use case:
>>
>> 1) stick with the more traditional approach and do as DFU BOOT does by
>>    setting the value of the dfu_alt_info env variable just before
>>    initiating the DFU operation. As Ilias sugggested, we could also add a
>>    save/restore so this is transparent to other DFU users.
>>
>> 2) move to a model where we explicitly pass in to the DFU operation the
>>    value of dfu_alt_info that we want used. In the normal/legacy DFU use
>>    cases, this would involve calling env_get() for the dfu_alt_info env
>>    variable and pass that value. I like this approach because it is
>>    cleaner and more explicit. However, there are many layers of function
>>    calls between the driver of the DFU operation (the one that would decide
>>    what dfu_alt_info value to use) and dfu_init_env_entities() where it is
>>    used, and passing this info across the function interfaces would
>>    involved lots of function interface updates.
>>
>> I'm curious if there is apetite for 2) or should we go with the more
>> traditional approach in 1).
>>
>> The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules.
>> For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that
>> if they override the dfu_alt_info env variable, they know what they are
>> doing.
>
> Decoupling that should be pretty easy. I personally like 2) more since
> it's much more scalable and doesn't involve saving.restoring values.
> But I am not sure how big of a task it is
>

I went with option 1 above.  I started working on option 2 however the
changes are quite invasive, touching all of the DFU interfaces, and I do not
have the understanding of the different DFU use cases nor access to the
different hardware using those use cases to test.  I also think that this
change should be part of a broader DFU cleanup to use only the latest
dfu_alt_info string format allowing multiple devices, so that we no longer
need to pass around the interface and devstr parameters.

Please take a look at the patch series titled "EFI Capsule update
explicitly sets dfu_alt_info" that I will post shortly.

thanks
Jon

> Cheers
> /Ilias
>>
>> Thanks
>> Jon
>>
>> >> >
>> >> > 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

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

[Ilias Apalodimas]

Hi Jon,

On Mon, 3 Feb 2025 at 23:38, Jon Humphreys <j-humphreys@ti.com> wrote:
>
> Ilias Apalodimas <ilias.apalodimas@linaro.org> writes:
>
> > Hi Jon,
> >
> > On Fri, 17 Jan 2025 at 00:02, Jon Humphreys <j-humphreys@ti.com> wrote:
> >>
> >> Sughosh Ganu <sughosh.ganu@linaro.org> writes:
> >>
> >> > On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
> >> > <mkorpershoek@baylibre.com> wrote:
> >> >>
> >> >> 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@ti.com> wrote:
> >> >>
> >> >> > Mattijs Korpershoek <mkorpershoek@baylibre.com> writes:
> >> >> >
> >> >> >> Hi Jonathan,
> >> >> >>
> >> >> >> Thank you for the patch.
> >> >> >>
> >> >> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys@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@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"
> >> >
> >> > A little context here. The DFU driver already had this policy in place
> >> > where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string
> >> > would be set by U-Boot, instead of taking the user provided string. It
> >> > was decided to use this for EFI capsule updates, as getting the string
> >> > which determines the location of writing the update images from within
> >> > U-Boot is more resilient than taking some user provided string.
> >> >
> >> >
> >> >>
> >> >> >
> >> >> > 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.
> >> >
> >> > I think either of 2) or 3) above can be looked at. Although not sure
> >> > if 2) will be breaking the current DFU policy.
> >> >
> >> > -sughosh
> >> >
> >>
> >> Thanks for the comments.  I have looked into this a bit further and see 2
> >> options we can take for the EFI capsule update use case:
> >>
> >> 1) stick with the more traditional approach and do as DFU BOOT does by
> >>    setting the value of the dfu_alt_info env variable just before
> >>    initiating the DFU operation. As Ilias sugggested, we could also add a
> >>    save/restore so this is transparent to other DFU users.
> >>
> >> 2) move to a model where we explicitly pass in to the DFU operation the
> >>    value of dfu_alt_info that we want used. In the normal/legacy DFU use
> >>    cases, this would involve calling env_get() for the dfu_alt_info env
> >>    variable and pass that value. I like this approach because it is
> >>    cleaner and more explicit. However, there are many layers of function
> >>    calls between the driver of the DFU operation (the one that would decide
> >>    what dfu_alt_info value to use) and dfu_init_env_entities() where it is
> >>    used, and passing this info across the function interfaces would
> >>    involved lots of function interface updates.
> >>
> >> I'm curious if there is apetite for 2) or should we go with the more
> >> traditional approach in 1).
> >>
> >> The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules.
> >> For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that
> >> if they override the dfu_alt_info env variable, they know what they are
> >> doing.
> >
> > Decoupling that should be pretty easy. I personally like 2) more since
> > it's much more scalable and doesn't involve saving.restoring values.
> > But I am not sure how big of a task it is
> >
>
> I went with option 1 above.  I started working on option 2 however the
> changes are quite invasive, touching all of the DFU interfaces, and I do not
> have the understanding of the different DFU use cases nor access to the
> different hardware using those use cases to test.  I also think that this
> change should be part of a broader DFU cleanup to use only the latest
> dfu_alt_info string format allowing multiple devices, so that we no longer
> need to pass around the interface and devstr parameters.
>
> Please take a look at the patch series titled "EFI Capsule update
> explicitly sets dfu_alt_info" that I will post shortly.

Fair enough. I'll have a look tomorrow

Thanks!
/Ilias
>
> thanks
> Jon
>
> > Cheers
> > /Ilias
> >>
> >> Thanks
> >> Jon
> >>
> >> >> >
> >> >> > 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

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

[Mattijs Korpershoek]

On lun., févr. 03, 2025 at 15:38, Jon Humphreys <j-humphreys@ti.com> wrote:

> Ilias Apalodimas <ilias.apalodimas@linaro.org> writes:

[...]

>>
>
> I went with option 1 above.  I started working on option 2 however the
> changes are quite invasive, touching all of the DFU interfaces, and I do not
> have the understanding of the different DFU use cases nor access to the
> different hardware using those use cases to test.  I also think that this
> change should be part of a broader DFU cleanup to use only the latest
> dfu_alt_info string format allowing multiple devices, so that we no longer
> need to pass around the interface and devstr parameters.
>
> Please take a look at the patch series titled "EFI Capsule update
> explicitly sets dfu_alt_info" that I will post shortly.

For future reference, this is the mentioned series:

http://lore.kernel.org/r/20250203215351.2840144-1-j-humphreys@ti.com/

>
> thanks
> Jon
>

[...]