| Message ID | 1353316830-23755-1-git-send-email-sachin.kamat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Inki, Thanks for your review. My comments inline. On 19 November 2012 15:14, Inki Dae <inki.dae@samsung.com> wrote: > > >> -----Original Message----- >> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] >> Sent: Monday, November 19, 2012 6:21 PM >> To: dri-devel@lists.freedesktop.org >> Cc: inki.dae@samsung.com; jy0922.shim@samsung.com; > sachin.kamat@linaro.org; >> patches@linaro.org >> Subject: [PATCH 1/1] drm/exynos: Fix potential NULL pointer dereference in >> exynos_drm_encoder.c >> >> Check overlay_ops is not NULL as checked in the previous 'if' condition. >> Fixes the following smatch error: >> drivers/gpu/drm/exynos/exynos_drm_encoder.c:509 >> exynos_drm_encoder_plane_disable() >> error: we previously assumed 'overlay_ops' could be null (see line 499) >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> --- >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> index e51503f..a44238e 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> @@ -506,6 +506,6 @@ void exynos_drm_encoder_plane_disable(struct >> drm_encoder *encoder, void *data) >> * because the setting for disabling the overlay will be updated >> * at vsync. >> */ >> - if (overlay_ops->wait_for_vblank) >> + if (overlay_ops && overlay_ops->wait_for_vblank) >> overlay_ops->wait_for_vblank(manager->dev); > > This code will be removed at -next. Since this code is already in mainline, I think this patch should be applied as a fix during this rc (for completeness). You may subsequently delete it in the next release as per your plan. > > Thanks, > Inki Dae > >> } >> -- >> 1.7.4.1 >
On 19 November 2012 15:30, Inki Dae <inki.dae@samsung.com> wrote: > > >> -----Original Message----- >> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] >> Sent: Monday, November 19, 2012 6:56 PM >> To: Inki Dae >> Cc: dri-devel@lists.freedesktop.org; jy0922.shim@samsung.com; >> patches@linaro.org >> Subject: Re: [PATCH 1/1] drm/exynos: Fix potential NULL pointer >> dereference in exynos_drm_encoder.c >> >> Hi Inki, >> >> Thanks for your review. My comments inline. >> >> On 19 November 2012 15:14, Inki Dae <inki.dae@samsung.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] >> >> Sent: Monday, November 19, 2012 6:21 PM >> >> To: dri-devel@lists.freedesktop.org >> >> Cc: inki.dae@samsung.com; jy0922.shim@samsung.com; >> > sachin.kamat@linaro.org; >> >> patches@linaro.org >> >> Subject: [PATCH 1/1] drm/exynos: Fix potential NULL pointer dereference >> in >> >> exynos_drm_encoder.c >> >> >> >> Check overlay_ops is not NULL as checked in the previous 'if' > condition. >> >> Fixes the following smatch error: >> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c:509 >> >> exynos_drm_encoder_plane_disable() >> >> error: we previously assumed 'overlay_ops' could be null (see line 499) >> >> >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> >> --- >> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +- >> >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> index e51503f..a44238e 100644 >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >> >> @@ -506,6 +506,6 @@ void exynos_drm_encoder_plane_disable(struct >> >> drm_encoder *encoder, void *data) >> >> * because the setting for disabling the overlay will be updated >> >> * at vsync. >> >> */ >> >> - if (overlay_ops->wait_for_vblank) >> >> + if (overlay_ops && overlay_ops->wait_for_vblank) >> >> overlay_ops->wait_for_vblank(manager->dev); >> > >> > This code will be removed at -next. >> >> Since this code is already in mainline, I think this patch should be >> applied as a fix during this rc (for completeness). >> You may subsequently delete it in the next release as per your plan. >> > > And NULL pointer checking was already done above like below, > if (overlay_ops && overlay_ops->disable) > overlay_ops->disable(manager->dev, zpos); Correct. But that check is applicable only for that one statement (overlay_ops->disable(manager->dev, zpos);). Similar check needs to be added to below 'if' code too. > > This is your missing point. > >> > >> > Thanks, >> > Inki Dae >> > >> >> } >> >> -- >> >> 1.7.4.1 >> > >> >> >> >> -- >> With warm regards, >> Sachin >
Hi Inki, On 19 November 2012 15:32, Sachin Kamat <sachin.kamat@linaro.org> wrote: > On 19 November 2012 15:30, Inki Dae <inki.dae@samsung.com> wrote: >> >> >>> -----Original Message----- >>> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] >>> Sent: Monday, November 19, 2012 6:56 PM >>> To: Inki Dae >>> Cc: dri-devel@lists.freedesktop.org; jy0922.shim@samsung.com; >>> patches@linaro.org >>> Subject: Re: [PATCH 1/1] drm/exynos: Fix potential NULL pointer >>> dereference in exynos_drm_encoder.c >>> >>> Hi Inki, >>> >>> Thanks for your review. My comments inline. >>> >>> On 19 November 2012 15:14, Inki Dae <inki.dae@samsung.com> wrote: >>> > >>> > >>> >> -----Original Message----- >>> >> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] >>> >> Sent: Monday, November 19, 2012 6:21 PM >>> >> To: dri-devel@lists.freedesktop.org >>> >> Cc: inki.dae@samsung.com; jy0922.shim@samsung.com; >>> > sachin.kamat@linaro.org; >>> >> patches@linaro.org >>> >> Subject: [PATCH 1/1] drm/exynos: Fix potential NULL pointer dereference >>> in >>> >> exynos_drm_encoder.c >>> >> >>> >> Check overlay_ops is not NULL as checked in the previous 'if' >> condition. >>> >> Fixes the following smatch error: >>> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c:509 >>> >> exynos_drm_encoder_plane_disable() >>> >> error: we previously assumed 'overlay_ops' could be null (see line 499) >>> >> >>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >>> >> --- >>> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +- >>> >> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >> >>> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >>> >> b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >>> >> index e51503f..a44238e 100644 >>> >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c >>> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c >>> >> @@ -506,6 +506,6 @@ void exynos_drm_encoder_plane_disable(struct >>> >> drm_encoder *encoder, void *data) >>> >> * because the setting for disabling the overlay will be updated >>> >> * at vsync. >>> >> */ >>> >> - if (overlay_ops->wait_for_vblank) >>> >> + if (overlay_ops && overlay_ops->wait_for_vblank) >>> >> overlay_ops->wait_for_vblank(manager->dev); >>> > >>> > This code will be removed at -next. >>> >>> Since this code is already in mainline, I think this patch should be >>> applied as a fix during this rc (for completeness). >>> You may subsequently delete it in the next release as per your plan. >>> >> >> And NULL pointer checking was already done above like below, >> if (overlay_ops && overlay_ops->disable) >> overlay_ops->disable(manager->dev, zpos); > Correct. But that check is applicable only for that one statement > (overlay_ops->disable(manager->dev, zpos);). > > Similar check needs to be added to below 'if' code too. What are your comments about this? > >> >> This is your missing point. >> >>> > >>> > Thanks, >>> > Inki Dae >>> > >>> >> } >>> >> -- >>> >> 1.7.4.1 >>> > >>> >>> >>> >>> -- >>> With warm regards, >>> Sachin >> > > > > -- > With warm regards, > Sachin
> -----Original Message----- > From: Sachin Kamat [mailto:sachin.kamat@linaro.org] > Sent: Thursday, November 22, 2012 3:13 PM > To: Inki Dae > Cc: dri-devel@lists.freedesktop.org; jy0922.shim@samsung.com; > patches@linaro.org > Subject: Re: [PATCH 1/1] drm/exynos: Fix potential NULL pointer > dereference in exynos_drm_encoder.c > > Hi Inki, > > On 19 November 2012 15:32, Sachin Kamat <sachin.kamat@linaro.org> wrote: > > On 19 November 2012 15:30, Inki Dae <inki.dae@samsung.com> wrote: > >> > >> > >>> -----Original Message----- > >>> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] > >>> Sent: Monday, November 19, 2012 6:56 PM > >>> To: Inki Dae > >>> Cc: dri-devel@lists.freedesktop.org; jy0922.shim@samsung.com; > >>> patches@linaro.org > >>> Subject: Re: [PATCH 1/1] drm/exynos: Fix potential NULL pointer > >>> dereference in exynos_drm_encoder.c > >>> > >>> Hi Inki, > >>> > >>> Thanks for your review. My comments inline. > >>> > >>> On 19 November 2012 15:14, Inki Dae <inki.dae@samsung.com> wrote: > >>> > > >>> > > >>> >> -----Original Message----- > >>> >> From: Sachin Kamat [mailto:sachin.kamat@linaro.org] > >>> >> Sent: Monday, November 19, 2012 6:21 PM > >>> >> To: dri-devel@lists.freedesktop.org > >>> >> Cc: inki.dae@samsung.com; jy0922.shim@samsung.com; > >>> > sachin.kamat@linaro.org; > >>> >> patches@linaro.org > >>> >> Subject: [PATCH 1/1] drm/exynos: Fix potential NULL pointer > dereference > >>> in > >>> >> exynos_drm_encoder.c > >>> >> > >>> >> Check overlay_ops is not NULL as checked in the previous 'if' > >> condition. > >>> >> Fixes the following smatch error: > >>> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c:509 > >>> >> exynos_drm_encoder_plane_disable() > >>> >> error: we previously assumed 'overlay_ops' could be null (see line > 499) > >>> >> > >>> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > >>> >> --- > >>> >> drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +- > >>> >> 1 files changed, 1 insertions(+), 1 deletions(-) > >>> >> > >>> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >>> >> b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >>> >> index e51503f..a44238e 100644 > >>> >> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >>> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c > >>> >> @@ -506,6 +506,6 @@ void exynos_drm_encoder_plane_disable(struct > >>> >> drm_encoder *encoder, void *data) > >>> >> * because the setting for disabling the overlay will be > updated > >>> >> * at vsync. > >>> >> */ > >>> >> - if (overlay_ops->wait_for_vblank) > >>> >> + if (overlay_ops && overlay_ops->wait_for_vblank) > >>> >> overlay_ops->wait_for_vblank(manager->dev); > >>> > > >>> > This code will be removed at -next. > >>> > >>> Since this code is already in mainline, I think this patch should be > >>> applied as a fix during this rc (for completeness). > >>> You may subsequently delete it in the next release as per your plan. > >>> > >> > >> And NULL pointer checking was already done above like below, > >> if (overlay_ops && overlay_ops->disable) > >> overlay_ops->disable(manager->dev, zpos); > > Correct. But that check is applicable only for that one statement > > (overlay_ops->disable(manager->dev, zpos);). > > > > Similar check needs to be added to below 'if' code too. > > What are your comments about this? > Left condition first is checked so as I mentioned before, it doesn't need overlay_ops checking because that was checked already. why do you think overlay_ops should be checked again? > > > >> > >> This is your missing point. > >> > >>> > > >>> > Thanks, > >>> > Inki Dae > >>> > > >>> >> } > >>> >> -- > >>> >> 1.7.4.1 > >>> > > >>> > >>> > >>> > >>> -- > >>> With warm regards, > >>> Sachin > >> > > > > > > > > -- > > With warm regards, > > Sachin > > > > -- > With warm regards, > Sachin
[snip] >> >> And NULL pointer checking was already done above like below, >> >> if (overlay_ops && overlay_ops->disable) >> >> overlay_ops->disable(manager->dev, zpos); >> > Correct. But that check is applicable only for that one statement >> > (overlay_ops->disable(manager->dev, zpos);). >> > >> > Similar check needs to be added to below 'if' code too. >> >> What are your comments about this? >> > > Left condition first is checked so as I mentioned before, it doesn't need > overlay_ops checking because that was checked already. why do you think > overlay_ops should be checked again? > Consider the case when overlay_ops is NULL. if (overlay_ops && overlay_ops->disable) overlay_ops->disable(manager->dev, zpos); It does not enter this condition as overlay_ops is NULL and moves to the next statement, if (overlay_ops->wait_for_vblank) where it gets dereferenced. Please note we are not returning back from the first condition if overlay_ops is NULL. Hence we need to check the condition in second case too.
> -----Original Message----- > From: Sachin Kamat [mailto:sachin.kamat@linaro.org] > Sent: Thursday, November 22, 2012 5:19 PM > To: Inki Dae > Cc: dri-devel@lists.freedesktop.org; jy0922.shim@samsung.com; > patches@linaro.org > Subject: Re: [PATCH 1/1] drm/exynos: Fix potential NULL pointer > dereference in exynos_drm_encoder.c > > [snip] > >> >> And NULL pointer checking was already done above like below, > >> >> if (overlay_ops && overlay_ops->disable) > >> >> overlay_ops->disable(manager->dev, zpos); > >> > Correct. But that check is applicable only for that one statement > >> > (overlay_ops->disable(manager->dev, zpos);). > >> > > >> > Similar check needs to be added to below 'if' code too. > >> > >> What are your comments about this? > >> > > > > Left condition first is checked so as I mentioned before, it doesn't > need > > overlay_ops checking because that was checked already. why do you think > > overlay_ops should be checked again? > > > > Consider the case when overlay_ops is NULL. > > if (overlay_ops && overlay_ops->disable) > overlay_ops->disable(manager->dev, zpos); > > It does not enter this condition as overlay_ops is NULL and moves to > the next statement, > if (overlay_ops->wait_for_vblank) where it gets dereferenced. > > Please note we are not returning back from the first condition if > overlay_ops is NULL. > Hence we need to check the condition in second case too. > Ah~ Right. I didn't check it surely. :) Thanks, Inki Dae > -- > With warm regards, > Sachin
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c index e51503f..a44238e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c @@ -506,6 +506,6 @@ void exynos_drm_encoder_plane_disable(struct drm_encoder *encoder, void *data) * because the setting for disabling the overlay will be updated * at vsync. */ - if (overlay_ops->wait_for_vblank) + if (overlay_ops && overlay_ops->wait_for_vblank) overlay_ops->wait_for_vblank(manager->dev); }
Check overlay_ops is not NULL as checked in the previous 'if' condition. Fixes the following smatch error: drivers/gpu/drm/exynos/exynos_drm_encoder.c:509 exynos_drm_encoder_plane_disable() error: we previously assumed 'overlay_ops' could be null (see line 499) Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> --- drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)