Re: [PATCH v2 2/2] drm/amdkfd: validate SVM ranges against the target GPU address limits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] drm/amdgpu: out-of-bounds array access in amdgpu_vm_pt_descendant() on invalid fault address
- [High] mm: infinite page fault loop when VMA crosses GPUVM limit
- [High] drm/amdkfd: unmap skips valid subranges if overall range exceeds GPU limit
--

commit c516e2b1eb5a9d1a668925627f8b8692a22ac1fd
Author: Daniel Huang <danielhu@amd.com>
drm/amdkfd: validate SVM ranges against the target GPU address limits

This commit adds limits validation for SVM ranges mapped to the GPU in
amdkfd, ensuring ranges don't exceed the GPU address limits. It checks
the limits in both mapping and unmapping paths, as well as the fault
handling path.

Link: https://sashiko.dev/#/patchset/20261016145129.69645-1-danielhu%40amd.com

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index cfb428c72686e..c3e4f0367522d 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1427,6 +1427,16 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  		return -EINVAL;
>  	}
>  
> +	/* Such a range was rejected by svm_range_map_to_gpu() and so was
> +	 * never mapped. Skip it rather than failing, since unmap is also
> +	 * used on the eviction and process teardown paths.
> +	 */
> +	if (!kfd_svm_range_fits_gpu(adev, start, last)) {

[Severity: High]
Does this skip unmapping for valid subranges if the overall range
exceeds the GPU limit?

When a large user-space virtual memory area (prange) partially overlaps the
GPU limit, subranges within the limit can be successfully mapped in
svm_range_map_to_gpus().

However, when the entire prange is later unmapped here,
kfd_svm_range_fits_gpu() checks the overall bounds (start to last). Since
last exceeds the limit, it returns false and unmapping is skipped entirely.

Could this leave the valid page table entries of the previously mapped
subrange permanently mapped to potentially freed memory, causing a
use-after-free and resource leak on the GPU?

> +		pr_debug("CPU[0x%llx 0x%llx] exceeds GPUVM, skipping unmap\n",
> +			 start, last);
> +		return 0;
> +	}
> +
>  	return amdgpu_vm_update_range(adev, vm, false, true, true, false, NULL, gpu_start,

[ ... ]

> @@ -1604,6 +1624,20 @@ svm_range_map_to_gpus(struct svm_range *prange, unsigned long offset,
>  			continue;
>  		}
>  
> +		/* GPUs in the same process can have different address limits.
> +		 * Skip the ones that cannot represent this range instead of
> +		 * failing the whole mapping.
> +		 */
> +		if (npages && !kfd_svm_range_fits_gpu(pdd->dev->adev,
> +						      prange->start + offset,
> +						      prange->start + offset +
> +						      npages - 1)) {

[Severity: High]
Can this cause an infinite page fault loop when the virtual memory area
crosses the GPU limit?

If the GPU faults on an address within an allocation that is below the GPU
limit, svm_range_restore_pages() allows it to proceed because it only checks
the faulting address. Then svm_range_create_unregistered_range() creates a
range spanning the entire area, which might exceed the GPU limit.

When this code skips the mapping for the GPU because the end of the range
exceeds the limit, it continues and returns 0 for success. Will the page
fault handler exit with success without resolving the fault, causing the GPU
to continuously refault on the same address and leading to a hard hang?

> +			pr_debug("device idx %d cannot map CPU[0x%lx 0x%lx]\n",
> +				 gpuidx, prange->start + offset,
> +				 prange->start + offset + npages - 1);
> +			continue;
> +		}
> +
>  		set_bit(gpuidx, prange->bitmap_mapped);

[ ... ]

> @@ -3096,6 +3130,14 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
>  		return -EFAULT;
>  	}
>  
> +	/* addr is a CPU page index here. Reject faults the GPUVM cannot
> +	 * represent before any range is looked up or migrated.
> +	 */
> +	if (!kfd_svm_range_fits_gpu(adev, addr, addr)) {

[Severity: Critical]
Can returning -EFAULT here trigger an out-of-bounds array access in the
fallback path?

When this returns -EFAULT, amdgpu_vm_handle_fault() bypasses its early return
and falls through to its fallback path. It then passes the exact same
unvalidated, out-of-bounds addr to amdgpu_vm_update_range():

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_handle_fault() {
    ...
    if (!svm_range_restore_pages(adev, pasid, vmid, node_id,
                                 addr >> PAGE_SHIFT, ts, write_fault))
        return true;
    ...
    r = amdgpu_vm_update_range(adev, vm, true, false, false, false,
                               NULL, addr, addr, flags, value, 0,
                               NULL, NULL, NULL);
    ...
}

Because amdgpu_vm_update_range() lacks internal bounds checking against
max_pfn, it eventually uses an out-of-bounds index derived from the
unvalidated addr to index into the root page directory's flexible entries
array in amdgpu_vm_pt_descendant(). Could this lead to memory corruption or
a kernel panic?

> +		pr_debug("fault address 0x%llx exceeds GPUVM\n", addr);
> +		return -EFAULT;
> +	}
> +
>  	p = kfd_lookup_process_by_pasid(pasid, NULL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016170546.88570-1-danielhu@amd.com?part=2