Re: [PATCH] mfd: ene-kb3930: Fix potential NULL pointer dereference

[Lee Jones <lee@kernel.org>]

On Mon, 10 Feb 2025, Chenyuan Yang wrote:

> On Mon, Feb 10, 2025 at 11:34 AM Lee Jones <lee@kernel.org> wrote:
> >
> > On Mon, 20 Jan 2025, Chenyuan Yang wrote:
> >
> > > The off_gpios could be NULL. Add missing check in the kb3930_probe().
> > > This is similar to the issue fixed in commit
> > > https://patchwork.kernel.org/project/linux-fbdev/patch/20240114143921.550736-1-andriy.shevchenko@linux.intel.com/> >
> > > Fixes: ede6b2d1dfc0 ("mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller")
> > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > > ---
> > >  drivers/mfd/ene-kb3930.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/mfd/ene-kb3930.c b/drivers/mfd/ene-kb3930.c
> > > index fa0ad2f14a39..60824d847bf0 100644
> > > --- a/drivers/mfd/ene-kb3930.c
> > > +++ b/drivers/mfd/ene-kb3930.c
> > > @@ -162,6 +162,8 @@ static int kb3930_probe(struct i2c_client *client)
> > >                       devm_gpiod_get_array_optional(dev, "off", GPIOD_IN);
> > >               if (IS_ERR(ddata->off_gpios))
> > >                       return PTR_ERR(ddata->off_gpios);
> > > +             if (!ddata->off_gpios)
> > > +                     return -ENOMEM;
> >
> > I don't see many other call sites checking for NULL - why is this
> > different?
> 
> It looks like other places using devm_gpiod_get_array_optional do
> check for a NULL return value, such as in
> drivers/iio/resolver/ad2s1210.c, drivers/mtd/maps/physmap-core.c, and
> this patch commit:
> https://patchwork.kernel.org/project/linux-fbdev/patch/20240114143921.550736-1-andriy.shevchenko@linux.intel.com/.
> 
> > What about IS_ERR_OR_NULL() instead?
> 
> Yes, that sounds good. However, I have a question—what error number
> should be returned in this case?

I just took a closer look and don't thing returning on NULL is correct.

The call is *_optional() meaning that NULL is an acceptable value.

I believe that this is the correct solution:

diff --git a/drivers/mfd/ene-kb3930.c b/drivers/mfd/ene-kb3930.c
index fa0ad2f14a39..9460a67acb0b 100644
--- a/drivers/mfd/ene-kb3930.c
+++ b/drivers/mfd/ene-kb3930.c
@@ -162,7 +162,7 @@ static int kb3930_probe(struct i2c_client *client)
                        devm_gpiod_get_array_optional(dev, "off", GPIOD_IN);
                if (IS_ERR(ddata->off_gpios))
                        return PTR_ERR(ddata->off_gpios);
-               if (ddata->off_gpios->ndescs < 2) {
+               if (ddata->off_gpios && ddata->off_gpios->ndescs < 2) {
                        dev_err(dev, "invalid off-gpios property\n");
                        return -EINVAL;
                }

-- 
Lee Jones [李琼斯]