Re: [PATCH] leds: fix a potential NULL pointer dereference
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 wrote: On 3/31/19 11:06 AM, Geert Uytterhoeven wrote: On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski 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 --- 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(>dev); @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, dev_err(>dev, "no platform data\n"); return -EINVAL; } - devid = (int)(uintptr_t)of_match_device( - of_pca9532_leds_match, >dev)->data; + of_id = of_match_device(of_pca9532_leds_match, + >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
Re: [PATCH] leds: fix a potential NULL pointer dereference
Hi Jacek, On Sun, Mar 31, 2019 at 1:01 PM Jacek Anaszewski wrote: > On 3/31/19 11:06 AM, Geert Uytterhoeven wrote: > On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski > > 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 > >>> --- > >>>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(>dev); > >>> @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, > >>>dev_err(>dev, "no platform data\n"); > >>>return -EINVAL; > >>>} > >>> - devid = (int)(uintptr_t)of_match_device( > >>> - of_pca9532_leds_match, >dev)->data; > >>> + of_id = of_match_device(of_pca9532_leds_match, > >>> + >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 -- ge...@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
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 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 --- 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(>dev); @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, dev_err(>dev, "no platform data\n"); return -EINVAL; } - devid = (int)(uintptr_t)of_match_device( - of_pca9532_leds_match, >dev)->data; + of_id = of_match_device(of_pca9532_leds_match, + >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
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 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 --- 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(>dev); @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, dev_err(>dev, "no platform data\n"); return -EINVAL; } - devid = (int)(uintptr_t)of_match_device( - of_pca9532_leds_match, >dev)->data; + of_id = of_match_device(of_pca9532_leds_match, + >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
Hi Jacek, On Sun, Mar 10, 2019 at 9:40 PM Jacek Anaszewski 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 > > --- > > 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(>dev); > > @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, > > dev_err(>dev, "no platform data\n"); > > return -EINVAL; > > } > > - devid = (int)(uintptr_t)of_match_device( > > - of_pca9532_leds_match, >dev)->data; > > + of_id = of_match_device(of_pca9532_leds_match, > > + >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 -- ge...@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
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 >> --- >> 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 i...@metux.net -- +49-151-27565287
Re: [PATCH] leds: fix a potential NULL pointer dereference
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 --- 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(>dev); @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, dev_err(>dev, "no platform data\n"); return -EINVAL; } - devid = (int)(uintptr_t)of_match_device( - of_pca9532_leds_match, >dev)->data; + of_id = of_match_device(of_pca9532_leds_match, + >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
[PATCH] leds: fix a potential NULL pointer dereference
In case of_match_device cannot find a match, the fixes returns -EINVAL to avoid NULL pointer dereference. Signed-off-by: Kangjie Lu --- 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(>dev); @@ -528,8 +529,11 @@ static int pca9532_probe(struct i2c_client *client, dev_err(>dev, "no platform data\n"); return -EINVAL; } - devid = (int)(uintptr_t)of_match_device( - of_pca9532_leds_match, >dev)->data; + of_id = of_match_device(of_pca9532_leds_match, + >dev); + if (unlikely(!of_id)) + return -EINVAL; + devid = (int)of_id->data; } else { devid = id->driver_data; } -- 2.17.1