Re: [PATCH 4/6] drm/radeon: reject empty VRAM ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/radeon: NULL pointer dereference in radeon_ttm_set_active_vram_size()
--

commit 5b48ce714a8da9ec1142b39b688fe2ea60f7c240
Author: Jiale Yao <yaojiale02@163.com>

drm/radeon: reject empty VRAM ranges

This commit rejects VRAM sizes smaller than a page in radeon_ttm_init_vram()
before calling ttm_range_man_init() to avoid hitting a zero-range BUG check
in drm_mm_init().

> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index e7ab8162ac694..925593af1688b 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -69,6 +69,9 @@ struct radeon_device *radeon_get_rdev(struct ttm_device *bdev)
>  
>  static int radeon_ttm_init_vram(struct radeon_device *rdev)
>  {
> +	if (rdev->mc.real_vram_size < PAGE_SIZE)
> +		return -EINVAL;
> +
>  	return ttm_range_man_init(&rdev->mman.bdev, TTM_PL_VRAM,
>  				  false, rdev->mc.real_vram_size >> PAGE_SHIFT);
>  }

[Severity: High]
Can returning -EINVAL here lead to a NULL pointer dereference in
radeon_ttm_set_active_vram_size() during error teardown?

In drivers/gpu/drm/radeon/radeon_ttm.c, radeon_ttm_init() sets
rdev->mman.initialized = true before calling radeon_ttm_init_vram():

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_init() {
	...
	rdev->mman.initialized = true;

	r = radeon_ttm_init_vram(rdev);
	if (r) {
		DRM_ERROR("Failed initializing VRAM heap.\n");
		return r;
	}
	...
}

When rdev->mc.real_vram_size < PAGE_SIZE, radeon_ttm_init_vram() now
returns -EINVAL before ttm_range_man_init() registers the TTM_PL_VRAM
manager, leaving rdev->mman.initialized set to true while the TTM_PL_VRAM
manager slot remains NULL.

As the error propagates up through radeon_bo_init() -> rdev->asic->init() ->
radeon_device_init() to radeon_driver_load_kms(), error cleanup calls
radeon_driver_unload_kms() -> radeon_device_fini() -> radeon_fini().

The ASIC .fini callbacks (such as r100_fini(), r600_fini(), and cik_fini())
stop the CP/DMA engines before calling radeon_bo_fini(), which invokes
radeon_ttm_set_active_vram_size() (for example via r100_cp_fini() ->
r100_cp_disable()):

drivers/gpu/drm/radeon/radeon_ttm.c:radeon_ttm_set_active_vram_size() {
	struct ttm_resource_manager *man;

	if (!rdev->mman.initialized)
		return;

	man = ttm_manager_type(&rdev->mman.bdev, TTM_PL_VRAM);
	/* this just adjusts TTM size idea, which sets lpfn to the correct value */
	man->size = size >> PAGE_SHIFT;
}

Because rdev->mman.initialized is true, wouldn't this bypass the early
return and dereference a NULL man pointer (and also abort teardown before
radeon_bo_fini() and radeon_ttm_fini() can clean up the initialized
ttm_device)?

Should radeon_ttm_init() only set rdev->mman.initialized = true after
initialization succeeds (or clean up ttm_device_fini() on failure), or
should radeon_ttm_set_active_vram_size() also check if man is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019084138.1821477-1-yaojiale02@163.com?part=4