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

[Inki Dae]

> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat at linaro.org]
> Sent: Thursday, November 22, 2012 3:13 PM
> To: Inki Dae
> Cc: dri-devel at lists.freedesktop.org; jy0922.shim at samsung.com;
> patches at 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 at linaro.org> wrote:
> > On 19 November 2012 15:30, Inki Dae <inki.dae at samsung.com> wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: Sachin Kamat [mailto:sachin.kamat at linaro.org]
> >>> Sent: Monday, November 19, 2012 6:56 PM
> >>> To: Inki Dae
> >>> Cc: dri-devel at lists.freedesktop.org; jy0922.shim at samsung.com;
> >>> patches at 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 at samsung.com> wrote:
> >>> >
> >>> >
> >>> >> -----Original Message-----
> >>> >> From: Sachin Kamat [mailto:sachin.kamat at linaro.org]
> >>> >> Sent: Monday, November 19, 2012 6:21 PM
> >>> >> To: dri-devel at lists.freedesktop.org
> >>> >> Cc: inki.dae at samsung.com; jy0922.shim at samsung.com;
> >>> > sachin.kamat at linaro.org;
> >>> >> patches at 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 at 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