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
