Re: [PATCH] drm/amdgpu/virt: fix integer overflow in size_kb shift

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