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@xxxxxxxx> 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@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 ;-) > > 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. 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. -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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
