Re: [PATCH v1 1/1] drm: Multiple Null pointer dereference [null-pointer-deref] (CWE 476) problems:

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 08 Feb 2018, Joe Moriarty <joe.moriarty@oracle.com> wrote:
> The Parfait (version 2.1.0) static code analysis tool found multiple NULL
> pointer derefernce problems.

Thanks for the patch. Multiple problems requires multiple patches to fix
them, one patch per problem. Please split up the patch.

Thanks,
Jani.

>
> - 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.
>
> - 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.
>
> - 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.
>
> - 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_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(-)
>
> 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;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 9acc1e157813..938eee77f014 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_slot(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_slot(dev, type);
>  	if (!minor || !device_is_registered(minor->kdev))
>  		return;
>  
> 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;
> 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);
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel