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

[Anton Moryakov]

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 at chromium.org>:

> Hi,
>
> On Fri, 16 May 2025 at 14:51, <ant.v.moryakov at gmail.com> wrote:
> >
> > From: Anton Moryakov <ant.v.moryakov at 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 at 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
>