Re: [PATCH 3/3] drm/amdgpu: add the BO-va mapping offset when kmapping an IB

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

Pre-existing issues:
- [High] Incomplete bug fix: multiple CPU-side consumers of `amdgpu_cs_find_mapping()` ignore `mapping->offset`, causing validation bypasses and incorrect hardware programming.
--

--- Patch [3]: [PATCH 3/3] drm/amdgpu: add the BO-va mapping offset when kmapping an IB ---
commit d8bdec727584870cf6250f253ea262e5bbf96aef
Author: Junrui Luo <[email protected]>

drm/amdgpu: add the BO-va mapping offset when kmapping an IB

This patch fixes the CPU-side view of indirect buffers (IBs) in
amdgpu_cs_patch_ibs(). It adds the missing mapping offset to the kmapped
pointer so it accurately reflects the bytes programmed into the page tables.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 9c514cb010966..a72cee871af3d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1047,7 +1047,7 @@ static int amdgpu_cs_patch_ibs(struct amdgpu_cs_parser *p,
>  		if (r)
>  			return r;
>  
> -		kptr += va_start - (m->start * AMDGPU_GPU_PAGE_SIZE);
> +		kptr += m->offset + va_start - (m->start * AMDGPU_GPU_PAGE_SIZE);

[Severity: High]
This is a pre-existing issue, but while this patch fixes the mapping offset
calculation here, do we need to apply the same fix to other CPU-side consumers
of amdgpu_cs_find_mapping()?

The commit message notes that every other CPU-side consumer omits
mapping->offset in the same way. Does this mean they are still vulnerable to
incorrect hardware programming or validation bypasses?

For example, when calculating the physical GPU address to patch into the UVD
IB, amdgpu_uvd_cs_pass2() in drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c does not
include mapping->offset:

	addr -= mapping->start * AMDGPU_GPU_PAGE_SIZE;
	start += addr;

Similar calculations omitting mapping->offset occur in:
- drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c:amdgpu_vce_cs_reloc()
- drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c:vcn_v3_0_dec_msg()
- drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c:vcn_v4_0_dec_msg()
- drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c:amdgpu_dev_coredump_print_ibs()
- drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c:amdgpu_userq_fence_read_wptr()

If a user submits a command stream with an IB mapped with a non-zero
offset_in_bo, could this lead to hardware hangs by bypassing VCN codec
validation, or corrupt UVD/VCE command stream physical address patching?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3