[PATCH] drm/amdgpu/umsch: free UMSCH resources on debug BO allocation failure

[PATCH] drm/amdgpu/umsch: free UMSCH resources on debug BO allocation failure

[Haoxiang Li]

umsch_mm_init() allocates a writeback slot and a command buffer
BO before allocating the UMSCH debug BO. If the debug BO allocation
fails, the function returns directly without releasing the previously
allocated command buffer BO and writeback slot.

Release the command buffer BO and free the writeback slot on this
failure path to avoid leaking resources during UMSCH initialization.

Fixes: 98a2e3a0d155 ("drm/amdgpu/umsch: add support to capture fw debug log")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
index cd707d70a0bf..622cc7320fa8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
@@ -300,6 +300,10 @@ static int umsch_mm_init(struct amdgpu_device *adev)
 				    &adev->umsch_mm.log_cpu_addr);
 	if (r) {
 		dev_err(adev->dev, "(%d) failed to allocate umsch debug bo\n", r);
+		amdgpu_bo_free_kernel(&adev->umsch_mm.cmd_buf_obj,
+			      &adev->umsch_mm.cmd_buf_gpu_addr,
+			      (void **)&adev->umsch_mm.cmd_buf_ptr);
+		amdgpu_device_wb_free(adev, adev->umsch_mm.wb_index);
 		return r;
 	}
 
-- 
2.25.1

Re: [PATCH] drm/amdgpu/umsch: free UMSCH resources on debug BO allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Unvalidated header_size allows out-of-bounds read in debugfs node.
- [Medium] Resource leak on IP block initialization failure paths.
--

commit a513c879c542eea03995c30cb8fc3fe49117d147
Author: Haoxiang Li <haoxiang_li2024@163.com>

drm/amdgpu/umsch: free UMSCH resources on debug BO allocation failure

The commit adds resource cleanup for cmd_buf_obj and wb_index in 
umsch_mm_init(). It fixes a resource leak that occurs if the debug BO 
allocation fails.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
> index cd707d70a0bf8..622cc7320fa83 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c
> @@ -300,6 +300,10 @@ static int umsch_mm_init(struct amdgpu_device *adev)
>  				    &adev->umsch_mm.log_cpu_addr);
>  	if (r) {
>  		dev_err(adev->dev, "(%d) failed to allocate umsch debug bo\n", r);
> +		amdgpu_bo_free_kernel(&adev->umsch_mm.cmd_buf_obj,
> +			      &adev->umsch_mm.cmd_buf_gpu_addr,
> +			      (void **)&adev->umsch_mm.cmd_buf_ptr);
> +		amdgpu_device_wb_free(adev, adev->umsch_mm.wb_index);
>  		return r;
>  	}

[Severity: Medium]
This is a pre-existing issue, but does umsch_mm_sw_init() also leak 
resources if umsch_mm_ring_init() fails?

drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c:umsch_mm_sw_init() {
	...
	r = umsch_mm_ring_init(&adev->umsch_mm);
	if (r)
		return r;
	...
}

In the IP block framework, if the sw_init callback fails, the framework skips
calling the corresponding sw_fini teardown function. Returning directly from
umsch_mm_sw_init() without unwinding seems to permanently leak the resources 
allocated by umsch_mm_init(). 

Does a similar issue exist in umsch_mm_hw_init()?

drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c:umsch_mm_hw_init() {
	...
	r = umsch_mm_set_hw_resources(&adev->umsch_mm);
	if (r)
		return r;
	...
}

If umsch_mm_set_hw_resources() fails, the function returns directly, which
appears to leak the firmware objects previously allocated in 
umsch_mm_load_microcode().

[Severity: High]
This isn't a bug introduced by this patch, but could 
amdgpu_debugfs_umsch_fwlog_read() allow out-of-bounds memory access due to 
unvalidated plog->header_size?

drivers/gpu/drm/amd/amdgpu/amdgpu_umsch_mm.c:amdgpu_debugfs_umsch_fwlog_read() {
	...
		available = read_num[0] + write_pos - plog->header_size;
	...
			if (read_pos == AMDGPU_UMSCHFW_LOG_SIZE)
				read_pos = plog->header_size;
	...
}

Because plog->header_size is read directly from volatile memory, could a 
modified value cause an integer underflow when calculating available and 
read_num? 

When read_pos wraps, it is updated to this unvalidated header_size, which 
might cause the subsequent copy_to_user() to copy out-of-bounds kernel or I/O 
memory. 

Additionally, it appears the error handling for copy_to_user() might 
be masking partial faults by using if (read_num[i] == copy_to_user(...)).

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