[PATCH] drm/radeon: fix double-free/UAF of ttm->sg on userptr pin failure

Seongjun Hong posted 1 patch 1 month ago
drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] drm/radeon: fix double-free/UAF of ttm->sg on userptr pin failure
Posted by Seongjun Hong 1 month ago
If sg_alloc_table_from_pages() or dma_map_sgtable() fails inside
radeon_ttm_tt_pin_userptr(), the error path frees ttm->sg with
kfree(). But ttm->sg is not owned by this function - it is allocated
once in radeon_ttm_tt_populate() and is only supposed to be freed by
radeon_ttm_tt_unpopulate(), which persists across multiple bind/unbind
cycles of the same ttm_tt.

kfree()'ing it here without resetting ttm->sg to NULL leaves a
dangling pointer:

 - radeon_ttm_tt_unpopulate() will kfree() the same pointer again
   later, a double-free.

 - Anything that dereferences ttm->sg in the meantime (e.g. the
   "!ttm->sg || !ttm->sg->sgl" check in
   radeon_ttm_tt_unpin_userptr(), or a retried bind calling
   sg_alloc_table_from_pages(ttm->sg, ...) again) is a
   use-after-free.

Additionally, if sg_alloc_table_from_pages() succeeded but
dma_map_sgtable() failed, kfree() only frees the struct sg_table
header, not the sgl entries array it allocated internally, leaking
that allocation.

Use sg_free_table() instead, which releases only the sgl entries
this function allocated and leaves the ttm->sg header intact for
radeon_ttm_tt_unpopulate() to free later, matching the ownership the
normal (non-error) unpin path already assumes.

Fixes: f72a113a71ab ("drm/radeon: add userptr support v8")
Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Signed-off-by: Seongjun Hong <hsj0512@snu.ac.kr>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 22fc35a0e8d8..cbc0339cf127 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -374,7 +374,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
 	return 0;
 
 release_sg:
-	kfree(ttm->sg);
+	sg_free_table(ttm->sg);
 
 release_pages:
 	unpin_user_pages(ttm->pages, pinned);
-- 
2.43.0
[PATCH] drm/radeon: check radeon_ttm_tt_pin_userptr() return value in bind
Posted by Seongjun Hong 1 month ago
radeon_ttm_backend_bind() calls radeon_ttm_tt_pin_userptr() without
checking its return value. If pinning fails partway through (e.g.
sg_alloc_table_from_pages()/dma_map_sgtable() failure, or an invalid
userptr range), ttm->pages[] and gtt->ttm.dma_address[] are left
incompletely populated - containing stale entries left over from a
previous bind cycle, or uninitialized memory on the very first one.

radeon_ttm_backend_bind() proceeds anyway and calls
radeon_gart_bind(rdev, ..., ttm->pages, gtt->ttm.dma_address, flags),
which writes those stale/uninitialized DMA addresses straight into
the GPU's GART page table entries. This is not a bounds violation
(the table indices themselves stay in range), but it programs the
GPU to have DMA read/write access to whatever physical memory those
stale addresses happen to resolve to, which may since have been
freed and reused for something else.

Propagate the error and bail out before calling radeon_gart_bind().

Fixes: f72a113a71ab ("drm/radeon: add userptr support v8")
Reported-by: Sashiko AI Review <sashiko-bot@kernel.org>
Signed-off-by: Seongjun Hong <hsj0512@snu.ac.kr>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index cbc0339cf127..58bd1e73587a 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -431,7 +431,9 @@ static int radeon_ttm_backend_bind(struct ttm_device *bdev,
 		return 0;
 
 	if (gtt->userptr) {
-		radeon_ttm_tt_pin_userptr(bdev, ttm);
+		r = radeon_ttm_tt_pin_userptr(bdev, ttm);
+		if (r)
+			return r;
 		flags &= ~RADEON_GART_PAGE_WRITE;
 	}
 
-- 
2.43.0