While reviewing Christian's annotation patch I noticed that we have a user-after-free for the WAIT_FOR_SUBMIT case: We drop the syncobj reference before we've completed the waiting.
Of course usually there's nothing bad happening here since userspace keeps the reference, but we can't rely on userspace to play nice here!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Fixes: bc9c80fe01a2 ("drm/syncobj: use the timeline point in drm_syncobj_find_fence v4") Cc: Christian König christian.koenig@amd.com Cc: Lionel Landwerlin lionel.g.landwerlin@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.2+ --- drivers/gpu/drm/drm_syncobj.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6e74e6745eca..349146049849 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -388,19 +388,18 @@ int drm_syncobj_find_fence(struct drm_file *file_private, return -ENOENT;
*fence = drm_syncobj_fence_get(syncobj); - drm_syncobj_put(syncobj);
if (*fence) { ret = dma_fence_chain_find_seqno(fence, point); if (!ret) - return 0; + goto out; dma_fence_put(*fence); } else { ret = -EINVAL; }
if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)) - return ret; + goto out;
memset(&wait, 0, sizeof(wait)); wait.task = current; @@ -432,6 +431,9 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (wait.node.next) drm_syncobj_remove_wait(syncobj, &wait);
+out: + drm_syncobj_put(syncobj); + return ret; } EXPORT_SYMBOL(drm_syncobj_find_fence);
Am 19.01.21 um 14:03 schrieb Daniel Vetter:
While reviewing Christian's annotation patch I noticed that we have a user-after-free for the WAIT_FOR_SUBMIT case: We drop the syncobj reference before we've completed the waiting.
Of course usually there's nothing bad happening here since userspace keeps the reference, but we can't rely on userspace to play nice here!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Fixes: bc9c80fe01a2 ("drm/syncobj: use the timeline point in drm_syncobj_find_fence v4") Cc: Christian König christian.koenig@amd.com Cc: Lionel Landwerlin lionel.g.landwerlin@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.2+
Reviewed-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/drm_syncobj.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6e74e6745eca..349146049849 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -388,19 +388,18 @@ int drm_syncobj_find_fence(struct drm_file *file_private, return -ENOENT;
*fence = drm_syncobj_fence_get(syncobj);
drm_syncobj_put(syncobj);
if (*fence) { ret = dma_fence_chain_find_seqno(fence, point); if (!ret)
return 0;
goto out;dma_fence_put(*fence); } else { ret = -EINVAL; }
if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
return ret;
goto out;memset(&wait, 0, sizeof(wait)); wait.task = current;
@@ -432,6 +431,9 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (wait.node.next) drm_syncobj_remove_wait(syncobj, &wait);
+out:
- drm_syncobj_put(syncobj);
- return ret; } EXPORT_SYMBOL(drm_syncobj_find_fence);
On Tue, Jan 19, 2021 at 02:08:12PM +0100, Christian König wrote:
Am 19.01.21 um 14:03 schrieb Daniel Vetter:
While reviewing Christian's annotation patch I noticed that we have a user-after-free for the WAIT_FOR_SUBMIT case: We drop the syncobj reference before we've completed the waiting.
Of course usually there's nothing bad happening here since userspace keeps the reference, but we can't rely on userspace to play nice here!
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Fixes: bc9c80fe01a2 ("drm/syncobj: use the timeline point in drm_syncobj_find_fence v4") Cc: Christian König christian.koenig@amd.com Cc: Lionel Landwerlin lionel.g.landwerlin@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.2+
Reviewed-by: Christian König christian.koenig@amd.com
Pushed to drm-misc-fixes, thanks for reviewing. -Daniel
drivers/gpu/drm/drm_syncobj.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 6e74e6745eca..349146049849 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -388,19 +388,18 @@ int drm_syncobj_find_fence(struct drm_file *file_private, return -ENOENT; *fence = drm_syncobj_fence_get(syncobj);
- drm_syncobj_put(syncobj); if (*fence) { ret = dma_fence_chain_find_seqno(fence, point); if (!ret)
return 0;
goto out;
return ret;
goto out;@@ -432,6 +431,9 @@ int drm_syncobj_find_fence(struct drm_file *file_private, if (wait.node.next) drm_syncobj_remove_wait(syncobj, &wait); +out:
- drm_syncobj_put(syncobj);
- return ret; } EXPORT_SYMBOL(drm_syncobj_find_fence);
dri-devel@lists.freedesktop.org
-
Christian König -
Daniel Vetter -
Daniel Vetter