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 <[email protected]> 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/[email protected]?part=4
