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) 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 -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html