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@xxxxxxxxxxxxxxx > Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> > --- > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
