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

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

[Chenyuan Yang]

The off_gpios could be NULL. Add missing check in the kb3930_probe().
This is similar to the issue fixed in commit
b1ba8bcb2d1ffce11b308ce166c9cc28d989e3b9 ("backlight: hx8357:Fix potential NULL pointer dereference").

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;
 		if (ddata->off_gpios->ndescs < 2) {
 			dev_err(dev, "invalid off-gpios property\n");
 			return -EINVAL;
-- 
2.34.1

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

[Chenyuan Yang]

Dear Developers,

I am writing to check if there has been any update on this issue.
Given that it involves a potential null pointer dereference, I believe
we should address it as soon as possible

Best,
Chenyuan

On Mon, Jan 20, 2025 at 1:07 PM Chenyuan Yang <chenyuan0y@gmail.com> wrote:
>
> The off_gpios could be NULL. Add missing check in the kb3930_probe().
> This is similar to the issue fixed in commit
> b1ba8bcb2d1ffce11b308ce166c9cc28d989e3b9 ("backlight: hx8357:Fix potential NULL pointer dereference").
>
> 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;
>                 if (ddata->off_gpios->ndescs < 2) {
>                         dev_err(dev, "invalid off-gpios property\n");
>                         return -EINVAL;
> --
> 2.34.1
>

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

[Krzysztof Kozlowski]

On 27/01/2025 17:30, Chenyuan Yang wrote:
> Dear Developers,
> 
> I am writing to check if there has been any update on this issue.
> Given that it involves a potential null pointer dereference, I believe
> we should address it as soon as possible
> 
How did you test it or reproduce the issue? You claim that it is ASAP
and ping during the merge window, so I assume this is real issue and
tested on device?

Best regards,
Krzysztof

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

[Chenyuan Yang]

Hi Krzysztof,

This was caught by our static analysis tool because
devm_gpiod_get_array_optional() can return a NULL pointer in
low-memory situations. It's essentially the same scenario described
here:
https://patchwork.kernel.org/project/linux-fbdev/patch/20240114143921.550736-1-andriy.shevchenko@linux.intel.com/.

Best,
Chenyuan

On Mon, Jan 27, 2025 at 11:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 27/01/2025 17:30, Chenyuan Yang wrote:
> > Dear Developers,
> >
> > I am writing to check if there has been any update on this issue.
> > Given that it involves a potential null pointer dereference, I believe
> > we should address it as soon as possible
> >
> How did you test it or reproduce the issue? You claim that it is ASAP
> and ping during the merge window, so I assume this is real issue and
> tested on device?
>
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 27/01/2025 20:45, Chenyuan Yang wrote:
> Hi Krzysztof,
> 
> This was caught by our static analysis tool because
> devm_gpiod_get_array_optional() can return a NULL pointer in
> low-memory situations. It's essentially the same scenario described

Don't top post.

So you did not reproduce it, did not test it, thus I suggest not to ping
us as we missed anything. Try to get this tested first.

Best regards,
Krzysztof

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

[Lee Jones]

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
> b1ba8bcb2d1ffce11b308ce166c9cc28d989e3b9 ("backlight: hx8357:Fix potential NULL pointer dereference").
> 
> 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?

What about IS_ERR_OR_NULL() instead?

>  		if (ddata->off_gpios->ndescs < 2) {
>  			dev_err(dev, "invalid off-gpios property\n");
>  			return -EINVAL;
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]

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

[Chenyuan Yang]

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?

> >               if (ddata->off_gpios->ndescs < 2) {
> >                       dev_err(dev, "invalid off-gpios property\n");
> >                       return -EINVAL;
> > --
> > 2.34.1
> >
>
> --
> Lee Jones [李琼斯]

-Chenyuan

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

[Lee Jones]

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 [李琼斯]

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

[Chenyuan Yang]

Thanks so much for your insights!
I will resubmit the patch.

-Chenyuan

On Tue, Feb 11, 2025 at 9:06 AM Lee Jones <lee@kernel.org> wrote:
>
> 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 [李琼斯]