[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 at 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 at ti.com> wrote:
>
> > Mattijs Korpershoek <mkorpershoek at baylibre.com> writes:
> >
> >> Hi Jonathan,
> >>
> >> Thank you for the patch.
> >>
> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys <j-humphreys at ti.com> wrote:
> >>
> >>> If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment
> >>> variable is dynamically set when initializing the DFU entities, which is
> >>> done as part of normal flow of a DFU operation.
> >>>
> >>> The USB DFU boot support will set it's own specific value for dfu_alt_info
> >>> before performing the DFU operation. This means that if
> >>> CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable
> >>> that the USB DFU boot path had set is overwritten, causing USB DFU boot to
> >>> fail.
> >>>
> >>> Likewise, if the user sets their own value for dfu_alt_info, say at the
> >>> U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
> >>>
> >>> This patch will first check that dfu_alt_info isn't already set before
> >>> calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
> >>
> >> To me, this is a policy change: before we could override the environment
> >> via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already
> >> set in the environment).
> >>
> >> Also, it seems that this change goes against the uefi doc which states:
> >>
> >> """
> >> A string is defined which is to be used for populating the
> >> dfu_alt_info variable. This string is used by the function
> >> set_dfu_alt_info. Instead of taking the variable from the environment,
> >> the capsule update feature requires that the variable be set through
> >> the function, since that is more robust. Allowing the user to change
> >> the location of the firmware updates is not a very secure
> >> practice. Getting this information from the firmware itself is more
> >> secure, assuming the firmware has been verified by a previous stage
> >> boot loader.
> >> """
> >>
> >> See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-update
> >>
> >> Moreover, looking at various boards that implement
> >> set_dfu_alt_info(), we can see different behaviours:
> >>
> >> Boards examples that won't override "dfu_alt_info" via
> >> set_dfu_alt_info() if "dfu_alt_info" is already set via environment
> >>
> >> * board/xilinx/zynq/board.c
> >> * board/emulation/common/qemu_dfu.c
> >>
> >> Boards examplesthat will always override the "dfu_alt_info" via
> >> set_dfu_alt_info():
> >>
> >> * board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
> >> * board/ti/am62px/evm.c
> >>
> >> Since set_dfu_alt_info() is a board specific callback, why can't this
> >> logic be implemented for boards that want this behaviour change?
> >
> > Because I would then need to duplicate the same logic for every board that
> > wanted both USB DFU boot and EFI capsules to work.  And the paramters
> > passed in do not allow the function to know the use case (am I DFU booting
> > or updating EFI capsules?).  See more below.
>
> I understand that duplicating logic for every board you maintain is not
> optimal, however, it gives each vendor the freedom of implementing their
> policy.
>
> I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
>
> Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
>
> >
> >>
> >> Regards,
> >>
> >> Mattijs
> >>
> >>>
> >>> Signed-off-by: Jonathan Humphreys <j-humphreys at ti.com>
> >>> ---
> >>>  drivers/dfu/dfu.c | 7 +++++--
> >>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> >>> index 756569217bb..ab8abae1d89 100644
> >>> --- a/drivers/dfu/dfu.c
> >>> +++ b/drivers/dfu/dfu.c
> >>> @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr)
> >>>     dfu_reinit_needed = false;
> >>>     dfu_alt_info_changed = false;
> >>>
> >>> +   str_env = env_get("dfu_alt_info");
> >>>  #ifdef CONFIG_SET_DFU_ALT_INFO
> >>> -   set_dfu_alt_info(interface, devstr);
> >>> +   if (!str_env) {
> >>> +           set_dfu_alt_info(interface, devstr);
> >>> +           str_env = env_get("dfu_alt_info");
> >>> +   }
> >>>  #endif
> >>> -   str_env = env_get("dfu_alt_info");
> >>>     if (!str_env) {
> >>>             pr_err("\"dfu_alt_info\" env variable not defined!\n");
> >>>             return -EINVAL;
> >>> --
> >>> 2.34.1
> >
> > Mattijs, thanks for the thorough reply.  I did wrestle a lot with how wide
> > of a fix to propose for this problem, and in the end, decided on the narrow
> > fix of simply preventing the overwriting of the variable.
> >
> > Yes it is a policy change, but the policy is already unclear, inconsistent,
> > and confusing, IMO.
> >
> > For example:
> > 1) EFI capsule update wants to very strictly control the dfu alt values
> >    by setting it in set_dfu_alt_info(), but then any other DFU use
> >    case breaks.  USB DFU boot is now broken.
> > 2) The behavior the user sees wrt the dfu_alt_info env variable is very
> >    confusing and non-intuitive. Take this example:
> >
> > => env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000"
> > => env print dfu_alt_info
> > dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000
> > => dfu 0 list
> > DFU alt settings list:
> > dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR
> > dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR
> > dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR
> > => env print dfu_alt_info
> > dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000
> > =>
> >
> > As you can see, the user set's the dfu_alt_info variable according to their
> > specific use case, then simply tries to list the DFU alt settings, and
> > because this code goes through the dfu_init_env_entities() path, it gets
> > changed to the EFI capsule settings.
> >
> > I was hoping to get a simpler fix in now so we can get USB DFU boot working
> > again, and we can visit the overall policy design next.  As you suggest, I
> > could also push the testing of overwriting into the board specific
> > set_dfu_alt_info() function, but then I need to duplicate the code in 8
> > different places for the TI boards, and other vendors may still have the
> > problem.
>
> I agree that the above behaviour is confusing and I'm reconsidering to
> take up this patch. I'd like some buy-in from either Heinrich, Ilias or
> Sughosh on this since I'm not 100% confortable with the "policy change"

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