From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-f194.google.com ([209.85.213.194]:35328 "EHLO mail-yb0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752443AbcHVTi1 (ORCPT ); Mon, 22 Aug 2016 15:38:27 -0400 Received: by mail-yb0-f194.google.com with SMTP id b96so876398ybi.2 for ; Mon, 22 Aug 2016 12:38:27 -0700 (PDT) From: Rob Clark To: dri-devel@lists.freedesktop.org Cc: Daniel Vetter , Vaishali Thakkar , Al Viro , freedreno@lists.freedesktop.org, Rob Clark , stable@vger.kernel.org Subject: [PATCH 2/2] drm/msm: protect against faults from copy_from_user() in submit ioctl Date: Mon, 22 Aug 2016 15:38:05 -0400 Message-Id: <1471894685-20199-2-git-send-email-robdclark@gmail.com> In-Reply-To: <1471894685-20199-1-git-send-email-robdclark@gmail.com> References: <1471894685-20199-1-git-send-email-robdclark@gmail.com> Sender: stable-owner@vger.kernel.org List-ID: 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) + 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