diff mbox series

[tiL4.19-AD,1/2] drm/tilcdc: Check drm_fb_cma_get_gem_obj() return value

Message ID 1b3ce6819beec9779468d1b661889d5b2dfa0c3e.1551352503.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show
Series drm/tilcdc: a cleanup and a fix | expand

Commit Message

Jyri Sarha Feb. 28, 2019, 11:18 a.m. UTC
drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
checked before dereferencing the returned pointer.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Laurent Pinchart March 5, 2019, 3:43 p.m. UTC | #1
Hi Jyri,

Thank you for the patch.

On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
> checked before dereferencing the returned pointer.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 1067e702c22c..a8ae064f52bb 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  	u64 dma_base_and_ceiling;
>  
>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
> +	if (WARN_ON(!gem))
> +		return;

But this should not happen, right ? Don't we have the required checks in
place to ensure there will always be a valid GEM object available here ?

>  	start = gem->paddr + fb->offsets[0] +
>  		crtc->y * fb->pitches[0] +
Jyri Sarha March 5, 2019, 7:42 p.m. UTC | #2
On 05/03/2019 17:43, Laurent Pinchart wrote:
> Hi Jyri,
> 
> Thank you for the patch.
> 
> On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
>> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
>> checked before dereferencing the returned pointer.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 1067e702c22c..a8ae064f52bb 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>>  	u64 dma_base_and_ceiling;
>>  
>>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
>> +	if (WARN_ON(!gem))
>> +		return;
> 
> But this should not happen, right ? Don't we have the required checks in
> place to ensure there will always be a valid GEM object available here ?
> 

The patch is trying to make clockwork - a static analysis tool - happy.
Should have mentioned that in the patch. But the fact that static
clockwork complains about this suggests that either there is no such
check elsewhere, or clockwork does not understand it.

>>  	start = gem->paddr + fb->offsets[0] +
>>  		crtc->y * fb->pitches[0] +
> 

Best regards,
Jyri
Laurent Pinchart March 5, 2019, 8:02 p.m. UTC | #3
On Tue, Mar 05, 2019 at 09:42:50PM +0200, Jyri Sarha wrote:
> On 05/03/2019 17:43, Laurent Pinchart wrote:
> > Hi Jyri,
> > 
> > Thank you for the patch.
> > 
> > On Thu, Feb 28, 2019 at 01:18:50PM +0200, Jyri Sarha wrote:
> >> drm_fb_cma_get_gem_obj() may return NULL. The return value needs to be
> >> checked before dereferencing the returned pointer.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >> ---
> >>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> index 1067e702c22c..a8ae064f52bb 100644
> >> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> >> @@ -75,6 +75,8 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
> >>  	u64 dma_base_and_ceiling;
> >>  
> >>  	gem = drm_fb_cma_get_gem_obj(fb, 0);
> >> +	if (WARN_ON(!gem))
> >> +		return;
> > 
> > But this should not happen, right ? Don't we have the required checks in
> > place to ensure there will always be a valid GEM object available here ?
> > 
> 
> The patch is trying to make clockwork - a static analysis tool - happy.
> Should have mentioned that in the patch. But the fact that static
> clockwork complains about this suggests that either there is no such
> check elsewhere, or clockwork does not understand it.

I'd go for the latter. drm_fb_cma_get_gem_obj() will return NULL if the
plane index is >= 4 (which is clearly not the case here), and will
otherwise return the GEM object pointer from an internal array. That
array is populated when the framebuffer is allocated created, by
drm_gem_fb_create() called from tilcdc_fb_create() in this case. The
check here is not needed, but clockwork can't easily know about that.

> >>  	start = gem->paddr + fb->offsets[0] +
> >>  		crtc->y * fb->pitches[0] +
diff mbox series

Patch

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 1067e702c22c..a8ae064f52bb 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -75,6 +75,8 @@  static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
 	u64 dma_base_and_ceiling;
 
 	gem = drm_fb_cma_get_gem_obj(fb, 0);
+	if (WARN_ON(!gem))
+		return;
 
 	start = gem->paddr + fb->offsets[0] +
 		crtc->y * fb->pitches[0] +