Re: [PATCH v2 4/4] drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Joe Moriarty]

On 2/19/2018 8:32 AM, Daniel Vetter wrote:

On Mon, Feb 12, 2018 at 02:51:44PM -0500, Joe Moriarty wrote:

The Parfait (version 2.1.0) static code analysis tool found the
following NULL pointer derefernce problem.

- drivers/gpu/drm/drm_vblank.c
Null pointer checks were added to return values from calls to
drm_crtc_from_index().  There is a possibility, however minute, that
crtc->index may not be found when trying to find the struct crtc
from it's assigned index given in drm_crtc_init_with_planes().
3 return checks for NULL where added.

Signed-off-by: Joe Moriarty 
Reviewed-by: Steven Sistare 


These are all drivers bugs, we'd need at least a WARN_ON when the crtc
doesn't exist. Otherwise this would just silently paper over a fairly
serious kernel bug (which doesn't improve things really).

Something like

if (WARN_ON(!crtc))
return NULL;

is what I'd go with.

-Daniel

I will make the requested changes and resubmit the patch.  Thanks for 
reviewing the patches.


Joe


---
  drivers/gpu/drm/drm_vblank.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..a3a1bce87468 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -120,7 +120,7 @@ static u32 __get_vblank_counter(struct drm_device *dev, 
unsigned int pipe)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
  
-		if (crtc->funcs->get_vblank_counter)

+   if (crtc && crtc->funcs->get_vblank_counter)
return crtc->funcs->get_vblank_counter(crtc);
}
  
@@ -318,7 +318,7 @@ static void __disable_vblank(struct drm_device *dev, unsigned int pipe)

if (drm_core_check_feature(dev, DRIVER_MODESET)) {
struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
  
-		if (crtc->funcs->disable_vblank) {

+   if (crtc && crtc->funcs->disable_vblank) {
crtc->funcs->disable_vblank(crtc);
return;
}
@@ -918,7 +918,7 @@ static int __enable_vblank(struct drm_device *dev, unsigned 
int pipe)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
  
-		if (crtc->funcs->enable_vblank)

+   if (crtc && crtc->funcs->enable_vblank)
return crtc->funcs->enable_vblank(crtc);
}
  
--

2.15.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Daniel Vetter]

On Mon, Feb 12, 2018 at 02:51:44PM -0500, Joe Moriarty wrote:
> The Parfait (version 2.1.0) static code analysis tool found the
> following NULL pointer derefernce problem.
> 
> - drivers/gpu/drm/drm_vblank.c
> Null pointer checks were added to return values from calls to
> drm_crtc_from_index().  There is a possibility, however minute, that
> crtc->index may not be found when trying to find the struct crtc
> from it's assigned index given in drm_crtc_init_with_planes().
> 3 return checks for NULL where added.
> 
> Signed-off-by: Joe Moriarty 
> Reviewed-by: Steven Sistare 

These are all drivers bugs, we'd need at least a WARN_ON when the crtc
doesn't exist. Otherwise this would just silently paper over a fairly
serious kernel bug (which doesn't improve things really).

Something like

if (WARN_ON(!crtc))
return NULL;

is what I'd go with.

-Daniel

> ---
>  drivers/gpu/drm/drm_vblank.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 32d9bcf5be7f..a3a1bce87468 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -120,7 +120,7 @@ static u32 __get_vblank_counter(struct drm_device *dev, 
> unsigned int pipe)
>   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  
> - if (crtc->funcs->get_vblank_counter)
> + if (crtc && crtc->funcs->get_vblank_counter)
>   return crtc->funcs->get_vblank_counter(crtc);
>   }
>  
> @@ -318,7 +318,7 @@ static void __disable_vblank(struct drm_device *dev, 
> unsigned int pipe)
>   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  
> - if (crtc->funcs->disable_vblank) {
> + if (crtc && crtc->funcs->disable_vblank) {
>   crtc->funcs->disable_vblank(crtc);
>   return;
>   }
> @@ -918,7 +918,7 @@ static int __enable_vblank(struct drm_device *dev, 
> unsigned int pipe)
>   if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  
> - if (crtc->funcs->enable_vblank)
> + if (crtc && crtc->funcs->enable_vblank)
>   return crtc->funcs->enable_vblank(crtc);
>   }
>  
> -- 
> 2.15.0
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel