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

[sashiko-bot@kernel.org]

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