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

[Vegard Nossum]

On 22 September 2016 at 19:38, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 4.7-stable review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Rob Clark <robdclark@xxxxxxxxx>
>
> commit d78d383ab354b0b9e1d23404ae0d9fbdeb9aa035 upstream.
>
> 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
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
>
> ---
>  drivers/gpu/drm/msm/msm_drv.h        |    6 ++++++
>  drivers/gpu/drm/msm/msm_gem.c        |    9 +++++++++
>  drivers/gpu/drm/msm/msm_gem_submit.c |    2 ++
>  3 files changed, 17 insertions(+)
>
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -148,6 +148,12 @@ struct msm_drm_private {
>         } vram;
>
>         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 {
> --- 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
>  {
>         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
>          */
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -394,6 +394,7 @@ int msm_ioctl_gem_submit(struct drm_devi
>                 return -ENOMEM;
>
>         mutex_lock(&dev->struct_mutex);
> +       priv->struct_mutex_task = current;
>
>         ret = submit_lookup_objects(submit, args, file);
>         if (ret)
> @@ -479,6 +480,7 @@ out:
>         submit_cleanup(submit);
>         if (ret)
>                 msm_gem_submit_free(submit);
> +       priv->struct_mutex_task = NULL;
>         mutex_unlock(&dev->struct_mutex);
>         return ret;
>  }

Not a stable comment per se, but a comment on the patch itself:

It seems a bit fragile/hacky to me. For example, in all the rest of
the kernel we require that mmap_sem is not held when doing
copy_*_user(), which seems like a much simpler, intuitive, and robust
way to achieve the same kind of deadlock protection that is
implemented by this patch.

Is it not possible to 1) drop the mutex over the copy_from_user(), 2)
move the copy_from_user() out, 3) pin and pre-fault the underlying
pages, or 4) something else?

Thanks,


Vegard