[PATCH] leds: fix a potential NULL pointer dereference

[PATCH] leds: fix a potential NULL pointer dereference

[Kangjie Lu]

In case of_match_device cannot find a match, the fixes returns
-EINVAL to avoid NULL pointer dereference.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 drivers/leds/leds-pca9532.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index 7fea18b0c15d..4b0335591728 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
 	const struct i2c_device_id *id)
 {
 	int devid;
+	const struct of_device_id *of_id;
 	struct pca9532_data *data = i2c_get_clientdata(client);
 	struct pca9532_platform_data *pca9532_pdata =
 			dev_get_platdata(&client->dev);
@@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
 			dev_err(&client->dev, "no platform data\n");
 			return -EINVAL;
 		}
-		devid = (int)(uintptr_t)of_match_device(
-			of_pca9532_leds_match, &client->dev)->data;
+		of_id = of_match_device(of_pca9532_leds_match,
+				&client->dev);
+		if (unlikely(!of_id))
+			return -EINVAL;
+		devid = (int)of_id->data;
 	} else {
 		devid = id->driver_data;
 	}
-- 
2.17.1

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Jacek Anaszewski]

Hi Kangjie,

Thank you for the patch.

On 3/9/19 7:04 AM, Kangjie Lu wrote:
> In case of_match_device cannot find a match, the fixes returns
> -EINVAL to avoid NULL pointer dereference.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
>   drivers/leds/leds-pca9532.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index 7fea18b0c15d..4b0335591728 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
>   	const struct i2c_device_id *id)
>   {
>   	int devid;
> +	const struct of_device_id *of_id;
>   	struct pca9532_data *data = i2c_get_clientdata(client);
>   	struct pca9532_platform_data *pca9532_pdata =
>   			dev_get_platdata(&client->dev);
> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
>   			dev_err(&client->dev, "no platform data\n");
>   			return -EINVAL;
>   		}
> -		devid = (int)(uintptr_t)of_match_device(
> -			of_pca9532_leds_match, &client->dev)->data;
> +		of_id = of_match_device(of_pca9532_leds_match,
> +				&client->dev);
> +		if (unlikely(!of_id))
> +			return -EINVAL;
> +		devid = (int)of_id->data;
>   	} else {
>   		devid = id->driver_data;
>   	}


Applied to the for-5.2 branch of linux-leds.git.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Enrico Weigelt, metux IT consult]

On 10.03.19 21:27, Jacek Anaszewski wrote:
> Hi Kangjie,
> 
> Thank you for the patch.
> 
> On 3/9/19 7:04 AM, Kangjie Lu wrote:
>> In case of_match_device cannot find a match, the fixes returns
>> -EINVAL to avoid NULL pointer dereference.
>>
>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>> ---
>>   drivers/leds/leds-pca9532.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>> index 7fea18b0c15d..4b0335591728 100644
>> --- a/drivers/leds/leds-pca9532.c
>> +++ b/drivers/leds/leds-pca9532.c
>> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
>>       const struct i2c_device_id *id)
>>   {
>>       int devid;
>> +    const struct of_device_id *of_id;

Looks like an indention mismatch that might call for the
Great White Handkerchief ;-)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Geert Uytterhoeven]

Hi Jacek,

On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 3/9/19 7:04 AM, Kangjie Lu wrote:
> > In case of_match_device cannot find a match, the fixes returns
> > -EINVAL to avoid NULL pointer dereference.
> >
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > ---
> >   drivers/leds/leds-pca9532.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> > index 7fea18b0c15d..4b0335591728 100644
> > --- a/drivers/leds/leds-pca9532.c
> > +++ b/drivers/leds/leds-pca9532.c
> > @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
> >       const struct i2c_device_id *id)
> >   {
> >       int devid;
> > +     const struct of_device_id *of_id;
> >       struct pca9532_data *data = i2c_get_clientdata(client);
> >       struct pca9532_platform_data *pca9532_pdata =
> >                       dev_get_platdata(&client->dev);
> > @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
> >                       dev_err(&client->dev, "no platform data\n");
> >                       return -EINVAL;
> >               }
> > -             devid = (int)(uintptr_t)of_match_device(
> > -                     of_pca9532_leds_match, &client->dev)->data;
> > +             of_id = of_match_device(of_pca9532_leds_match,
> > +                             &client->dev);
> > +             if (unlikely(!of_id))

Use of unlikey() is frowned upon.
Moreover, this cannot happen, as pca9532_of_populate_pdata() already
contains a similar check.

Kangjie: please stop submitting patches for missing checks, without
investigating if the failures can actually happen. Thanks!

> > +                     return -EINVAL;
> > +             devid = (int)of_id->data;
> >       } else {
> >               devid = id->driver_data;
> >       }
>
>
> Applied to the for-5.2 branch of linux-leds.git.

And also as a fix for v5.1...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Jacek Anaszewski]

Hi Linus,

Yesterday I submitted pull request [0] containing this patch, but
the patch turns out to be pointless. Since I see my pull request merged
on top of mainline trunk I suspect it will not be a problem to
drop the patch or even whole pull request. In the latter case I'll
re-submit it for -rc4.

Sorry for the confusion.

Best regards,
Jacek Anaszewski

On 3/31/19 11:06 AM, Geert Uytterhoeven wrote:
> Hi Jacek,
> 
> On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 3/9/19 7:04 AM, Kangjie Lu wrote:
>>> In case of_match_device cannot find a match, the fixes returns
>>> -EINVAL to avoid NULL pointer dereference.
>>>
>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>> ---
>>>    drivers/leds/leds-pca9532.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>>> index 7fea18b0c15d..4b0335591728 100644
>>> --- a/drivers/leds/leds-pca9532.c
>>> +++ b/drivers/leds/leds-pca9532.c
>>> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
>>>        const struct i2c_device_id *id)
>>>    {
>>>        int devid;
>>> +     const struct of_device_id *of_id;
>>>        struct pca9532_data *data = i2c_get_clientdata(client);
>>>        struct pca9532_platform_data *pca9532_pdata =
>>>                        dev_get_platdata(&client->dev);
>>> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
>>>                        dev_err(&client->dev, "no platform data\n");
>>>                        return -EINVAL;
>>>                }
>>> -             devid = (int)(uintptr_t)of_match_device(
>>> -                     of_pca9532_leds_match, &client->dev)->data;
>>> +             of_id = of_match_device(of_pca9532_leds_match,
>>> +                             &client->dev);
>>> +             if (unlikely(!of_id))
> 
> Use of unlikey() is frowned upon.
> Moreover, this cannot happen, as pca9532_of_populate_pdata() already
> contains a similar check.
> 
> Kangjie: please stop submitting patches for missing checks, without
> investigating if the failures can actually happen. Thanks!
> 
>>> +                     return -EINVAL;
>>> +             devid = (int)of_id->data;
>>>        } else {
>>>                devid = id->driver_data;
>>>        }
>>
>>
>> Applied to the for-5.2 branch of linux-leds.git.
> 
> And also as a fix for v5.1...
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

[0] https://lkml.org/lkml/2019/3/30/222

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Jacek Anaszewski]

Hi Geert,

Thank you for the notification.

On 3/31/19 11:06 AM, Geert Uytterhoeven wrote:
> Hi Jacek,
> 
> On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 3/9/19 7:04 AM, Kangjie Lu wrote:
>>> In case of_match_device cannot find a match, the fixes returns
>>> -EINVAL to avoid NULL pointer dereference.
>>>
>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>> ---
>>>    drivers/leds/leds-pca9532.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>>> index 7fea18b0c15d..4b0335591728 100644
>>> --- a/drivers/leds/leds-pca9532.c
>>> +++ b/drivers/leds/leds-pca9532.c
>>> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
>>>        const struct i2c_device_id *id)
>>>    {
>>>        int devid;
>>> +     const struct of_device_id *of_id;
>>>        struct pca9532_data *data = i2c_get_clientdata(client);
>>>        struct pca9532_platform_data *pca9532_pdata =
>>>                        dev_get_platdata(&client->dev);
>>> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
>>>                        dev_err(&client->dev, "no platform data\n");
>>>                        return -EINVAL;
>>>                }
>>> -             devid = (int)(uintptr_t)of_match_device(
>>> -                     of_pca9532_leds_match, &client->dev)->data;
>>> +             of_id = of_match_device(of_pca9532_leds_match,
>>> +                             &client->dev);
>>> +             if (unlikely(!of_id))
> 
> Use of unlikey() is frowned upon.

What do you mean? Can you give some reference?

> Moreover, this cannot happen, as pca9532_of_populate_pdata() already
> contains a similar check.

Right, I assumed this fixes a real problem and didn't spent too much
time investigating the whole context.. Lesson for the future.

> Kangjie: please stop submitting patches for missing checks, without
> investigating if the failures can actually happen. Thanks!
> 
>>> +                     return -EINVAL;
>>> +             devid = (int)of_id->data;
>>>        } else {
>>>                devid = id->driver_data;
>>>        }
>>
>>
>> Applied to the for-5.2 branch of linux-leds.git.
> 
> And also as a fix for v5.1...

Yes, but it had been in linux-next for almost two weeks before that.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Geert Uytterhoeven]

Hi Jacek,

On Sun, Mar 31, 2019 at 1:01 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> On 3/31/19 11:06 AM, Geert Uytterhoeven wrote:
> On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski
> > <jacek.anaszewski@gmail.com> wrote:
> >> On 3/9/19 7:04 AM, Kangjie Lu wrote:
> >>> In case of_match_device cannot find a match, the fixes returns
> >>> -EINVAL to avoid NULL pointer dereference.
> >>>
> >>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> >>> ---
> >>>    drivers/leds/leds-pca9532.c | 8 ++++++--
> >>>    1 file changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> >>> index 7fea18b0c15d..4b0335591728 100644
> >>> --- a/drivers/leds/leds-pca9532.c
> >>> +++ b/drivers/leds/leds-pca9532.c
> >>> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
> >>>        const struct i2c_device_id *id)
> >>>    {
> >>>        int devid;
> >>> +     const struct of_device_id *of_id;
> >>>        struct pca9532_data *data = i2c_get_clientdata(client);
> >>>        struct pca9532_platform_data *pca9532_pdata =
> >>>                        dev_get_platdata(&client->dev);
> >>> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
> >>>                        dev_err(&client->dev, "no platform data\n");
> >>>                        return -EINVAL;
> >>>                }
> >>> -             devid = (int)(uintptr_t)of_match_device(
> >>> -                     of_pca9532_leds_match, &client->dev)->data;
> >>> +             of_id = of_match_device(of_pca9532_leds_match,
> >>> +                             &client->dev);
> >>> +             if (unlikely(!of_id))
> >
> > Use of unlikey() is frowned upon.
>
> What do you mean? Can you give some reference?

I have more memories of this being discussed, but I could find only
https://lwn.net/Articles/420019/

It may be useful for some heavily used core code, but not in most drivers
or uncritical code like probe paths, due to:
  - many people getting it wrong,
  - usually it doesn't make any difference at all.

> >> Applied to the for-5.2 branch of linux-leds.git.
> >
> > And also as a fix for v5.1...
>
> Yes, but it had been in linux-next for almost two weeks before that.

Sorry, I only noticed when it got upstream.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] leds: fix a potential NULL pointer dereference

[Jacek Anaszewski]

Hi Geert,

On 4/1/19 9:08 AM, Geert Uytterhoeven wrote:
> Hi Jacek,
> 
> On Sun, Mar 31, 2019 at 1:01 PM Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>> On 3/31/19 11:06 AM, Geert Uytterhoeven wrote:
>> On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski
>>> <jacek.anaszewski@gmail.com> wrote:
>>>> On 3/9/19 7:04 AM, Kangjie Lu wrote:
>>>>> In case of_match_device cannot find a match, the fixes returns
>>>>> -EINVAL to avoid NULL pointer dereference.
>>>>>
>>>>> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
>>>>> ---
>>>>>     drivers/leds/leds-pca9532.c | 8 ++++++--
>>>>>     1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>>>>> index 7fea18b0c15d..4b0335591728 100644
>>>>> --- a/drivers/leds/leds-pca9532.c
>>>>> +++ b/drivers/leds/leds-pca9532.c
>>>>> @@ -513,6 +513,7 @@ static int pca9532_probe(struct i2c_client *client,
>>>>>         const struct i2c_device_id *id)
>>>>>     {
>>>>>         int devid;
>>>>> +     const struct of_device_id *of_id;
>>>>>         struct pca9532_data *data = i2c_get_clientdata(client);
>>>>>         struct pca9532_platform_data *pca9532_pdata =
>>>>>                         dev_get_platdata(&client->dev);
>>>>> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client,
>>>>>                         dev_err(&client->dev, "no platform data\n");
>>>>>                         return -EINVAL;
>>>>>                 }
>>>>> -             devid = (int)(uintptr_t)of_match_device(
>>>>> -                     of_pca9532_leds_match, &client->dev)->data;
>>>>> +             of_id = of_match_device(of_pca9532_leds_match,
>>>>> +                             &client->dev);
>>>>> +             if (unlikely(!of_id))
>>>
>>> Use of unlikey() is frowned upon.
>>
>> What do you mean? Can you give some reference?
> 
> I have more memories of this being discussed, but I could find only
> https://lwn.net/Articles/420019/

Thanks!

> It may be useful for some heavily used core code, but not in most drivers
> or uncritical code like probe paths, due to:
>    - many people getting it wrong,
>    - usually it doesn't make any difference at all.
> 
>>>> Applied to the for-5.2 branch of linux-leds.git.
>>>
>>> And also as a fix for v5.1...
>>
>> Yes, but it had been in linux-next for almost two weeks before that.
> 
> Sorry, I only noticed when it got upstream.

No problem.

-- 
Best regards,
Jacek Anaszewski