[PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[Sam Protsenko]

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.protsenko@linaro.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

Re: [PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[Mattijs Korpershoek]

Hi Sam,

Thank you for the patch.

On Tue, Jul 08, 2025 at 23:23, Sam Protsenko <semen.protsenko@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.protsenko@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!

Reviewed-by: Mattijs Korpershoek <mkorpershoek@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

Re: [PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[Sam Protsenko]

On Fri, Jul 11, 2025 at 3:02 AM Mattijs Korpershoek
<mkorpershoek@kernel.org> wrote:
>
> Hi Sam,
>
> Thank you for the patch.
>
> On Tue, Jul 08, 2025 at 23:23, Sam Protsenko <semen.protsenko@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.protsenko@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 <mkorpershoek@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

Re: [PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[Mattijs Korpershoek]

On Fri, Jul 11, 2025 at 15:14, Sam Protsenko <semen.protsenko@linaro.org> wrote:

> On Fri, Jul 11, 2025 at 3:02 AM Mattijs Korpershoek
> <mkorpershoek@kernel.org> wrote:
>>
>> Hi Sam,
>>
>> Thank you for the patch.

[...]

>>
>> 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.

Yes, we should definitely work on enabling testing DFU with sandbox.
This is out of scope for this patch, but I've opened a TODO item
for that here:

https://source.denx.de/u-boot/custodians/u-boot-dfu/-/issues/7

Thanks again for the fix.

Mattijs

>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@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

Re: [PATCH] dfu: Fix dfu_config_interfaces() for single interface DFU syntax

[Mattijs Korpershoek]

Hi,

On Tue, 08 Jul 2025 23:23:42 -0500, Sam Protsenko 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)
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/1] dfu: Fix dfu_config_interfaces() for single interface DFU syntax
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/772526fb9f55a1fea439ea3a6923136cd121cc88

--
Mattijs