[PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock

[PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock

[Rob Clark]

Use instead __copy_from_user_inatomic() and fallback to slow-path where
we drop and re-aquire the lock in case of fault.

Cc: stable@vger.kernel.org
Reported-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 42323d1..03d4ce2 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -66,6 +66,14 @@ void msm_gem_submit_free(struct msm_gem_submit *submit)
 	kfree(submit);
 }
 
+static inline unsigned long __must_check
+copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
+{
+	if (access_ok(VERIFY_READ, from, n))
+		return __copy_from_user_inatomic(to, from, n);
+	return -EFAULT;
+}
+
 static int submit_lookup_objects(struct msm_gem_submit *submit,
 		struct drm_msm_gem_submit *args, struct drm_file *file)
 {
@@ -73,6 +81,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 	int ret = 0;
 
 	spin_lock(&file->table_lock);
+	pagefault_disable();
 
 	for (i = 0; i < args->nr_bos; i++) {
 		struct drm_msm_gem_submit_bo submit_bo;
@@ -86,10 +95,15 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 		 */
 		submit->bos[i].flags = 0;
 
-		ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo));
-		if (ret) {
-			ret = -EFAULT;
-			goto out_unlock;
+		ret = copy_from_user_inatomic(&submit_bo, userptr, sizeof(submit_bo));
+		if (unlikely(ret)) {
+			pagefault_enable();
+			spin_unlock(&file->table_lock);
+			ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo));
+			if (ret)
+				goto out;
+			spin_lock(&file->table_lock);
+			pagefault_disable();
 		}
 
 		if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) {
@@ -129,9 +143,12 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 	}
 
 out_unlock:
-	submit->nr_bos = i;
+	pagefault_enable();
 	spin_unlock(&file->table_lock);
 
+out:
+	submit->nr_bos = i;
+
 	return ret;
 }
 
-- 
2.7.4

[PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock

[Rob Clark]

Use instead __copy_from_user_inatomic() and fallback to slow-path where
we drop and re-aquire the lock in case of fault.

Cc: stable@vger.kernel.org
Reported-by: Vaishali Thakkar <vaishali.thakkar@oracle.com>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 42323d1..03d4ce2 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -66,6 +66,14 @@ void msm_gem_submit_free(struct msm_gem_submit *submit)
 	kfree(submit);
 }
 
+static inline unsigned long __must_check
+copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
+{
+	if (access_ok(VERIFY_READ, from, n))
+		return __copy_from_user_inatomic(to, from, n);
+	return -EFAULT;
+}
+
 static int submit_lookup_objects(struct msm_gem_submit *submit,
 		struct drm_msm_gem_submit *args, struct drm_file *file)
 {
@@ -73,6 +81,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 	int ret = 0;
 
 	spin_lock(&file->table_lock);
+	pagefault_disable();
 
 	for (i = 0; i < args->nr_bos; i++) {
 		struct drm_msm_gem_submit_bo submit_bo;
@@ -86,10 +95,15 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 		 */
 		submit->bos[i].flags = 0;
 
-		ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo));
-		if (ret) {
-			ret = -EFAULT;
-			goto out_unlock;
+		ret = copy_from_user_inatomic(&submit_bo, userptr, sizeof(submit_bo));
+		if (unlikely(ret)) {
+			pagefault_enable();
+			spin_unlock(&file->table_lock);
+			ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo));
+			if (ret)
+				goto out;
+			spin_lock(&file->table_lock);
+			pagefault_disable();
 		}
 
 		if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) {
@@ -129,9 +143,12 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 	}
 
 out_unlock:
-	submit->nr_bos = i;
+	pagefault_enable();
 	spin_unlock(&file->table_lock);
 
+out:
+	submit->nr_bos = i;
+
 	return ret;
 }
 
-- 
2.7.4

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Rob Clark]

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)
+		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

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

[Rob Clark]

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)
+		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

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

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

[Daniel Vetter]

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

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

[Daniel Vetter]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rob Clark]

On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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 ;-)

hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
see how there could be a race, and the "!= current" case I don't
really care..

and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
come up with any low overhead way to check if submit->cmds /
submit->objs wasn't a GEM object (otherwise I would have bailed out
earlier than copy_from_user() with an -EINVAL, and wouldn't have
needed this)

BR,
-R

> -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

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

[Rob Clark]

On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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 ;-)

hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
see how there could be a race, and the "!= current" case I don't
really care..

and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
come up with any low overhead way to check if submit->cmds /
submit->objs wasn't a GEM object (otherwise I would have bailed out
earlier than copy_from_user() with an -EINVAL, and wouldn't have
needed this)

BR,
-R

> -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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Daniel Vetter]

On Sun, Aug 28, 2016 at 12:43:46PM -0400, Rob Clark wrote:
> On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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 ;-)
> 
> hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
> see how there could be a race, and the "!= current" case I don't
> really care..
> 
> and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
> come up with any low overhead way to check if submit->cmds /
> submit->objs wasn't a GEM object (otherwise I would have bailed out
> earlier than copy_from_user() with an -EINVAL, and wouldn't have
> needed this)

Like I said imo the correct fix isn't trying to detect your own objects,
but by adding a slowpath into the CS ioctl where you drop locks and copy
relocations (or whatever it is you need) into a vmalloced (or similar)
staging area. Not fast, but it'll work. And the only signal you need for
that is the short read/write from copy_*_user_atomic.

Without this you'll still hit the lockdep snag as soon as arm's copy*user
functions gain all the necessary annotations (like x86 already has). And
that seems to happen rsn if Al Viro's plans hold up.
-Daniel

> 
> BR,
> -R
> 
> > -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

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

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

[Daniel Vetter]

On Sun, Aug 28, 2016 at 12:43:46PM -0400, Rob Clark wrote:
> On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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 ;-)
> 
> hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
> see how there could be a race, and the "!= current" case I don't
> really care..
> 
> and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
> come up with any low overhead way to check if submit->cmds /
> submit->objs wasn't a GEM object (otherwise I would have bailed out
> earlier than copy_from_user() with an -EINVAL, and wouldn't have
> needed this)

Like I said imo the correct fix isn't trying to detect your own objects,
but by adding a slowpath into the CS ioctl where you drop locks and copy
relocations (or whatever it is you need) into a vmalloced (or similar)
staging area. Not fast, but it'll work. And the only signal you need for
that is the short read/write from copy_*_user_atomic.

Without this you'll still hit the lockdep snag as soon as arm's copy*user
functions gain all the necessary annotations (like x86 already has). And
that seems to happen rsn if Al Viro's plans hold up.
-Daniel

> 
> BR,
> -R
> 
> > -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

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

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

[Rob Clark]

On Sun, Aug 28, 2016 at 12:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Aug 28, 2016 at 12:43:46PM -0400, Rob Clark wrote:
>> On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > 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 ;-)
>>
>> hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
>> see how there could be a race, and the "!= current" case I don't
>> really care..
>>
>> and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
>> come up with any low overhead way to check if submit->cmds /
>> submit->objs wasn't a GEM object (otherwise I would have bailed out
>> earlier than copy_from_user() with an -EINVAL, and wouldn't have
>> needed this)
>
> Like I said imo the correct fix isn't trying to detect your own objects,
> but by adding a slowpath into the CS ioctl where you drop locks and copy
> relocations (or whatever it is you need) into a vmalloced (or similar)
> staging area. Not fast, but it'll work. And the only signal you need for
> that is the short read/write from copy_*_user_atomic.

well, the previous patch adds slowpath (but that is only dropping
table_lock spinlock, not struct_mutex..)

I'll consider an even-more-slowpath approach when I re-work locking to
be more fine grained, but that isn't something for -fixes (let alone
stable).  And either way, I'd prefer to just disallow abusing GEM BOs
as part of the submit ioctl data structure, since there is no valid
reason for that use-case (unlike, perhaps, a pwrite type interface).
But I'll settle for just keeping that from being a DoS vector, as this
patch does.

But that is a whole different topic.. for now I need a fixes/stable
solution, and I do not see why READ_ONCE() is needed, although if
someone could explain why it is needed I will add it..

> Without this you'll still hit the lockdep snag as soon as arm's copy*user
> functions gain all the necessary annotations (like x86 already has). And
> that seems to happen rsn if Al Viro's plans hold up.

I did locally add the missing might_fault() to arm's copy_from_user()
for testing, and did not see issues.

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> > -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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

[Rob Clark]

On Sun, Aug 28, 2016 at 12:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Aug 28, 2016 at 12:43:46PM -0400, Rob Clark wrote:
>> On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > 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 ;-)
>>
>> hmm, is READ_ONCE() actually needed?  In the "== current" case I don't
>> see how there could be a race, and the "!= current" case I don't
>> really care..
>>
>> and yeah, maybe not sloppy but I wouldn't call it pretty.  I couldn't
>> come up with any low overhead way to check if submit->cmds /
>> submit->objs wasn't a GEM object (otherwise I would have bailed out
>> earlier than copy_from_user() with an -EINVAL, and wouldn't have
>> needed this)
>
> Like I said imo the correct fix isn't trying to detect your own objects,
> but by adding a slowpath into the CS ioctl where you drop locks and copy
> relocations (or whatever it is you need) into a vmalloced (or similar)
> staging area. Not fast, but it'll work. And the only signal you need for
> that is the short read/write from copy_*_user_atomic.

well, the previous patch adds slowpath (but that is only dropping
table_lock spinlock, not struct_mutex..)

I'll consider an even-more-slowpath approach when I re-work locking to
be more fine grained, but that isn't something for -fixes (let alone
stable).  And either way, I'd prefer to just disallow abusing GEM BOs
as part of the submit ioctl data structure, since there is no valid
reason for that use-case (unlike, perhaps, a pwrite type interface).
But I'll settle for just keeping that from being a DoS vector, as this
patch does.

But that is a whole different topic.. for now I need a fixes/stable
solution, and I do not see why READ_ONCE() is needed, although if
someone could explain why it is needed I will add it..

> Without this you'll still hit the lockdep snag as soon as arm's copy*user
> functions gain all the necessary annotations (like x86 already has). And
> that seems to happen rsn if Al Viro's plans hold up.

I did locally add the missing might_fault() to arm's copy_from_user()
for testing, and did not see issues.

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> > -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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno