drivers/mfd/ene-kb3930.c | 2 ++ 1 file changed, 2 insertions(+)
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.1On 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 [李琼斯]
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
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 [李琼斯]
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 [李琼斯]
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
>
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
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
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
© 2016 - 2025 Red Hat, Inc.