drm/msm: protect against faults from copy_from_user() in submit ioctl
Subsystems (3)
PATCH 1/1 drm/msm: protect against faults from copy_from_user() in submit ioctl
commit d78d383ab354b0b9e1d23404ae0d9fbdeb9aa035 upstream.
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
--- drivers/gpu/drm/msm/msm_drv.h | 6 ++++++ drivers/gpu/drm/msm/msm_gem.c | 9 +++++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 2 ++ 3 files changed, 17 insertions(+)
drivers/gpu/drm/msm/msm_drv.h +6 -0
@@ -148,6 +148,12 @@ struct msm_drm_private { } vram;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 {
drivers/gpu/drm/msm/msm_gem.c +9 -0
@@ -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 */
drivers/gpu/drm/msm/msm_gem_submit.c +2 -0
@@ -394,6 +394,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,return -ENOMEM ;mutex_lock (&dev->struct_mutex); + priv->struct_mutex_task = current; ret =submit_lookup_objects (submit, args, file);if (ret)
@@ -479,6 +480,7 @@ out:submit_cleanup (submit);if (ret)msm_gem_submit_free (submit); + priv->struct_mutex_task =NULL ;mutex_unlock (&dev->struct_mutex);return ret; }
Signature · 1 line
2.7.4
Greg KH
maintainerQuoted 8 lines · Rob Clark
commit
d78d383ab354b0b9e1d23404ae0d9fbdeb9aa035upstream.An evil userspace could try to cause deadlock by passing an unfaulted-in GEM bo as
submit->bos(orsubmit->cmds) table. Which will triggermsm_gem_fault()while we already holdstruct_mutex. See:https://github.com/freedreno/msmtest/blob/master/evilsubmittest.c
Quoted 7 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/msm_drv.h | 6 ++++++ drivers/gpu/drm/msm/msm_gem.c | 9 +++++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 2 ++ 3 files changed, 17 insertions(+)
What stable kernel(s) do you want this applied to?
thanks,
greg k-h
Quoted 17 lines · Greg KH
Quoted 8 lines · Rob Clark
commit
d78d383ab354b0b9e1d23404ae0d9fbdeb9aa035upstream.An evil userspace could try to cause deadlock by passing an unfaulted-in GEM bo as
submit->bos(orsubmit->cmds) table. Which will triggermsm_gem_fault()while we already holdstruct_mutex. See:https://github.com/freedreno/msmtest/blob/master/evilsubmittest.c
Quoted 7 lines
Signed-off-by Rob Clark <[email protected]>--- drivers/gpu/drm/msm/msm_drv.h | 6 ++++++ drivers/gpu/drm/msm/msm_gem.c | 9 +++++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 2 ++ 3 files changed, 17 insertions(+)
Quoted 1 line
What stable kernel(s) do you want this applied to?
Oh, sorry, this was rebased back to 4.7 (since I got an email that it did not apply there). And I expect it should apply cleanly in this form going back a few more kernel versions from there.
BR,
-R
Quoted 3 lines
thanks,
greg k-h