[PATCH v2 0/4] Parfait changes

[PATCH v2 0/4] Parfait changes

[Joe Moriarty]

The following patch(s) are bugs found by the static compiler
'Parfait'.  Care was taken to make sure false positive results
were removed from this patchset.

Parfait Overview
================

https://labs.oracle.com/pls/apex/f?p=labs:49:::::P49_PROJECT_ID:13

v1:
Initial release

v2:
- Split original v1 patch into 4 separate patches per request
from Jani Nikula
- Fixed system hang during boot up on test machine.


Joe Moriarty (4):
  drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
  drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
  drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
  drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

 drivers/gpu/drm/drm_dp_mst_topology.c |  8 +++++---
 drivers/gpu/drm/drm_drv.c             | 38 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_edid.c            |  2 ++
 drivers/gpu/drm/drm_vblank.c          |  6 +++---
 4 files changed, 44 insertions(+), 10 deletions(-)

-- 
2.15.0

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

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

[Joe Moriarty]

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 <joe.moriarty@oracle.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
---
 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 &dev->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(&drm_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

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

[Joe Moriarty]

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

- drivers/gpu/drm/drm_dp_mst_topology.c
The call to drm_dp_calculate_rad() in function drm_dp_port_setup_pdt()
could result in a NULL pointer being returned to port->mstb due to a
failure to allocate memory for port->mstb.

Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 70dcfa58d3c2..ec503d416062 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1082,10 +1082,12 @@ static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
 		lct = drm_dp_calculate_rad(port, rad);
 
 		port->mstb = drm_dp_add_mst_branch_device(lct, rad);
-		port->mstb->mgr = port->mgr;
-		port->mstb->port_parent = port;
+		if (port->mstb) {
+			port->mstb->mgr = port->mgr;
+			port->mstb->port_parent = port;
 
-		send_link = true;
+			send_link = true;
+		}
 		break;
 	}
 	return send_link;
-- 
2.15.0

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

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

[Joe Moriarty]

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

- drivers/gpu/drm/drm_edid.c
The call to drm_cvt_mode() in function drm_mode_std() for the
HDTV hack resulted in the possibility of accessing a NULL pointer
if drm_mode_std() returned NULL.  A check for this added right after
the call to drm_cvt_mode() in this particular area of code.

Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
---
 drivers/gpu/drm/drm_edid.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ddd537914575..23c9977d8999 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2083,6 +2083,8 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
 	if (hsize == 1366 && vsize == 768 && vrefresh_rate == 60) {
 		mode = drm_cvt_mode(dev, 1366, 768, vrefresh_rate, 0, 0,
 				    false);
+		if (!mode)
+			return NULL;
 		mode->hdisplay = 1366;
 		mode->hsync_start = mode->hsync_start - 1;
 		mode->hsync_end = mode->hsync_end - 1;
-- 
2.15.0

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

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

[Joe Moriarty]

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 <joe.moriarty@oracle.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
---
 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

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

[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 <joe.moriarty@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>

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 &dev->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(&drm_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

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

[Daniel Vetter]

On Mon, Feb 12, 2018 at 02:51:42PM -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_dp_mst_topology.c
> The call to drm_dp_calculate_rad() in function drm_dp_port_setup_pdt()
> could result in a NULL pointer being returned to port->mstb due to a
> failure to allocate memory for port->mstb.
> 
> Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>

Small allocations don't fail, so just pushed to drm-misc-next, not -fixes.

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 70dcfa58d3c2..ec503d416062 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1082,10 +1082,12 @@ static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
>  		lct = drm_dp_calculate_rad(port, rad);
>  
>  		port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> -		port->mstb->mgr = port->mgr;
> -		port->mstb->port_parent = port;
> +		if (port->mstb) {
> +			port->mstb->mgr = port->mgr;
> +			port->mstb->port_parent = port;
>  
> -		send_link = true;
> +			send_link = true;
> +		}
>  		break;
>  	}
>  	return send_link;
> -- 
> 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

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

[Daniel Vetter]

On Mon, Feb 12, 2018 at 02:51:43PM -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_edid.c
> The call to drm_cvt_mode() in function drm_mode_std() for the
> HDTV hack resulted in the possibility of accessing a NULL pointer
> if drm_mode_std() returned NULL.  A check for this added right after
> the call to drm_cvt_mode() in this particular area of code.
> 
> Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>

Applied to drm-misc-next, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_edid.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ddd537914575..23c9977d8999 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2083,6 +2083,8 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
>  	if (hsize == 1366 && vsize == 768 && vrefresh_rate == 60) {
>  		mode = drm_cvt_mode(dev, 1366, 768, vrefresh_rate, 0, 0,
>  				    false);
> +		if (!mode)
> +			return NULL;
>  		mode->hdisplay = 1366;
>  		mode->hsync_start = mode->hsync_start - 1;
>  		mode->hsync_end = mode->hsync_end - 1;
> -- 
> 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

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 <joe.moriarty@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>

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

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

[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 <joe.moriarty@oracle.com>
>> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
> 
> 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 &dev->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(&drm_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 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 <joe.moriarty@oracle.com>
>> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
> 
> 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