diff mbox series

drivers: spi: fix deref ater null.might in spi-uclass.c

Message ID 20250516125053.25457-1-ant.v.moryakov@gmail.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series drivers: spi: fix deref ater null.might in spi-uclass.c | expand

Commit Message

Anton Moryakov May 16, 2025, 12:50 p.m. UTC 
From: Anton Moryakov <ant.v.moryakov@gmail.com>

The static analyzer (Svace) reported 
After having been compared to a NULL value at spi-uclass.c:465,
pointer 'dev' is passed as 1st parameter in call to function 'dev_get_flags' 
at spi-uclass.c:469, where it is dereferenced at device.h:240.

Correct explained:
1. Added dev && !device_active(dev) check before calling device_active()
2. Added explicit if (!dev) check with ret = -ENODEV setting
3. Protected logging in error block with if(dev) check

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
---
 drivers/spi/spi-uclass.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Tom Rini May 16, 2025, 3:56 p.m. UTC | #1 
On Fri, May 16, 2025 at 03:50:53PM +0300, ant.v.moryakov@gmail.com wrote:
> From: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> The static analyzer (Svace) reported 
> After having been compared to a NULL value at spi-uclass.c:465,
> pointer 'dev' is passed as 1st parameter in call to function 'dev_get_flags' 
> at spi-uclass.c:469, where it is dereferenced at device.h:240.
> 
> Correct explained:
> 1. Added dev && !device_active(dev) check before calling device_active()
> 2. Added explicit if (!dev) check with ret = -ENODEV setting
> 3. Protected logging in error block with if(dev) check
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> ---
>  drivers/spi/spi-uclass.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index d6049753740..52b79223f96 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -345,7 +345,7 @@ int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
>  		return ret;
>  	}
>  
> -	if (!device_active(dev)) {
> +	if (dev && !device_active(dev)) {
>  		struct spi_slave *slave;
>  
>  		ret = device_probe(dev);

Given that it looks like from a quick glance most places do not check
for dev before dereferencing perhaps this should be done in
dev_get_flags or so, and a follow-up for the places which had already
been checking.
Simon Glass May 16, 2025, 4:20 p.m. UTC | #2 
Hi,

On Fri, 16 May 2025 at 14:51, <ant.v.moryakov@gmail.com> wrote:
>
> From: Anton Moryakov <ant.v.moryakov@gmail.com>
>
> The static analyzer (Svace) reported
> After having been compared to a NULL value at spi-uclass.c:465,
> pointer 'dev' is passed as 1st parameter in call to function 'dev_get_flags'
> at spi-uclass.c:469, where it is dereferenced at device.h:240.
>
> Correct explained:
> 1. Added dev && !device_active(dev) check before calling device_active()
> 2. Added explicit if (!dev) check with ret = -ENODEV setting
> 3. Protected logging in error block with if(dev) check
>
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> ---
>  drivers/spi/spi-uclass.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> index d6049753740..52b79223f96 100644
> --- a/drivers/spi/spi-uclass.c
> +++ b/drivers/spi/spi-uclass.c
> @@ -345,7 +345,7 @@ int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
>                 return ret;
>         }
>
> -       if (!device_active(dev)) {
> +       if (dev && !device_active(dev)) {
>                 struct spi_slave *slave;
>
>                 ret = device_probe(dev);
> @@ -355,6 +355,11 @@ int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
>                 slave->dev = dev;
>         }
>
> +       if (!dev) {
> +               ret = -ENODEV;
> +               goto err;
> +       }
> +
>         slave = dev_get_parent_priv(dev);
>         bus_data = dev_get_uclass_priv(bus);
>
> @@ -373,8 +378,11 @@ int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
>         return 0;
>
>  err:
> -       log_debug("%s: Error path, device '%s'\n", __func__, dev->name);
> -
> +       if(dev)
> +               log_debug("%s: Error path, device '%s'\n", __func__, dev->name);
> +       else
> +               log_debug("%s: Error path, NULL device\n", __func__);
> +
>         return ret;
>  }
>
> --
> 2.30.2
>

You are not allowed to pass NULL as dev to driver model functions. So
the bug here is in the SPI code.

There is a code-size cost to checking arguments and we tend to be
reluctant to pay it in U-Boot, where code size is often critical.

We do have assert() but it is fairly intrusive and people seldom
enable it. If you have a better way, please suggest it!

Regards,
Simon
Anton Moryakov May 16, 2025, 4:24 p.m. UTC | #3 
I can then leave only
+       if (!dev) {
+               ret = -ENODEV;
+               goto err;
+       }
+
but make it higher
How do you like this option?


пт, 16 мая 2025 г. в 19:20, Simon Glass <sjg@chromium.org>:

> Hi,
>
> On Fri, 16 May 2025 at 14:51, <ant.v.moryakov@gmail.com> wrote:
> >
> > From: Anton Moryakov <ant.v.moryakov@gmail.com>
> >
> > The static analyzer (Svace) reported
> > After having been compared to a NULL value at spi-uclass.c:465,
> > pointer 'dev' is passed as 1st parameter in call to function
> 'dev_get_flags'
> > at spi-uclass.c:469, where it is dereferenced at device.h:240.
> >
> > Correct explained:
> > 1. Added dev && !device_active(dev) check before calling device_active()
> > 2. Added explicit if (!dev) check with ret = -ENODEV setting
> > 3. Protected logging in error block with if(dev) check
> >
> > Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> > ---
> >  drivers/spi/spi-uclass.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> > index d6049753740..52b79223f96 100644
> > --- a/drivers/spi/spi-uclass.c
> > +++ b/drivers/spi/spi-uclass.c
> > @@ -345,7 +345,7 @@ int spi_get_bus_and_cs(int busnum, int cs, struct
> udevice **busp,
> >                 return ret;
> >         }
> >
> > -       if (!device_active(dev)) {
> > +       if (dev && !device_active(dev)) {
> >                 struct spi_slave *slave;
> >
> >                 ret = device_probe(dev);
> > @@ -355,6 +355,11 @@ int spi_get_bus_and_cs(int busnum, int cs, struct
> udevice **busp,
> >                 slave->dev = dev;
> >         }
> >
> > +       if (!dev) {
> > +               ret = -ENODEV;
> > +               goto err;
> > +       }
> > +
> >         slave = dev_get_parent_priv(dev);
> >         bus_data = dev_get_uclass_priv(bus);
> >
> > @@ -373,8 +378,11 @@ int spi_get_bus_and_cs(int busnum, int cs, struct
> udevice **busp,
> >         return 0;
> >
> >  err:
> > -       log_debug("%s: Error path, device '%s'\n", __func__, dev->name);
> > -
> > +       if(dev)
> > +               log_debug("%s: Error path, device '%s'\n", __func__,
> dev->name);
> > +       else
> > +               log_debug("%s: Error path, NULL device\n", __func__);
> > +
> >         return ret;
> >  }
> >
> > --
> > 2.30.2
> >
>
> You are not allowed to pass NULL as dev to driver model functions. So
> the bug here is in the SPI code.
>
> There is a code-size cost to checking arguments and we tend to be
> reluctant to pay it in U-Boot, where code size is often critical.
>
> We do have assert() but it is fairly intrusive and people seldom
> enable it. If you have a better way, please suggest it!
>
> Regards,
> Simon
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
index d6049753740..52b79223f96 100644
--- a/drivers/spi/spi-uclass.c
+++ b/drivers/spi/spi-uclass.c
@@ -345,7 +345,7 @@  int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
 		return ret;
 	}
 
-	if (!device_active(dev)) {
+	if (dev && !device_active(dev)) {
 		struct spi_slave *slave;
 
 		ret = device_probe(dev);
@@ -355,6 +355,11 @@  int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
 		slave->dev = dev;
 	}
 
+	if (!dev) {
+		ret = -ENODEV;
+		goto err;
+	}
+
 	slave = dev_get_parent_priv(dev);
 	bus_data = dev_get_uclass_priv(bus);
 
@@ -373,8 +378,11 @@  int spi_get_bus_and_cs(int busnum, int cs, struct udevice **busp,
 	return 0;
 
 err:
-	log_debug("%s: Error path, device '%s'\n", __func__, dev->name);
-
+	if(dev)
+		log_debug("%s: Error path, device '%s'\n", __func__, dev->name);
+	else
+		log_debug("%s: Error path, NULL device\n", __func__);
+	
 	return ret;
 }