[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 at 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 at 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 at vger.kernel.org
>> >> Signed-off-by: Rob Clark <robdclark at 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