From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:32877 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753181AbcHWGDY (ORCPT ); Tue, 23 Aug 2016 02:03:24 -0400 Received: by mail-wm0-f67.google.com with SMTP id o80so16529251wme.0 for ; Mon, 22 Aug 2016 23:03:09 -0700 (PDT) Date: Tue, 23 Aug 2016 08:03:06 +0200 From: Daniel Vetter To: Rob Clark Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Vaishali Thakkar , Al Viro , freedreno@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl Message-ID: <20160823060306.GK24290@phenom.ffwll.local> References: <1471894685-20199-1-git-send-email-robdclark@gmail.com> <1471894685-20199-2-git-send-email-robdclark@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1471894685-20199-2-git-send-email-robdclark@gmail.com> Sender: stable-owner@vger.kernel.org List-ID: 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@vger.kernel.org > Signed-off-by: Rob Clark > --- > 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