Re: [PATCH] drm/amdgpu: reject out of range PFNs in amdgpu_vm_update_range()

[Christian König]

On 9/2/26 14:25, Andrei Rusu de Castro wrote:
> amdgpu_vm_update_range() walks the page tables for the inclusive PFN
> range [@start, @last] without validating it. Two properties have to
> hold for that walk to be well defined, and both are currently left to
> caller convention.
> 
> The range must not be reversed. The resource cursor is initialized with
> a size of (last - start + 1) * AMDGPU_GPU_PAGE_SIZE in unsigned
> arithmetic, so a @last below @start - 1 wraps to a multi-terabyte size
> and the update runs past the intended end.
> 
> @last must also stay below vm_manager.max_pfn. Unlike the lower levels,
> the root level uses a mask wide enough to be ineffective. The page-table
> walker derives an index from the PFN and applies a 0xffffffff root mask,
> while amdgpu_vm_pt_num_entries() allocates the root directory with only
> 
>         round_up(max_pfn, 1ULL << shift) >> shift
> 
> entries. A PFN at or above max_pfn therefore indexes past the root entry
> array on the first descent.
> 
> The buffer-object mapping path enforces both properties in
> amdgpu_vm_verify_parameters(), which rejects a zero size "which also
> leads to end < begin" and an lpfn >= max_pfn. Ranges reaching
> amdgpu_vm_update_range() from amdgpu_vm_bo_update() and
> amdgpu_vm_clear_freed() come from mappings validated there. The retry
> fault handler instead forwards one hardware-reported PFN without
> checking it against the configured aperture.
> 
> The KFD SVM path is not covered either. This has been possible since
> commit f80fe9d3c114 ("drm/amdkfd: map svm range to GPUs"), which first
> mapped SVM ranges into the GPUVM page tables without an equivalent bound
> check. svm_range_map_to_gpu() and svm_range_unmap_from_gpu() check only
> amdgpu_vm_ready(). Their callers guard against reversed ranges, but
> nothing compares the result against max_pfn.
> 
> When amdgpu.vm_size configures a GPU virtual address space smaller than
> the CPU address space, a process can register memory whose PFN exceeds
> max_pfn. A range near 113 TiB against a 256-entry root produced index
> 116128 and oopsed in amdgpu_vm_ptes_update().
> 
> Validate the shared page-table-walk primitive rather than relying on
> every caller to duplicate its root-array precondition. Reject invalid
> ranges before device entry, allocation, or locking. Two in-tree paths
> can reach an out-of-range PFN once the aperture is smaller than an
> address a process can touch: KFD SVM and the retry fault handler. The
> fault handler will now log -EINVAL and return the fault as unhandled.
> The validated buffer-object callers are unaffected.
> 
> Source and call-chain analysis identified the missing
> shared precondition. The amdgpu VM and KFD SVM objects and the complete
> amdgpu object were built with W=1. No runtime test was performed.
> 
> Fixes: f80fe9d3c114 ("drm/amdkfd: map svm range to GPUs")
> 
> Signed-off-by: Andrei Rusu de Castro <arc@empyreal.works>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index aedf72c2333e..f9a46d706340 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1140,6 +1140,14 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>         struct amdgpu_res_cursor cursor;
>         int r, idx;
> 
> +       /*
> +        * The page table walk indexes the root PD without masking, so an
> +        * out of range PFN would index past the end of its entry array.
> +        * A reversed range would additionally underflow the cursor size.
> +        */
> +       if (start > last || last >= adev->vm_manager.max_pfn)
> +               return -EINVAL;
> +

That is certainly not something which should be checked here.

The SVM subsystem must fail to initialize if we can't represent the addresses in the GPUVM.

If we end up here that is way to late to check.

Regards,
Christian.

>         if (!drm_dev_enter(adev_to_drm(adev), &idx))
>                 return -ENODEV;
> 
>