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

[sashiko-bot@kernel.org]

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