[PATCH] drm/radeon: switch from get_user_pages to pin_user_pages

[PATCH] drm/radeon: switch from get_user_pages to pin_user_pages

[Seongjun Hong]

Switch old api get_user_pages to pin_user_pages according to [1].
It guarantees these DMA pages are pinned.

[1] Documentation/core-api/pin_user_pages.rst

Signed-off-by: Seongjun Hong <hsj0512@snu.ac.kr>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index e7ab8162ac69..22fc35a0e8d8 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -349,7 +349,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
 		uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
 		struct page **pages = ttm->pages + pinned;
 
-		r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
+		r = pin_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
 				   pages);
 		if (r < 0)
 			goto release_pages;
@@ -377,7 +377,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
 	kfree(ttm->sg);
 
 release_pages:
-	release_pages(ttm->pages, pinned);
+	unpin_user_pages(ttm->pages, pinned);
 	return r;
 }
 
@@ -404,7 +404,7 @@ static void radeon_ttm_tt_unpin_userptr(struct ttm_device *bdev, struct ttm_tt *
 			set_page_dirty(page);
 
 		mark_page_accessed(page);
-		put_page(page);
+		unpin_user_page(page);
 	}
 
 	sg_free_table(ttm->sg);
-- 
2.43.0

Re: [PATCH] drm/radeon: switch from get_user_pages to pin_user_pages

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Missing FOLL_LONGTERM flag in pin_user_pages for long-term DMA.

Pre-existing issues:
- [Critical] The error path in `radeon_ttm_tt_pin_userptr` leaks scatterlist entries and leaves a dangling pointer that triggers a Use-After-Free.
- [Critical] The return value of `radeon_ttm_tt_pin_userptr` is ignored, causing the driver to program the GPU GART with uninitialized memory.
--

commit 5aab225074cf5de663f6a8e7e139c7bfe602c69f
Author: Seongjun Hong <hsj0512@snu.ac.kr>

drm/radeon: switch from get_user_pages to pin_user_pages

This commit replaces get_user_pages with pin_user_pages to guarantee
that these DMA pages are pinned correctly.

> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index e7ab8162ac694..22fc35a0e8d81 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -349,7 +349,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
>  		uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
>  		struct page **pages = ttm->pages + pinned;
>  
> -		r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
> +		r = pin_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0,
>  				   pages);

[Severity: High]
Does this pinning operation require the FOLL_LONGTERM flag?

Documentation (Documentation/core-api/pin_user_pages.rst) dictates that
long-term DMA pins must use FOLL_LONGTERM. Without it, pinning memory
indefinitely in ZONE_MOVABLE or CMA regions might break memory compaction
and hotplug, potentially leading to CMA exhaustion.

>  		if (r < 0)
>  			goto release_pages;
[ ... ]
> @@ -377,7 +377,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm
>  	kfree(ttm->sg);

[Severity: Critical]
This is a pre-existing issue, but does this error path leak the scatterlist
and leave a dangling pointer?

If dma_map_sgtable() fails, we jump here and call kfree(ttm->sg). This
frees the container struct without calling sg_free_table(ttm->sg), leaking
the inner scatterlist array.

Additionally, ttm->sg is not set to NULL. When the object is later torn down,
radeon_ttm_tt_unpin_userptr() sees a non-NULL ttm->sg:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpin_userptr() {
    ...
    /* double check that we don't free the table twice */
    if (!ttm->sg || !ttm->sg->sgl)
        return;

    /* free the sg table and pages again */
    dma_unmap_sgtable(rdev->dev, ttm->sg, direction, 0);
    ...
}

Could this lead to a use-after-free when it dereferences the freed pointer
and attempts to unmap it?

>  
>  release_pages:
> -	release_pages(ttm->pages, pinned);
> +	unpin_user_pages(ttm->pages, pinned);
>  	return r;
>  }

[Severity: Critical]
This is also a pre-existing issue, but is it safe to ignore the return value
of radeon_ttm_tt_pin_userptr() here?

In radeon_ttm_backend_bind():

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_backend_bind() {
    ...
    if (gtt->userptr) {
        radeon_ttm_tt_pin_userptr(bdev, ttm);
        flags &= ~RADEON_GART_PAGE_WRITE;
    }

    r = radeon_gart_bind(rdev, gtt->offset, ttm->num_pages,
                         ttm->pages, gtt->ttm.dma_address, flags);
    ...
}

If radeon_ttm_tt_pin_userptr() fails (for example, if userspace provides an
invalid userptr), the failure is ignored. The ttm->dma_address array remains
uninitialized (zeroed) and is blindly passed to radeon_gart_bind().

Could this result in programming the hardware GART with zeroed DMA addresses,
effectively mapping physical address 0x0 for GPU read/write access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827090100.428302-1-hsj0512@snu.ac.kr?part=1

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

[Seongjun Hong]

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

[Seongjun Hong]

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

Re: [PATCH] drm/radeon: check radeon_ttm_tt_pin_userptr() return value in bind

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The error path in `radeon_ttm_tt_pin_userptr()` incorrectly frees `ttm->sg` using `kfree()` without setting the pointer to NULL and without properly freeing the inner scatterlist array if it was allocated. This leads to a memory leak, dangling pointer, use-after-free, double-put of pages, and double-free during the object teardown sequence.
--

commit 4389e2026f1a66251ba9951cbc3ed34d9181fa4f
Author: Seongjun Hong <hsj0512@snu.ac.kr>

drm/radeon: check radeon_ttm_tt_pin_userptr() return value in bind

This commit propagates the error from radeon_ttm_tt_pin_userptr() in
radeon_ttm_backend_bind() to avoid writing stale or uninitialized DMA
addresses to the GPU's GART page table.

> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index e7ab8162ac694..1b72cac9ed6b6 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;

[Severity: Critical]
This is a pre-existing issue, but by returning the error here, we trigger
the TTM object teardown sequence which exposes memory management bugs in
the existing error handling paths.

If radeon_ttm_tt_pin_userptr() fails at dma_map_sgtable() (for instance),
it does this:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_pin_userptr() {
    ...
	r = dma_map_sgtable(rdev->dev, ttm->sg, direction, 0);
	if (r)
		goto release_sg;
    ...
release_sg:
	kfree(ttm->sg);
release_pages:
	release_pages(ttm->pages, pinned);
	return r;
}

This frees ttm->sg without setting it to NULL, leaving a dangling pointer.
It also leaks the internal scatterlist nodes instead of using sg_free_table().

Because we now return the error from radeon_ttm_backend_bind(), TTM initiates
teardown via ttm_tt_unpopulate() -> radeon_ttm_tt_unpopulate() ->
radeon_ttm_tt_unbind() -> radeon_ttm_tt_unpin_userptr().

In radeon_ttm_tt_unpin_userptr(), we have a use-after-free read:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpin_userptr() {
    ...
	/* double check that we don't free the table twice */
	if (!ttm->sg || !ttm->sg->sgl)
		return;
    ...
}

It also does a double-put on the pages, as they were already released in the
initial error path:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpin_userptr() {
    ...
		put_page(page);
    ...
}

Finally, radeon_ttm_tt_unpopulate() causes a double-free of ttm->sg:

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_tt_unpopulate() {
    ...
	if (gtt && gtt->userptr) {
		kfree(ttm->sg);
    ...
}

Can we update the error paths in radeon_ttm_tt_pin_userptr() and related
teardown functions to safely handle these failure cases before propagating
the error here?

>  		flags &= ~RADEON_GART_PAGE_WRITE;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827160124.679309-2-hsj0512@snu.ac.kr?part=1