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