Re: [PATCH 2/3] drm/amdgpu: fix VM update overrun on non-4K page kernels

On 8/6/26 06:45, Junrui Luo via B4 Relay wrote:
> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> From: Junrui Luo <[email protected]>
> 
> The contiguity scan in amdgpu_vm_update_range() rounds num_entries up to
> count * AMDGPU_GPU_PAGES_IN_CPU_PAGE, but count is only constrained by
> the loop bound when the loop body executes. The guard
> num_entries > AMDGPU_GPU_PAGES_IN_CPU_PAGE proves that
> tmp = num_entries / AMDGPU_GPU_PAGES_IN_CPU_PAGE is at least 1, while
> the initial count of 2 needs tmp >= 2.
> 
> Each iteration consumes a multiple of AMDGPU_GPU_PAGES_IN_CPU_PAGE, so a
> mapping whose GPU page count is not a multiple of it eventually reaches
> an iteration where num_entries is above AMDGPU_GPU_PAGES_IN_CPU_PAGE but
> below twice that. tmp is then 1, the loop body never runs, count keeps
> its initial value, and num_entries is rounded up past what the cursor
> holds, tripping BUG_ON(size > cur->remaining) in amdgpu_res_next().
> AMDGPU_GEM_VA is DRM_RENDER_ALLOW and amdgpu_vm_verify_parameters() only
> requires map_size to be a multiple of AMDGPU_GPU_PAGE_SIZE, so an
> unprivileged caller can reach this. On 4K page hosts
> AMDGPU_GPU_PAGES_IN_CPU_PAGE is 1, tmp >= 2 always holds, and the bug is
> unreachable.
> 
> Clamp the rounded-up value against num_entries, mirroring the min() that
> amdgpu_res_first() already applies to cur->size. A contiguous short tail
> is then mapped in full, and a non-contiguous one falls back to a single
> CPU page so the loop still makes forward progress.
> 
> Fixes: a39f2a8d7066 ("drm/amdgpu: nuke amdgpu_vm_bo_split_mapping v2")
> Reported-by: Yuhao Jiang <[email protected]>
> Assisted-by: Claude:claude-opus-5
> Cc: [email protected]
> Signed-off-by: Junrui Luo <[email protected]>
> ---
> Found by code inspection; not tested on hardware. I have no access to a
> 64K-page host with an AMD GPU, so the BUG_ON() path was not exercised at
> runtime.

That check is clearly not correct. The pages_addr must be fully consumed, otherwise we run into major problems later on.

We could do something like this instead:

if (pages_addr) {
...
	if (WARN_ON(num_entries % AMDGPU_GPU_PAGES_IN_CPU_PAGE))
		return -EINVAL;
...

Regards,
Christian.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index dc6a9d7dd0b2..365a1c4a4527 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1193,8 +1193,9 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>                                 }
>                                 if (!contiguous)
>                                         count--;
> -                               num_entries = count *
> -                                       AMDGPU_GPU_PAGES_IN_CPU_PAGE;
> +                               num_entries = min(count *
> +                                                 AMDGPU_GPU_PAGES_IN_CPU_PAGE,
> +                                                 num_entries);
>                         }
> 
>                         if (!contiguous) {
> 
> --
> 2.51.2
> 
>