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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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