On Sun, Aug 28, 2016 at 12:53 PM, Daniel Vetter [off-list ref] wrote:

On Sun, Aug 28, 2016 at 12:43:46PM -0400, Rob Clark wrote:

quoted

On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter [off-list ref] wrote:

quoted

On Mon, Aug 22, 2016 at 03:38:05PM -0400, Rob Clark wrote:

quoted

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

quoted

BR,
-R

quoted

-Daniel

quoted

+             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