Hi Sam, Thank you for the patch.
On Tue, Jul 08, 2025 at 23:23, Sam Protsenko <[email protected]> wrote: > As stated in DFU documentation [1], the device interface part might be > missing in dfu_alt_info: > > dfu_alt_info > The DFU setting for the USB download gadget with a semicolon > separated string of information on each alternate: > dfu_alt_info="<alt1>;<alt2>;....;<altN>" > When several devices are used, the format is: > - <interface> <dev>'='alternate list (';' separated) > > So in first case dfu_alt_info might look like something like this: > > dfu_alt_info="mmc 0=rawemmc raw 0 0x747c000 mmcpart 1;" > > And in second case (when the interface is missing): > > dfu_alt_info="rawemmc raw 0 0x747c000 mmcpart 1;" > > When the interface is not specified the 'dfu' command crashes when > called using 'dfu 0' or 'dfu list' syntax: > > => dfu list > "Synchronous Abort" handler, esr 0x96000006, far 0x0 > > That's happening due to incorrect string handling in > dfu_config_interfaces(). In case when the interface is not specified in > dfu_alt_info it triggers this corner case: > > d = strsep(&s, "="); // now d contains s, and s is NULL > if (!d) > break; > a = strsep(&s, "&"); // s is already NULL, so a is NULL too > if (!a) // corner case > a = s; // a is NULL now > > which causes NULL pointer dereference later in this call, due to 'a' > being NULL: > > part = skip_spaces(part); > > That's because as per strsep() behavior, when delimiter ("&") is not > found, the token (a) becomes the entire string (s), and string (s) > becomes NULL. To fix that issue assign "a = d" instead of "a = s", > because at that point variable d actually contains previous s, which > should be used in this case. > > [1] doc/usage/dfu.rst > > Fixes: commit febabe3ed4f4 ("dfu: allow to manage DFU on several devices") > Signed-off-by: Sam Protsenko <[email protected]> Good catch! I can indeed reproduce this on sandbox after enabling CMD_DFU: $ ./u-boot -T [...] => setenv dfu_alt_info "rawemmc raw 0 0x747c000 mmcpart 1;" => dfu list [1] 116917 segmentation fault (core dumped) ./u-boot -T And I confirm it's fixed with this patch. Thanks for all the details, it makes the review and the testing so much easier! Reviewed-by: Mattijs Korpershoek <[email protected]> > --- > drivers/dfu/dfu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c > index 756569217bbb..eefdf44ec877 100644 > --- a/drivers/dfu/dfu.c > +++ b/drivers/dfu/dfu.c > @@ -147,7 +147,7 @@ int dfu_config_interfaces(char *env) > break; > a = strsep(&s, "&"); > if (!a) > - a = s; > + a = d; > do { > part = strsep(&a, ";"); > part = skip_spaces(part); > -- > 2.39.5
