On Fri, Jul 11, 2025 at 3:02 AM Mattijs Korpershoek
<mkorpersh...@kernel.org> wrote:
>
> Hi Sam,
>
> Thank you for the patch.
>
> On Tue, Jul 08, 2025 at 23:23, Sam Protsenko <semen.protse...@linaro.org>
> 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 <semen.protse...@linaro.org>
>
> 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!
>
Sometimes it takes longer to write a commit message than to come up
with the patch itself, hehe. I also tried to come up with some test
for that use case, but when you run the Py test suite on the sandbox
-- it just skips the DFU test. I figured some env hooks have to be
provided to make it work, but it took too much time for me already, so
I kinda ended up skipping that part.
> Reviewed-by: Mattijs Korpershoek <mkorpersh...@kernel.org>
>
> > ---
> > 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
