[PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Michael Ferolito]

The current behaviour of this function will dereference a null pointer
if the serial# environment variable is unset. This was discovered on a
board where U-Boot did not have access to the first 256MB of ram,
resulting in a board crash.
In the event that U-Boot has full access to memory, it will still read
from address 0, which is probably not optimal.
This simple check is enough to fix it

Signed-off-by: Michael Ferolito <michaelsunn101@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Heiko Schocher <hs@denx.de>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/g_dnl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index 631969b340..f2540eb6de 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -207,7 +207,8 @@ void g_dnl_clear_detach(void)
 static int on_serialno(const char *name, const char *value, enum env_op op,
 		int flags)
 {
-	g_dnl_set_serialnumber((char *)value);
+	if (value)
+		g_dnl_set_serialnumber((char *)value);
 	return 0;
 }
 U_BOOT_ENV_CALLBACK(serialno, on_serialno);
-- 
2.48.1

Re: [PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Heiko Schocher]

Hi Michael,

On 28.01.25 04:09, Michael Ferolito wrote:
> The current behaviour of this function will dereference a null pointer
> if the serial# environment variable is unset. This was discovered on a
> board where U-Boot did not have access to the first 256MB of ram,
> resulting in a board crash.
> In the event that U-Boot has full access to memory, it will still read
> from address 0, which is probably not optimal.
> This simple check is enough to fix it
> 
> Signed-off-by: Michael Ferolito <michaelsunn101@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>   drivers/usb/gadget/g_dnl.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Nitpick:
A changelog would be nice to get an idea what has changed from version
to version ...

> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index 631969b340..f2540eb6de 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -207,7 +207,8 @@ void g_dnl_clear_detach(void)
>   static int on_serialno(const char *name, const char *value, enum env_op op,
>   		int flags)
>   {
> -	g_dnl_set_serialnumber((char *)value);
> +	if (value)
> +		g_dnl_set_serialnumber((char *)value);
>   	return 0;
>   }
>   U_BOOT_ENV_CALLBACK(serialno, on_serialno);
> 

Good catch!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Mattijs Korpershoek]

Hi Michael,

Thank you for the patch.

On lun., janv. 27, 2025 at 21:09, Michael Ferolito <michaelsunn101@gmail.com> wrote:

> The current behaviour of this function will dereference a null pointer
> if the serial# environment variable is unset. This was discovered on a
> board where U-Boot did not have access to the first 256MB of ram,
> resulting in a board crash.
> In the event that U-Boot has full access to memory, it will still read
> from address 0, which is probably not optimal.
> This simple check is enough to fix it
>
> Signed-off-by: Michael Ferolito <michaelsunn101@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/g_dnl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index 631969b340..f2540eb6de 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -207,7 +207,8 @@ void g_dnl_clear_detach(void)
>  static int on_serialno(const char *name, const char *value, enum env_op op,
>  		int flags)
>  {
> -	g_dnl_set_serialnumber((char *)value);
> +	if (value)
> +		g_dnl_set_serialnumber((char *)value);
>  	return 0;
>  }
>  U_BOOT_ENV_CALLBACK(serialno, on_serialno);
> -- 
> 2.48.1

Re: [PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Michael]

Are there any other action items I should take care of?


On Tue, Jan 28, 2025 at 3:40 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Michael,
>
> Thank you for the patch.
>
> On lun., janv. 27, 2025 at 21:09, Michael Ferolito <michaelsunn101@gmail.com> wrote:
>
> > The current behaviour of this function will dereference a null pointer
> > if the serial# environment variable is unset. This was discovered on a
> > board where U-Boot did not have access to the first 256MB of ram,
> > resulting in a board crash.
> > In the event that U-Boot has full access to memory, it will still read
> > from address 0, which is probably not optimal.
> > This simple check is enough to fix it
> >
> > Signed-off-by: Michael Ferolito <michaelsunn101@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Heiko Schocher <hs@denx.de>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> > ---
> >  drivers/usb/gadget/g_dnl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> > index 631969b340..f2540eb6de 100644
> > --- a/drivers/usb/gadget/g_dnl.c
> > +++ b/drivers/usb/gadget/g_dnl.c
> > @@ -207,7 +207,8 @@ void g_dnl_clear_detach(void)
> >  static int on_serialno(const char *name, const char *value, enum env_op op,
> >               int flags)
> >  {
> > -     g_dnl_set_serialnumber((char *)value);
> > +     if (value)
> > +             g_dnl_set_serialnumber((char *)value);
> >       return 0;
> >  }
> >  U_BOOT_ENV_CALLBACK(serialno, on_serialno);
> > --
> > 2.48.1

Re: [PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Mattijs Korpershoek]

Hi Michael,

On mar., janv. 28, 2025 at 23:31, Michael <michaelsunn101@gmail.com> wrote:

> Are there any other action items I should take care of?

TLDR: no.

Details below:
Not at the moment. Usually we give the community some time to review.
This time depends on the custodian(maintainer) responsible for taking in the patch.

Since I'm responsible for fastboot, I usually give between 1 and 2 weeks
before I apply the changes to my tree: https://source.denx.de/u-boot/custodians/u-boot-dfu

When the change is applied, CI will test your patch. If something fails,
I might reach out to you again to help troubleshooting.

If it passes, I will later on send a pull request to Tom with this patch
included.
Once Tom accepts the pull request, this change will be merged into
master.

More details on the process here:
https://docs.u-boot.org/en/latest/develop/process.html#work-flow-of-a-custodian

Hope that clarifies things a bit, and thank you for contributing!
Mattijs

>
>
> On Tue, Jan 28, 2025 at 3:40 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Michael,
>>
>> Thank you for the patch.
>>
>> On lun., janv. 27, 2025 at 21:09, Michael Ferolito <michaelsunn101@gmail.com> wrote:
>>
>> > The current behaviour of this function will dereference a null pointer
>> > if the serial# environment variable is unset. This was discovered on a
>> > board where U-Boot did not have access to the first 256MB of ram,
>> > resulting in a board crash.
>> > In the event that U-Boot has full access to memory, it will still read
>> > from address 0, which is probably not optimal.
>> > This simple check is enough to fix it
>> >
>> > Signed-off-by: Michael Ferolito <michaelsunn101@gmail.com>
>> > Cc: Marek Vasut <marex@denx.de>
>> > Cc: Heiko Schocher <hs@denx.de>
>> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>
>> > ---
>> >  drivers/usb/gadget/g_dnl.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
>> > index 631969b340..f2540eb6de 100644
>> > --- a/drivers/usb/gadget/g_dnl.c
>> > +++ b/drivers/usb/gadget/g_dnl.c
>> > @@ -207,7 +207,8 @@ void g_dnl_clear_detach(void)
>> >  static int on_serialno(const char *name, const char *value, enum env_op op,
>> >               int flags)
>> >  {
>> > -     g_dnl_set_serialnumber((char *)value);
>> > +     if (value)
>> > +             g_dnl_set_serialnumber((char *)value);
>> >       return 0;
>> >  }
>> >  U_BOOT_ENV_CALLBACK(serialno, on_serialno);
>> > --
>> > 2.48.1

Re: [PATCH v4] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset

[Mattijs Korpershoek]

Hi,

On Mon, 27 Jan 2025 21:09:45 -0600, Michael Ferolito wrote:
> The current behaviour of this function will dereference a null pointer
> if the serial# environment variable is unset. This was discovered on a
> board where U-Boot did not have access to the first 256MB of ram,
> resulting in a board crash.
> In the event that U-Boot has full access to memory, it will still read
> from address 0, which is probably not optimal.
> This simple check is enough to fix it
> 
> [...]

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

[1/1] usb: gadget: g_dnl: Fix NULLPTR dereference when serial# is unset
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/dcf1c627cf436191919c5a3b153d1033245b54b7

--
Mattijs