* [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash @ 2021-02-01 14:41 Wenjia Zhao 2021-02-02 8:28 ` Lee Jones 2021-02-02 9:25 ` Daniel Thompson 0 siblings, 2 replies; 5+ messages in thread From: Wenjia Zhao @ 2021-02-01 14:41 UTC (permalink / raw) Cc: Daniel Thompson, driverfuzzing, Bartlomiej Zolnierkiewicz, Jingoo Han, linux-fbdev, dri-devel, linux-kernel, Lee Jones Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> --- drivers/video/backlight/pcf50633-backlight.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/video/backlight/pcf50633-backlight.c b/drivers/video/backlight/pcf50633-backlight.c index 540dd338..43267af 100644 --- a/drivers/video/backlight/pcf50633-backlight.c +++ b/drivers/video/backlight/pcf50633-backlight.c @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pcf_bl); - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); + if (pdata) + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); /* * Should be different from bl_props.brightness, so we do not exit -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash 2021-02-01 14:41 [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash Wenjia Zhao @ 2021-02-02 8:28 ` Lee Jones 2021-02-02 9:25 ` Daniel Thompson 1 sibling, 0 replies; 5+ messages in thread From: Lee Jones @ 2021-02-02 8:28 UTC (permalink / raw) To: Wenjia Zhao Cc: Daniel Thompson, Bartlomiej Zolnierkiewicz, Jingoo Han, linux-kernel, dri-devel, linux-fbdev On Mon, 01 Feb 2021, Wenjia Zhao wrote: Please provide a suitable commit messages. Describe the problem. Describe the issue was found. Describe the solution. > Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> > --- > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/pcf50633-backlight.c b/drivers/video/backlight/pcf50633-backlight.c > index 540dd338..43267af 100644 > --- a/drivers/video/backlight/pcf50633-backlight.c > +++ b/drivers/video/backlight/pcf50633-backlight.c > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pcf_bl); > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); > + if (pdata) > + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); A tab should be 8 chars in Linux. > /* > * Should be different from bl_props.brightness, so we do not exit -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash 2021-02-01 14:41 [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash Wenjia Zhao 2021-02-02 8:28 ` Lee Jones @ 2021-02-02 9:25 ` Daniel Thompson 2021-02-02 21:25 ` FUZZ DR 1 sibling, 1 reply; 5+ messages in thread From: Daniel Thompson @ 2021-02-02 9:25 UTC (permalink / raw) To: Wenjia Zhao Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Jingoo Han, linux-kernel, dri-devel, Lee Jones On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote: > Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> There should be a patch description here explaining why the patch is needed and how it works. > --- > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/pcf50633-backlight.c b/drivers/video/backlight/pcf50633-backlight.c > index 540dd338..43267af 100644 > --- a/drivers/video/backlight/pcf50633-backlight.c > +++ b/drivers/video/backlight/pcf50633-backlight.c > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, pcf_bl); > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); > + if (pdata) > + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); Assuming you found this issue using a static analyzer then I think it might be better to if an "if (!pdata) return -EINVAL" further up the file instead. In other words it is better to "document" (via the return code) that the code does not support pdata == NULL than to add another untested code path. Daniel. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash 2021-02-02 9:25 ` Daniel Thompson @ 2021-02-02 21:25 ` FUZZ DR 2021-02-03 11:42 ` Daniel Thompson 0 siblings, 1 reply; 5+ messages in thread From: FUZZ DR @ 2021-02-02 21:25 UTC (permalink / raw) To: Daniel Thompson Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Jingoo Han, linux-kernel, dri-devel, Lee Jones [-- Attachment #1.1: Type: text/plain, Size: 2785 bytes --] Sorry about the missing description, I have a description at my local commit. But the commit description disappeared when I used git send-email to submit this patch. backlight: pcf50633: pdata may be a null pointer, null pointer dereference causes crash pdata has been checked at line 120 before dereference. However, it is used without check at line 130. So just add the check, Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> --- drivers/video/backlight/pcf50633-backlight.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/video/backlight/pcf50633-backlight.c b/drivers/video/backlight/pcf50633-backlight.c index 540dd338..43267af 100644 --- a/drivers/video/backlight/pcf50633-backlight.c +++ b/drivers/video/backlight/pcf50633-backlight.c @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pcf_bl); - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); + if (pdata) + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, pdata->ramp_time); /* * Should be different from bl_props.brightness, so we do not exit -- It is better to write a default value to the register if the ramp_time has a default value. Then it does not need to return -EINVAL. It will keep consistent with the behavior at line 120. Daniel Thompson <daniel.thompson@linaro.org> 于2021年2月2日周二 上午3:25写道： > On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote: > > Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> > > There should be a patch description here explaining why the patch > is needed and how it works. > > > > --- > > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/video/backlight/pcf50633-backlight.c > b/drivers/video/backlight/pcf50633-backlight.c > > index 540dd338..43267af 100644 > > --- a/drivers/video/backlight/pcf50633-backlight.c > > +++ b/drivers/video/backlight/pcf50633-backlight.c > > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device > *pdev) > > > > platform_set_drvdata(pdev, pcf_bl); > > > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > pdata->ramp_time); > > + if (pdata) > > + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > pdata->ramp_time); > > Assuming you found this issue using a static analyzer then I think it > might be better to if an "if (!pdata) return -EINVAL" further up the > file instead. > > In other words it is better to "document" (via the return code) that the > code does not support pdata == NULL than to add another untested code > path. > > > Daniel. > [-- Attachment #1.2: Type: text/html, Size: 3551 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash 2021-02-02 21:25 ` FUZZ DR @ 2021-02-03 11:42 ` Daniel Thompson 0 siblings, 0 replies; 5+ messages in thread From: Daniel Thompson @ 2021-02-03 11:42 UTC (permalink / raw) To: FUZZ DR Cc: linux-fbdev, Bartlomiej Zolnierkiewicz, Jingoo Han, linux-kernel, dri-devel, Lee Jones On Tue, Feb 02, 2021 at 03:25:49PM -0600, FUZZ DR wrote: > Sorry about the missing description, I have a description at my local > commit. But the commit description disappeared when I used git send-email > to submit this patch. > > backlight: pcf50633: pdata may be a null pointer, null pointer dereference > causes crash > pdata has been checked at line 120 before dereference. However, it is used > without check at line 130. So just add the check, To be clear what your analyzer is reporting is a mismatch of programmer intent. In line 120 suggests a belief that pdata could be NULL whilst line 130 suggests a belief that pdata is never NULL. Your analyzer cannot not tell you which belief is incorrect, only that there is a mismatch. > It is better to write a default value to the register if the ramp_time has > a default value. Then it does not need to return -EINVAL. It will keep > consistent with the behavior at line 120. I disagree. You have assumed that line 120 is correct and that this code needs to support the case where pdata is NULL. However eleven years of history disprove this! This code is never called without valid platform data. Therefore we should fix the assumption on line 120 by making it clear that this code code expects non-NULL platform data. Given there are developers working to eliminate this kind of platform data structure (by adopting device properties instead) I don't want to make their life harder by implementing unused and untested special cases that they would have to reason about. Daniel. > > Daniel Thompson <daniel.thompson@linaro.org> 于2021年2月2日周二 上午3:25写道： > > > On Mon, Feb 01, 2021 at 08:41:38AM -0600, Wenjia Zhao wrote: > > > Signed-off-by: Wenjia Zhao <driverfuzzing@gmail.com> > > > > There should be a patch description here explaining why the patch > > is needed and how it works. > > > > > > > --- > > > drivers/video/backlight/pcf50633-backlight.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/video/backlight/pcf50633-backlight.c > > b/drivers/video/backlight/pcf50633-backlight.c > > > index 540dd338..43267af 100644 > > > --- a/drivers/video/backlight/pcf50633-backlight.c > > > +++ b/drivers/video/backlight/pcf50633-backlight.c > > > @@ -127,7 +127,8 @@ static int pcf50633_bl_probe(struct platform_device > > *pdev) > > > > > > platform_set_drvdata(pdev, pcf_bl); > > > > > > - pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > > pdata->ramp_time); > > > + if (pdata) > > > + pcf50633_reg_write(pcf_bl->pcf, PCF50633_REG_LEDDIM, > > pdata->ramp_time); > > > > Assuming you found this issue using a static analyzer then I think it > > might be better to if an "if (!pdata) return -EINVAL" further up the > > file instead. > > > > In other words it is better to "document" (via the return code) that the > > code does not support pdata == NULL than to add another untested code > > path. > > > > > > Daniel. > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-03 11:42 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-01 14:41 [PATCH] backlight: pcf50633: pdata may be a null pointer, null pointer dereference cause crash Wenjia Zhao 2021-02-02 8:28 ` Lee Jones 2021-02-02 9:25 ` Daniel Thompson 2021-02-02 21:25 ` FUZZ DR 2021-02-03 11:42 ` Daniel Thompson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).