Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] kzalloc(0) returns ZERO_SIZE_PTR which bypasses NULL checks and leads to an invalid pointer dereference when the data exchange table is omitted. - [High] The delayed work vf2pf_work is not properly cancelled if the update interval is 0, leading to an infinite workqueue loop and a Use-After-Free when the device is destroyed. -- --- Patch [1]: [PATCH] drm/amdgpu/virt: fix integer overflow in size_kb shift --- commit 9ad17a9be1058b6419621f31c0a9ac90b297b043 Author: lihaojie <[email protected]> drm/amdgpu/virt: fix integer overflow in size_kb shift This commit casts size_kb to u64 before left-shifting by 10 to prevent 32-bit arithmetic overflow for buffer sizes. It updates the affected kzalloc and amdgpu_device_vram_access calls, and modifies the debug string formats to correctly print the new u64 values. > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > index b43fc643668d2..afa66d9fed906 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c [ ... ] > @@ -805,7 +805,8 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device > *adev) > /* got through this logic in early init stage to get necessary > flags, e.g. rlcg_acc related*/ > if (adev->virt.req_init_data_ver == GPU_CRIT_REGION_V2) { > pfvf_data = > - > kzalloc(adev->virt.crit_regn_tbl[AMD_SRIOV_MSG_DATAEXCHANGE_TABLE_ID].size_kb > << 10, > + kzalloc((u64)adev->virt.crit_regn_tbl > + > [AMD_SRIOV_MSG_DATAEXCHANGE_TABLE_ID].size_kb << 10, > GFP_KERNEL); > if (!pfvf_data) { > dev_err(adev->dev, "Failed to allocate memory > for pfvf_data\n"); > return; > } [Severity: High] This is a pre-existing issue, but does this allocation safely handle the case where the hypervisor omits the data exchange table? If the AMD_SRIOV_MSG_DATAEXCHANGE_TABLE_ID table is omitted, size_kb will be 0. kzalloc(0) returns ZERO_SIZE_PTR (0x10) instead of NULL, which causes the (!pfvf_data) check to succeed. Later, in amdgpu_virt_read_pf2vf_data(): if (adev->virt.fw_reserve.p_pf2vf == NULL) return -EINVAL; if (pf2vf_info->size > 1024) { The invalid ZERO_SIZE_PTR is not NULL, so it reaches the size check and is dereferenced, causing a kernel panic. Should the allocation be guarded against a 0 size? [Severity: High] This isn't a bug introduced by this patch, but is it possible for vf2pf_work to cause an infinite workqueue loop and a use-after-free during device teardown? If amdgpu_virt_read_pf2vf_data() fails to read valid data during initialization, adev->virt.vf2pf_update_interval_ms remains 0. In amdgpu_virt_fini_data_exchange(): if (adev->virt.vf2pf_update_interval_ms != 0) { dev_info(adev->dev, "clean up the vf2pf work item\n"); cancel_delayed_work_sync(&adev->virt.vf2pf_work); adev->virt.vf2pf_update_interval_ms = 0; } The work cancellation is skipped because the interval is 0. However, the work function amdgpu_virt_update_vf2pf_work_item() unconditionally reschedules itself: out: schedule_delayed_work(&(adev->virt.vf2pf_work), adev->virt.vf2pf_update_interval_ms); This leaves the work looping infinitely with a 0 delay. When the device is unloaded, the uncancelled work continues executing and dereferences adev after it has been freed. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
