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: sta...@vger.kernel.org
> Signed-off-by: Rob Clark <robdcl...@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
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
