Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Vaishali Thakkar]

Hello,

I was wondering about the call to copy_from_user in function submit_lookup_objects for drive
/gpu/drm/msm/msm_gem_submit.c  It calls copy_from_user[1] in a spin_lock, which is not normally
allowed, due to the possibility of a deadlock.  

Is there some reason that I am overlooking why it is OK in this case? Is there some code in the
same file which ensures that page fault will not occur when we are calling the function holding
spin_lock?

Thank you.

[1] http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/msm_gem_submit.c#L85

-- 
Vaishali

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar
<vaishali.thakkar@oracle.com> wrote:
> Hello,
>
> I was wondering about the call to copy_from_user in function submit_lookup_objects for drive
> /gpu/drm/msm/msm_gem_submit.c  It calls copy_from_user[1] in a spin_lock, which is not normally
> allowed, due to the possibility of a deadlock.
>
> Is there some reason that I am overlooking why it is OK in this case? Is there some code in the
> same file which ensures that page fault will not occur when we are calling the function holding
> spin_lock?

hmm, probably just that it isn't typical to use a swap file on these
devices (and that lockdep/etc doesn't warn about it)..  I guess we
probably need some sort of slow-path where we drop the lock and try
again in case there would be a fault..

BR,
-R

> Thank you.
>
> [1] http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/msm_gem_submit.c#L85
>
> --
> Vaishali

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Al Viro]

On Wed, Aug 17, 2016 at 11:08:46AM -0400, Rob Clark wrote:
> On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar
> <vaishali.thakkar@oracle.com> wrote:
> > Hello,
> >
> > I was wondering about the call to copy_from_user in function submit_lookup_objects for drive
> > /gpu/drm/msm/msm_gem_submit.c  It calls copy_from_user[1] in a spin_lock, which is not normally
> > allowed, due to the possibility of a deadlock.
> >
> > Is there some reason that I am overlooking why it is OK in this case? Is there some code in the
> > same file which ensures that page fault will not occur when we are calling the function holding
> > spin_lock?
> 
> hmm, probably just that it isn't typical to use a swap file on these
> devices (and that lockdep/etc doesn't warn about it)..  I guess we
> probably need some sort of slow-path where we drop the lock and try
> again in case there would be a fault..

Sigh...  Folks, you don't need swap *at* *all* for copy_from_user() to block.
	/* get a zero-filled 64K buffer */
	addr = mmap(NULL, 65536, PROT_READ | PROT_WRITE,
		    MAP_ANONYMOUS | MAP_SHARED, -1, 0);
	if (addr < 0)
		piss off
	buffer = (void *)addr;
	....
	pass buf to a syscall
and copy_from_user() in that syscall will have to allocate pages (and possibly
page tables as well).  Which can block just fine, no swap involved.  Moreover,
if you modify some parts of the buffer first, you will get the pages containing
those modifications already present, but anything still untouched will
	a) act as if it had been zeroed first and
	b) possibly block on the first dereference, be it from kernel or from
userland.  Worse yet, there's nothing to stop libc from using the above for
calloc() and its ilk, with your application having no way to tell.  As far
as application is concerned, it has asked a library function to allocate and
zero a piece of memory, got one and yes, it does appear to be properly zeroed.

The bottom line is, copy_from_user() can realistically block, without
anything fishy going on in the userland setup.

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Wed, Aug 17, 2016 at 1:08 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Aug 17, 2016 at 11:08:46AM -0400, Rob Clark wrote:
>> On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar
>> <vaishali.thakkar@oracle.com> wrote:
>> > Hello,
>> >
>> > I was wondering about the call to copy_from_user in function submit_lookup_objects for drive
>> > /gpu/drm/msm/msm_gem_submit.c  It calls copy_from_user[1] in a spin_lock, which is not normally
>> > allowed, due to the possibility of a deadlock.
>> >
>> > Is there some reason that I am overlooking why it is OK in this case? Is there some code in the
>> > same file which ensures that page fault will not occur when we are calling the function holding
>> > spin_lock?
>>
>> hmm, probably just that it isn't typical to use a swap file on these
>> devices (and that lockdep/etc doesn't warn about it)..  I guess we
>> probably need some sort of slow-path where we drop the lock and try
>> again in case there would be a fault..
>
> Sigh...  Folks, you don't need swap *at* *all* for copy_from_user() to block.
>         /* get a zero-filled 64K buffer */
>         addr = mmap(NULL, 65536, PROT_READ | PROT_WRITE,
>                     MAP_ANONYMOUS | MAP_SHARED, -1, 0);
>         if (addr < 0)
>                 piss off
>         buffer = (void *)addr;
>         ....
>         pass buf to a syscall


Sure, I know that.. but if you pass random garbage cmstream to the
gpu, it will crash (the gpu) too and/or result in corrupt rendering on
screen, etc.  GPU submit APIs don't exist for random end users, they
exist for one user that knows what it is doing (ie. mesa).

I'm not saying that I shouldn't fix it (although not quite sure how
yet.. taking/dropping the spinlock inside the loop is not a good
option from a performance standpoint).  What I am saying is that this
is not something that can happen accidentally (as it could in the case
of swap).  But I agree that I should fix it somehow to avoid issues
with an intentionally evil userspace.

If there is a copy_from_user() variant that will return an error
instead of blocking, I think that is really what I want so I can
implement a slow-path that drops the spin-lock temporarily.

BR,
-R


> and copy_from_user() in that syscall will have to allocate pages (and possibly
> page tables as well).  Which can block just fine, no swap involved.  Moreover,
> if you modify some parts of the buffer first, you will get the pages containing
> those modifications already present, but anything still untouched will
>         a) act as if it had been zeroed first and
>         b) possibly block on the first dereference, be it from kernel or from
> userland.  Worse yet, there's nothing to stop libc from using the above for
> calloc() and its ilk, with your application having no way to tell.  As far
> as application is concerned, it has asked a library function to allocate and
> zero a piece of memory, got one and yes, it does appear to be properly zeroed.
>
> The bottom line is, copy_from_user() can realistically block, without
> anything fishy going on in the userland setup.

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Wed, Aug 17, 2016 at 2:49 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Aug 17, 2016 at 1:08 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Wed, Aug 17, 2016 at 11:08:46AM -0400, Rob Clark wrote:
>>> On Wed, Aug 17, 2016 at 7:40 AM, Vaishali Thakkar
>>> <vaishali.thakkar@oracle.com> wrote:
>>> > Hello,
>>> >
>>> > I was wondering about the call to copy_from_user in function submit_lookup_objects for drive
>>> > /gpu/drm/msm/msm_gem_submit.c  It calls copy_from_user[1] in a spin_lock, which is not normally
>>> > allowed, due to the possibility of a deadlock.
>>> >
>>> > Is there some reason that I am overlooking why it is OK in this case? Is there some code in the
>>> > same file which ensures that page fault will not occur when we are calling the function holding
>>> > spin_lock?
>>>
>>> hmm, probably just that it isn't typical to use a swap file on these
>>> devices (and that lockdep/etc doesn't warn about it)..  I guess we
>>> probably need some sort of slow-path where we drop the lock and try
>>> again in case there would be a fault..
>>
>> Sigh...  Folks, you don't need swap *at* *all* for copy_from_user() to block.
>>         /* get a zero-filled 64K buffer */
>>         addr = mmap(NULL, 65536, PROT_READ | PROT_WRITE,
>>                     MAP_ANONYMOUS | MAP_SHARED, -1, 0);
>>         if (addr < 0)
>>                 piss off
>>         buffer = (void *)addr;
>>         ....
>>         pass buf to a syscall
>
>
> Sure, I know that.. but if you pass random garbage cmstream to the
> gpu, it will crash (the gpu) too and/or result in corrupt rendering on
> screen, etc.  GPU submit APIs don't exist for random end users, they
> exist for one user that knows what it is doing (ie. mesa).
>
> I'm not saying that I shouldn't fix it (although not quite sure how
> yet.. taking/dropping the spinlock inside the loop is not a good
> option from a performance standpoint).  What I am saying is that this
> is not something that can happen accidentally (as it could in the case
> of swap).  But I agree that I should fix it somehow to avoid issues
> with an intentionally evil userspace.
>
> If there is a copy_from_user() variant that will return an error
> instead of blocking, I think that is really what I want so I can
> implement a slow-path that drops the spin-lock temporarily.

ok, Chris pointed out copy_from_user_atomic() on irc.. that sounds
like what I want.. will put together a patch in a few

BR,
-R


> BR,
> -R
>
>
>> and copy_from_user() in that syscall will have to allocate pages (and possibly
>> page tables as well).  Which can block just fine, no swap involved.  Moreover,
>> if you modify some parts of the buffer first, you will get the pages containing
>> those modifications already present, but anything still untouched will
>>         a) act as if it had been zeroed first and
>>         b) possibly block on the first dereference, be it from kernel or from
>> userland.  Worse yet, there's nothing to stop libc from using the above for
>> calloc() and its ilk, with your application having no way to tell.  As far
>> as application is concerned, it has asked a library function to allocate and
>> zero a piece of memory, got one and yes, it does appear to be properly zeroed.
>>
>> The bottom line is, copy_from_user() can realistically block, without
>> anything fishy going on in the userland setup.

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Al Viro]

On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote:

> I'm not saying that I shouldn't fix it (although not quite sure how
> yet.. taking/dropping the spinlock inside the loop is not a good
> option from a performance standpoint).  What I am saying is that this
> is not something that can happen accidentally (as it could in the case
> of swap).  But I agree that I should fix it somehow to avoid issues
> with an intentionally evil userspace.

I wouldn't count on that not happening by accident.  With zero changes
in mesa itself - it can be as simple as change of allocator in the
bowels of libc or throwing libdmalloc into the link flags, etc.  And most
of the time it would've worked just fine, but the same call in a situation
when most of the memory is occupied by dirty pagecache pages can end up
having to wait for writeback.

> If there is a copy_from_user() variant that will return an error
> instead of blocking, I think that is really what I want so I can
> implement a slow-path that drops the spin-lock temporarily.

*shrug*

pagefault_disable()/pagefault_enable() are there for purpose, so's
__copy_from_user_inatomic()...  Just remember that __copy_from_user_inatomic()
does not check if the addresses are userland ones (i.e. the caller needs
to check access_ok() itself) and it is *NOT* guaranteed to zero what it
hadn't copied over.  Currently it does zero tail on some, but not all
architectures; come next cycle it and it will not do that zeroing on any
of those.

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Wed, Aug 17, 2016 at 3:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote:
>
>> I'm not saying that I shouldn't fix it (although not quite sure how
>> yet.. taking/dropping the spinlock inside the loop is not a good
>> option from a performance standpoint).  What I am saying is that this
>> is not something that can happen accidentally (as it could in the case
>> of swap).  But I agree that I should fix it somehow to avoid issues
>> with an intentionally evil userspace.
>
> I wouldn't count on that not happening by accident.  With zero changes
> in mesa itself - it can be as simple as change of allocator in the
> bowels of libc or throwing libdmalloc into the link flags, etc.  And most
> of the time it would've worked just fine, but the same call in a situation
> when most of the memory is occupied by dirty pagecache pages can end up
> having to wait for writeback.
>
>> If there is a copy_from_user() variant that will return an error
>> instead of blocking, I think that is really what I want so I can
>> implement a slow-path that drops the spin-lock temporarily.
>
> *shrug*
>
> pagefault_disable()/pagefault_enable() are there for purpose, so's
> __copy_from_user_inatomic()...  Just remember that __copy_from_user_inatomic()
> does not check if the addresses are userland ones (i.e. the caller needs
> to check access_ok() itself) and it is *NOT* guaranteed to zero what it
> hadn't copied over.  Currently it does zero tail on some, but not all
> architectures; come next cycle it and it will not do that zeroing on any
> of those.

hmm, looks like, at least on arm (not sure about arm64),

#define __copy_from_user_inatomic __copy_from_user

ie. copy_from_user() minus the access_ok() and memset in the
!access_ok() path.. but maybe what I want is just the
pagefault_disable() if that disables copy_from_user() being able to
block..

I guess I need to write evil_test_code.c and see what happens..

BR,
-R

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Al Viro]

On Wed, Aug 17, 2016 at 03:24:38PM -0400, Rob Clark wrote:

> hmm, looks like, at least on arm (not sure about arm64),
> 
> #define __copy_from_user_inatomic __copy_from_user
> 
> ie. copy_from_user() minus the access_ok() and memset in the
> !access_ok() path.. but maybe what I want is just the
> pagefault_disable() if that disables copy_from_user() being able to
> block..

On a bunch of platforms copy_from_user() starts with might_sleep(); again,
that'll spread to all of the pretty soon.

Right now those primitives are very badly out of sync; this will change,
but let's not add more PITA sources.

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Wed, Aug 17, 2016 at 3:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote:
>> If there is a copy_from_user() variant that will return an error
>> instead of blocking, I think that is really what I want so I can
>> implement a slow-path that drops the spin-lock temporarily.
>
> *shrug*
>
> pagefault_disable()/pagefault_enable() are there for purpose, so's
> __copy_from_user_inatomic()...  Just remember that __copy_from_user_inatomic()
> does not check if the addresses are userland ones (i.e. the caller needs
> to check access_ok() itself) and it is *NOT* guaranteed to zero what it
> hadn't copied over.  Currently it does zero tail on some, but not all
> architectures; come next cycle it and it will not do that zeroing on any
> of those.

Ok, this is what I came up with.. let me know what you think.  The
first hunk was just to see the problem in the first place (no idea if
other places on arm would have problems w/ that hunk so it wouldn't be
part of my fix+cc-stable patch.. but it seems like I good idea, I
would have discovered this issue much sooner if we had it)

------
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 35c9db8..ce2e182 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -542,6 +542,7 @@ __clear_user(void __user *addr, unsigned long n)

 static inline unsigned long __must_check copy_from_user(void *to,
const void __user *from, unsigned long n)
 {
+    might_fault();
     if (access_ok(VERIFY_READ, from, n))
         n = __copy_from_user(to, from, n);
     else /* security hole - plug it */
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 5cd4e9b..3cca013 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)
+                return -EFAULT;
+            spin_lock(&file->table_lock);
+            pagefault_disable();
         }

         if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) {
@@ -130,6 +144,7 @@ static int submit_lookup_objects(struct
msm_gem_submit *submit,

 out_unlock:
     submit->nr_bos = i;
+    pagefault_enable();
     spin_unlock(&file->table_lock);

     return ret;
------

danvet suggested the doubleplus-super-evil variant of the test
program, using an unfaulted but mmap'd gem bo passed in to submit
ioctl for the bo's table, which has the additional fun of wanting to
acquire the already held struct_mutex in msm_gem_fault().  Which
needed the further fix:

------
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 6cd4af4..4502e4b 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -201,6 +201,13 @@ int msm_gem_fault(struct vm_area_struct *vma,
struct vm_fault *vmf)
     pgoff_t pgoff;
     int ret;

+    /* I think 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
+     */
+    if (mutex_is_locked_by(&dev->struct_mutex, 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.h b/drivers/gpu/drm/msm/msm_gem.h
index b2f13cf..160b635 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -122,4 +122,18 @@ struct msm_gem_submit {
     } bos[0];
 };

+static inline bool mutex_is_locked_by(struct mutex *mutex,
+        struct task_struct *task)
+{
+    if (!mutex_is_locked(mutex))
+        return false;
+
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
+    return mutex->owner == task;
+#else
+    /* Since UP may be pre-empted, we cannot assume that we own the lock */
+    return false;
+#endif
+}
+
 #endif /* __MSM_GEM_H__ */
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 283d284..39429bb 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -18,19 +18,6 @@
 #include "msm_drv.h"
 #include "msm_gem.h"

-static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
-{
-    if (!mutex_is_locked(mutex))
-        return false;
-
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
-    return mutex->owner == task;
-#else
-    /* Since UP may be pre-empted, we cannot assume that we own the lock */
-    return false;
-#endif
-}
-
 static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
 {
     if (!mutex_trylock(&dev->struct_mutex)) {
------

And yes, I know mutex_is_locked_by() is not super-awesome.. for the
shrinker I plan to get rid of this with finer grained locking,
although I'm not sure that totally helps with the
doubleplus-evil-userspace vs submit ioctl case.. which I think could
still cause mischief by passing an unfaulted bo in the bos table,
which is processed (and with a finer-grained lock scheme, locked)
first, and then using it's vaddr for the cmds table (where it would be
faulted after already locked)..  perhaps I can just disallow a gem bo
to be used for either bos or cmds table in the submit ioctl (since
there is no legit reason to allow that)

Note that there is actually no hardware with this gpu which is non-SMP
so I don't mind kconfig to 'depends on SMP', at least for a short-term
solution.  I think any other solution isn't going to be something that
candidate for stable.

BR,
-R

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Daniel Vetter]

On Wed, Aug 17, 2016 at 08:31:20PM +0100, Al Viro wrote:
> On Wed, Aug 17, 2016 at 03:24:38PM -0400, Rob Clark wrote:
> 
> > hmm, looks like, at least on arm (not sure about arm64),
> > 
> > #define __copy_from_user_inatomic __copy_from_user
> > 
> > ie. copy_from_user() minus the access_ok() and memset in the
> > !access_ok() path.. but maybe what I want is just the
> > pagefault_disable() if that disables copy_from_user() being able to
> > block..
> 
> On a bunch of platforms copy_from_user() starts with might_sleep(); again,
> that'll spread to all of the pretty soon.
> 
> Right now those primitives are very badly out of sync; this will change,
> but let's not add more PITA sources.

That sounds great, as part of discussing this on irc with Rob I too
noticed that the the *copy*user* funcs are all rather out of sync. On
i915.ko we go full evil mode and pass (faulting) i915 buffer objects in as
targets for all these copy*user operations. And for added evilness we have
debugfs interfaces to force-unmap/evict these bo, which is used to make
sure that the fault handling in slow-paths (after dropping locks and
reacquiring them) also works - some of i915 code has slow-slow path
fallbacks ;-)

Oh and we have a debugfs knob to disable the prefaulting we do, since
without those the race is way too small.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Daniel Vetter]

On Wed, Aug 17, 2016 at 05:29:31PM -0400, Rob Clark wrote:
> On Wed, Aug 17, 2016 at 3:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Wed, Aug 17, 2016 at 02:49:32PM -0400, Rob Clark wrote:
> >> If there is a copy_from_user() variant that will return an error
> >> instead of blocking, I think that is really what I want so I can
> >> implement a slow-path that drops the spin-lock temporarily.
> >
> > *shrug*
> >
> > pagefault_disable()/pagefault_enable() are there for purpose, so's
> > __copy_from_user_inatomic()...  Just remember that __copy_from_user_inatomic()
> > does not check if the addresses are userland ones (i.e. the caller needs
> > to check access_ok() itself) and it is *NOT* guaranteed to zero what it
> > hadn't copied over.  Currently it does zero tail on some, but not all
> > architectures; come next cycle it and it will not do that zeroing on any
> > of those.
> 
> Ok, this is what I came up with.. let me know what you think.  The
> first hunk was just to see the problem in the first place (no idea if
> other places on arm would have problems w/ that hunk so it wouldn't be
> part of my fix+cc-stable patch.. but it seems like I good idea, I
> would have discovered this issue much sooner if we had it)
> 
> ------
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index 35c9db8..ce2e182 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -542,6 +542,7 @@ __clear_user(void __user *addr, unsigned long n)
> 
>  static inline unsigned long __must_check copy_from_user(void *to,
> const void __user *from, unsigned long n)
>  {
> +    might_fault();
>      if (access_ok(VERIFY_READ, from, n))
>          n = __copy_from_user(to, from, n);
>      else /* security hole - plug it */
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 5cd4e9b..3cca013 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();

Hm, I thought with spinlocks this isn't needed ... we have that in some
fastpaths to avoid locking inversion in i915_gem_execbuf.c, but that's
because a mutex critical section can still sleep.

> 
>      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)
> +                return -EFAULT;
> +            spin_lock(&file->table_lock);
> +            pagefault_disable();
>          }
> 
>          if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) {
> @@ -130,6 +144,7 @@ static int submit_lookup_objects(struct
> msm_gem_submit *submit,
> 
>  out_unlock:
>      submit->nr_bos = i;
> +    pagefault_enable();
>      spin_unlock(&file->table_lock);
> 
>      return ret;
> ------
> 
> danvet suggested the doubleplus-super-evil variant of the test
> program, using an unfaulted but mmap'd gem bo passed in to submit
> ioctl for the bo's table, which has the additional fun of wanting to
> acquire the already held struct_mutex in msm_gem_fault().  Which
> needed the further fix:
> 
> ------
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 6cd4af4..4502e4b 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -201,6 +201,13 @@ int msm_gem_fault(struct vm_area_struct *vma,
> struct vm_fault *vmf)
>      pgoff_t pgoff;
>      int ret;
> 
> +    /* I think 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
> +     */
> +    if (mutex_is_locked_by(&dev->struct_mutex, current))
> +        return VM_FAULT_SIGBUS;

This is an ok (well still horrible) heuristics for the shrinker, but for
correctness it kinda doesn't cut it. What you need to do instead is drop
all the locks, copy relocations into a temp memory area and then proceed
in the msm command submission path above.

Also reentrant mutexes are evil ;-)
-Daniel


> +
>      /* 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.h b/drivers/gpu/drm/msm/msm_gem.h
> index b2f13cf..160b635 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -122,4 +122,18 @@ struct msm_gem_submit {
>      } bos[0];
>  };
> 
> +static inline bool mutex_is_locked_by(struct mutex *mutex,
> +        struct task_struct *task)
> +{
> +    if (!mutex_is_locked(mutex))
> +        return false;
> +
> +#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
> +    return mutex->owner == task;
> +#else
> +    /* Since UP may be pre-empted, we cannot assume that we own the lock */
> +    return false;
> +#endif
> +}
> +
>  #endif /* __MSM_GEM_H__ */
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 283d284..39429bb 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -18,19 +18,6 @@
>  #include "msm_drv.h"
>  #include "msm_gem.h"
> 
> -static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
> -{
> -    if (!mutex_is_locked(mutex))
> -        return false;
> -
> -#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
> -    return mutex->owner == task;
> -#else
> -    /* Since UP may be pre-empted, we cannot assume that we own the lock */
> -    return false;
> -#endif
> -}
> -
>  static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
>  {
>      if (!mutex_trylock(&dev->struct_mutex)) {
> ------
> 
> And yes, I know mutex_is_locked_by() is not super-awesome.. for the
> shrinker I plan to get rid of this with finer grained locking,
> although I'm not sure that totally helps with the
> doubleplus-evil-userspace vs submit ioctl case.. which I think could
> still cause mischief by passing an unfaulted bo in the bos table,
> which is processed (and with a finer-grained lock scheme, locked)
> first, and then using it's vaddr for the cmds table (where it would be
> faulted after already locked)..  perhaps I can just disallow a gem bo
> to be used for either bos or cmds table in the submit ioctl (since
> there is no legit reason to allow that)
> 
> Note that there is actually no hardware with this gpu which is non-SMP
> so I don't mind kconfig to 'depends on SMP', at least for a short-term
> solution.  I think any other solution isn't going to be something that
> candidate for stable.
> 
> BR,
> -R

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

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Thu, Aug 18, 2016 at 4:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 17, 2016 at 05:29:31PM -0400, Rob Clark wrote:
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index 6cd4af4..4502e4b 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -201,6 +201,13 @@ int msm_gem_fault(struct vm_area_struct *vma,
>> struct vm_fault *vmf)
>>      pgoff_t pgoff;
>>      int ret;
>>
>> +    /* I think 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
>> +     */
>> +    if (mutex_is_locked_by(&dev->struct_mutex, current))
>> +        return VM_FAULT_SIGBUS;
>
> This is an ok (well still horrible) heuristics for the shrinker, but for
> correctness it kinda doesn't cut it. What you need to do instead is drop
> all the locks, copy relocations into a temp memory area and then proceed
> in the msm command submission path above.
>
> Also reentrant mutexes are evil ;-)

Please note that this is not a reentrant mutex in the fault path, it
bails with VM_FAULT_SIGBUG!

There is never a legit reason to use a gem bo for the bos (or cmds)
table in the ioctl, so while this may not be pretty, I believe it is
an acceptable solution.

BR,
-R

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Daniel Vetter]

On Thu, Aug 18, 2016 at 06:55:12AM -0400, Rob Clark wrote:
> On Thu, Aug 18, 2016 at 4:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Aug 17, 2016 at 05:29:31PM -0400, Rob Clark wrote:
> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> >> index 6cd4af4..4502e4b 100644
> >> --- a/drivers/gpu/drm/msm/msm_gem.c
> >> +++ b/drivers/gpu/drm/msm/msm_gem.c
> >> @@ -201,6 +201,13 @@ int msm_gem_fault(struct vm_area_struct *vma,
> >> struct vm_fault *vmf)
> >>      pgoff_t pgoff;
> >>      int ret;
> >>
> >> +    /* I think 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
> >> +     */
> >> +    if (mutex_is_locked_by(&dev->struct_mutex, current))
> >> +        return VM_FAULT_SIGBUS;
> >
> > This is an ok (well still horrible) heuristics for the shrinker, but for
> > correctness it kinda doesn't cut it. What you need to do instead is drop
> > all the locks, copy relocations into a temp memory area and then proceed
> > in the msm command submission path above.
> >
> > Also reentrant mutexes are evil ;-)
> 
> Please note that this is not a reentrant mutex in the fault path, it
> bails with VM_FAULT_SIGBUG!

Except on UP it totally deadlocks ;-)
-Daniel

> There is never a legit reason to use a gem bo for the bos (or cmds)
> table in the ioctl, so while this may not be pretty, I believe it is
> an acceptable solution.
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Use of copy_from_user in msm_gem_submit.c while holding a spin_lock

[Rob Clark]

On Thu, Aug 18, 2016 at 9:08 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 18, 2016 at 06:55:12AM -0400, Rob Clark wrote:
>> On Thu, Aug 18, 2016 at 4:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Wed, Aug 17, 2016 at 05:29:31PM -0400, Rob Clark wrote:
>> >> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> >> index 6cd4af4..4502e4b 100644
>> >> --- a/drivers/gpu/drm/msm/msm_gem.c
>> >> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> >> @@ -201,6 +201,13 @@ int msm_gem_fault(struct vm_area_struct *vma,
>> >> struct vm_fault *vmf)
>> >>      pgoff_t pgoff;
>> >>      int ret;
>> >>
>> >> +    /* I think 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
>> >> +     */
>> >> +    if (mutex_is_locked_by(&dev->struct_mutex, current))
>> >> +        return VM_FAULT_SIGBUS;
>> >
>> > This is an ok (well still horrible) heuristics for the shrinker, but for
>> > correctness it kinda doesn't cut it. What you need to do instead is drop
>> > all the locks, copy relocations into a temp memory area and then proceed
>> > in the msm command submission path above.
>> >
>> > Also reentrant mutexes are evil ;-)
>>
>> Please note that this is not a reentrant mutex in the fault path, it
>> bails with VM_FAULT_SIGBUG!
>
> Except on UP it totally deadlocks ;-)

except UP does not exist ;-)

like I mentioned, I can add 'depends SMP' to kconfig

BR,
-R

> -Daniel
>
>> There is never a legit reason to use a gem bo for the bos (or cmds)
>> table in the ioctl, so while this may not be pretty, I believe it is
>> an acceptable solution.
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch