Hi Jon,

On Mon, 3 Feb 2025 at 23:38, Jon Humphreys <j-humphr...@ti.com> wrote:
>
> Ilias Apalodimas <ilias.apalodi...@linaro.org> writes:
>
> > Hi Jon,
> >
> > On Fri, 17 Jan 2025 at 00:02, Jon Humphreys <j-humphr...@ti.com> wrote:
> >>
> >> Sughosh Ganu <sughosh.g...@linaro.org> writes:
> >>
> >> > On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek
> >> > <mkorpersh...@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-humphr...@ti.com> 
> >> >> wrote:
> >> >>
> >> >> > Mattijs Korpershoek <mkorpersh...@baylibre.com> writes:
> >> >> >
> >> >> >> Hi Jonathan,
> >> >> >>
> >> >> >> Thank you for the patch.
> >> >> >>
> >> >> >> On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys 
> >> >> >> <j-humphr...@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-humphr...@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

Reply via email to