diff mbox

[1/1] drm/exynos: Fix potential NULL pointer dereference in exynos_drm_encoder.c

Message ID 1353316830-23755-1-git-send-email-sachin.kamat@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sachin Kamat Nov. 19, 2012, 9:20 a.m. UTC
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(-)

Comments

Sachin Kamat Nov. 19, 2012, 9:55 a.m. UTC | #1
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
>
Sachin Kamat Nov. 19, 2012, 10:02 a.m. UTC | #2
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
>
Sachin Kamat Nov. 22, 2012, 6:13 a.m. UTC | #3
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
Inki Dae Nov. 22, 2012, 8:11 a.m. UTC | #4
> -----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
Sachin Kamat Nov. 22, 2012, 8:18 a.m. UTC | #5
[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.
Inki Dae Nov. 22, 2012, 8:26 a.m. UTC | #6
> -----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 mbox

Patch

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);
 }