* [PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock @ 2016-08-22 19:38 Rob Clark 2016-08-22 19:38 ` [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2016-08-22 19:38 UTC (permalink / raw) To: dri-devel Cc: Daniel Vetter, Vaishali Thakkar, Al Viro, freedreno, Rob Clark, stable Use instead __copy_from_user_inatomic() and fallback to slow-path where we drop and re-aquire the lock in case of fault. Cc: stable@vger.kernel.org Reported-by: Vaishali Thakkar <vaishali.thakkar@oracle.com> Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/msm/msm_gem_submit.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 42323d1..03d4ce2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -66,6 +66,14 @@ void msm_gem_submit_free(struct msm_gem_submit *submit) kfree(submit); } +static inline unsigned long __must_check +copy_from_user_inatomic(void *to, const void __user *from, unsigned long n) +{ + if (access_ok(VERIFY_READ, from, n)) + return __copy_from_user_inatomic(to, from, n); + return -EFAULT; +} + static int submit_lookup_objects(struct msm_gem_submit *submit, struct drm_msm_gem_submit *args, struct drm_file *file) { @@ -73,6 +81,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit, int ret = 0; spin_lock(&file->table_lock); + pagefault_disable(); for (i = 0; i < args->nr_bos; i++) { struct drm_msm_gem_submit_bo submit_bo; @@ -86,10 +95,15 @@ static int submit_lookup_objects(struct msm_gem_submit *submit, */ submit->bos[i].flags = 0; - ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo)); - if (ret) { - ret = -EFAULT; - goto out_unlock; + ret = copy_from_user_inatomic(&submit_bo, userptr, sizeof(submit_bo)); + if (unlikely(ret)) { + pagefault_enable(); + spin_unlock(&file->table_lock); + ret = copy_from_user(&submit_bo, userptr, sizeof(submit_bo)); + if (ret) + goto out; + spin_lock(&file->table_lock); + pagefault_disable(); } if (submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) { @@ -129,9 +143,12 @@ static int submit_lookup_objects(struct msm_gem_submit *submit, } out_unlock: - submit->nr_bos = i; + pagefault_enable(); spin_unlock(&file->table_lock); +out: + submit->nr_bos = i; + return ret; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl 2016-08-22 19:38 [PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock Rob Clark @ 2016-08-22 19:38 ` Rob Clark 2016-08-23 6:03 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2016-08-22 19:38 UTC (permalink / raw) To: dri-devel Cc: Daniel Vetter, Vaishali Thakkar, Al Viro, freedreno, Rob Clark, stable 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 <robdclark@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) + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl 2016-08-22 19:38 ` [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl Rob Clark @ 2016-08-23 6:03 ` Daniel Vetter 2016-08-28 16:43 ` Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2016-08-23 6:03 UTC (permalink / raw) To: Rob Clark Cc: dri-devel, Daniel Vetter, Vaishali Thakkar, Al Viro, freedreno, stable 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 <robdclark@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl 2016-08-23 6:03 ` Daniel Vetter @ 2016-08-28 16:43 ` Rob Clark 2016-08-28 16:53 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2016-08-28 16:43 UTC (permalink / raw) To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, Vaishali Thakkar, Al Viro, freedreno, stable On Tue, Aug 23, 2016 at 2:03 AM, Daniel Vetter <daniel@ffwll.ch> 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@vger.kernel.org >> Signed-off-by: Rob Clark <robdclark@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 ;-) 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl 2016-08-28 16:43 ` Rob Clark @ 2016-08-28 16:53 ` Daniel Vetter 2016-08-28 17:29 ` Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2016-08-28 16:53 UTC (permalink / raw) To: Rob Clark Cc: Daniel Vetter, dri-devel@lists.freedesktop.org, Vaishali Thakkar, Al Viro, freedreno, stable 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@ffwll.ch> 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@vger.kernel.org > >> Signed-off-by: Rob Clark <robdclark@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 ;-) > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl 2016-08-28 16:53 ` Daniel Vetter @ 2016-08-28 17:29 ` Rob Clark 0 siblings, 0 replies; 6+ messages in thread From: Rob Clark @ 2016-08-28 17:29 UTC (permalink / raw) To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, Vaishali Thakkar, Al Viro, freedreno, stable On Sun, Aug 28, 2016 at 12:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > 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@ffwll.ch> 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@vger.kernel.org >> >> Signed-off-by: Rob Clark <robdclark@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 ;-) >> >> 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. well, the previous patch adds slowpath (but that is only dropping table_lock spinlock, not struct_mutex..) I'll consider an even-more-slowpath approach when I re-work locking to be more fine grained, but that isn't something for -fixes (let alone stable). And either way, I'd prefer to just disallow abusing GEM BOs as part of the submit ioctl data structure, since there is no valid reason for that use-case (unlike, perhaps, a pwrite type interface). But I'll settle for just keeping that from being a DoS vector, as this patch does. But that is a whole different topic.. for now I need a fixes/stable solution, and I do not see why READ_ONCE() is needed, although if someone could explain why it is needed I will add it.. > 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. I did locally add the missing might_fault() to arm's copy_from_user() for testing, and did not see issues. BR, -R > -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-28 17:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-22 19:38 [PATCH 1/2] drm/msm: fix use of copy_from_user() while holding spinlock Rob Clark 2016-08-22 19:38 ` [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl Rob Clark 2016-08-23 6:03 ` Daniel Vetter 2016-08-28 16:43 ` Rob Clark 2016-08-28 16:53 ` Daniel Vetter 2016-08-28 17:29 ` Rob Clark
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox