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

2018-02-20 Thread Joe Moriarty 

On 2/19/2018 6:57 AM, Daniel Vetter wrote:

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

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

- drivers/gpu/drm/drm_drv.c
Any calls to drm_minor_get_slot() could result in the return of a NULL
pointer when an invalid DRM device type is encountered.  2 helper
functions where added for pointer manipulation (drm_minor_get_slot()
and drm_minor_set_minor()) along with checks for valid pointers for
struct drm_device variables throughout this module.

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


We do not ask for an invalid minor (userspace can't do that, it would be a
kernel bug). BUG_ON for the invalid case instead of all these changes
acceptable to shut up your checker?
-Daniel


Daniel,

I did the following and the static checker liked it:

default:
-   return NULL;
+   BUG();
}

I will make the change in the patch and resubmit.

Joe


---
  drivers/gpu/drm/drm_drv.c | 38 ++
  1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 9acc1e157813..dee6a4470e2c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -99,10 +99,36 @@ static struct drm_minor **drm_minor_get_slot(struct 
drm_device *dev,
case DRM_MINOR_CONTROL:
return >control;
default:
+   DRM_ERROR("Error in %s: Invalid dev, type = %d\n",
+ __func__, type);
return NULL;
}
  }
  
+static inline int drm_minor_set_minor(struct drm_device *dev,

+ unsigned int type,
+ struct drm_minor *minor)
+{
+   struct drm_minor **slot = drm_minor_get_slot(dev, type);
+   int retval = -ENODEV;
+
+   if (slot) {
+   retval = 0;
+   *slot = minor;
+   }
+   return retval;
+}
+
+static inline struct drm_minor *drm_minor_get_minor(struct drm_device *dev,
+   unsigned int type)
+{
+   struct drm_minor **slot = drm_minor_get_slot(dev, type);
+
+   if (slot)
+   return *slot;
+   return NULL;
+}
+
  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
  {
struct drm_minor *minor;
@@ -137,8 +163,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned 
int type)
goto err_index;
}
  
-	*drm_minor_get_slot(dev, type) = minor;

-   return 0;
+   r = drm_minor_set_minor(dev, type, minor);
+   if (r == 0)
+   return r;
  
  err_index:

spin_lock_irqsave(_minor_lock, flags);
@@ -155,6 +182,9 @@ static void drm_minor_free(struct drm_device *dev, unsigned 
int type)
unsigned long flags;
  
  	slot = drm_minor_get_slot(dev, type);

+   if (!slot)
+   return;
+
minor = *slot;
if (!minor)
return;
@@ -177,7 +207,7 @@ static int drm_minor_register(struct drm_device *dev, 
unsigned int type)
  
  	DRM_DEBUG("\n");
  
-	minor = *drm_minor_get_slot(dev, type);

+   minor = drm_minor_get_minor(dev, type);
if (!minor)
return 0;
  
@@ -209,7 +239,7 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)

struct drm_minor *minor;
unsigned long flags;
  
-	minor = *drm_minor_get_slot(dev, type);

+   minor = drm_minor_get_minor(dev, type);
if (!minor || !device_is_registered(minor->kdev))
return;
  
--

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 1/4] drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

2018-02-19 Thread Daniel Vetter 
On Mon, Feb 12, 2018 at 02:51:41PM -0500, Joe Moriarty wrote:
> The Parfait (version 2.1.0) static code analysis tool found the
> following NULL pointer dereference problem.
> 
> - drivers/gpu/drm/drm_drv.c
> Any calls to drm_minor_get_slot() could result in the return of a NULL
> pointer when an invalid DRM device type is encountered.  2 helper
> functions where added for pointer manipulation (drm_minor_get_slot()
> and drm_minor_set_minor()) along with checks for valid pointers for
> struct drm_device variables throughout this module.
> 
> Signed-off-by: Joe Moriarty 
> Reviewed-by: Steven Sistare 

We do not ask for an invalid minor (userspace can't do that, it would be a
kernel bug). BUG_ON for the invalid case instead of all these changes
acceptable to shut up your checker?
-Daniel

> ---
>  drivers/gpu/drm/drm_drv.c | 38 ++
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9acc1e157813..dee6a4470e2c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -99,10 +99,36 @@ static struct drm_minor **drm_minor_get_slot(struct 
> drm_device *dev,
>   case DRM_MINOR_CONTROL:
>   return >control;
>   default:
> + DRM_ERROR("Error in %s: Invalid dev, type = %d\n",
> +   __func__, type);
>   return NULL;
>   }
>  }
>  
> +static inline int drm_minor_set_minor(struct drm_device *dev,
> +   unsigned int type,
> +   struct drm_minor *minor)
> +{
> + struct drm_minor **slot = drm_minor_get_slot(dev, type);
> + int retval = -ENODEV;
> +
> + if (slot) {
> + retval = 0;
> + *slot = minor;
> + }
> + return retval;
> +}
> +
> +static inline struct drm_minor *drm_minor_get_minor(struct drm_device *dev,
> + unsigned int type)
> +{
> + struct drm_minor **slot = drm_minor_get_slot(dev, type);
> +
> + if (slot)
> + return *slot;
> + return NULL;
> +}
> +
>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  {
>   struct drm_minor *minor;
> @@ -137,8 +163,9 @@ static int drm_minor_alloc(struct drm_device *dev, 
> unsigned int type)
>   goto err_index;
>   }
>  
> - *drm_minor_get_slot(dev, type) = minor;
> - return 0;
> + r = drm_minor_set_minor(dev, type, minor);
> + if (r == 0)
> + return r;
>  
>  err_index:
>   spin_lock_irqsave(_minor_lock, flags);
> @@ -155,6 +182,9 @@ static void drm_minor_free(struct drm_device *dev, 
> unsigned int type)
>   unsigned long flags;
>  
>   slot = drm_minor_get_slot(dev, type);
> + if (!slot)
> + return;
> +
>   minor = *slot;
>   if (!minor)
>   return;
> @@ -177,7 +207,7 @@ static int drm_minor_register(struct drm_device *dev, 
> unsigned int type)
>  
>   DRM_DEBUG("\n");
>  
> - minor = *drm_minor_get_slot(dev, type);
> + minor = drm_minor_get_minor(dev, type);
>   if (!minor)
>   return 0;
>  
> @@ -209,7 +239,7 @@ static void drm_minor_unregister(struct drm_device *dev, 
> unsigned int type)
>   struct drm_minor *minor;
>   unsigned long flags;
>  
> - minor = *drm_minor_get_slot(dev, type);
> + minor = drm_minor_get_minor(dev, type);
>   if (!minor || !device_is_registered(minor->kdev))
>   return;
>  
> -- 
> 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