Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Aug 22, 2016 at 03:38:05PM -0400, Rob Clark wrote:
> An evil userspace could try to cause deadlock by passing an unfaulted-in
> GEM bo as submit->bos (or submit->cmds) table.  Which will trigger
> msm_gem_fault() while we already hold struct_mutex.  See:
> 
> https://github.com/freedreno/msmtest/blob/master/evilsubmittest.c
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_drv.h        | 6 ++++++
>  drivers/gpu/drm/msm/msm_gem.c        | 9 +++++++++
>  drivers/gpu/drm/msm/msm_gem_submit.c | 3 +++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index a35c1b6..957801e 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -157,6 +157,12 @@ struct msm_drm_private {
>  	struct shrinker shrinker;
>  
>  	struct msm_vblank_ctrl vblank_ctrl;
> +
> +	/* task holding struct_mutex.. currently only used in submit path
> +	 * to detect and reject faults from copy_from_user() for submit
> +	 * ioctl.
> +	 */
> +	struct task_struct *struct_mutex_task;
>  };
>  
>  struct msm_format {
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 8dfdeec..f6b8945 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -196,11 +196,20 @@ int msm_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  	struct drm_device *dev = obj->dev;
> +	struct msm_drm_private *priv = dev->dev_private;
>  	struct page **pages;
>  	unsigned long pfn;
>  	pgoff_t pgoff;
>  	int ret;
>  
> +	/* This should only happen if userspace tries to pass a mmap'd
> +	 * but unfaulted gem bo vaddr into submit ioctl, triggering
> +	 * a page fault while struct_mutex is already held.  This is
> +	 * not a valid use-case so just bail.
> +	 */
> +	if (priv->struct_mutex_task == current)

READ_ONCE here I think. Otherwise should at least work, though I still
think it's sloppy ;-)
-Daniel

> +		return VM_FAULT_SIGBUS;
> +
>  	/* Make sure we don't parallel update on a fault, nor move or remove
>  	 * something from beneath our feet
>  	 */
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 03d4ce2..0be57a9 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -426,6 +426,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	priv->struct_mutex_task = current;
> +
>  	if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
>  		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
>  		if (out_fence_fd < 0) {
> @@ -549,6 +551,7 @@ out:
>  out_unlock:
>  	if (ret && (out_fence_fd >= 0))
>  		put_unused_fd(out_fence_fd);
> +	priv->struct_mutex_task = NULL;
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch