Re: [PATCH AUTOSEL 6.18-6.1] drm/amdkfd: add sanity check in svm_range_is_valid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdkfd: left-shift overflow of start and size in svm_range_is_valid()
--

commit 0feea440041a7f2032b2a5a7f211d24baae6cfb4
Author: Eric Huang <jinhuieric.huang@amd.com>

drm/amdkfd: add sanity check in svm_range_is_valid

This commit adds zero-size and addition-overflow checks in
svm_range_is_valid() to prevent SVM range calculations from overflowing or
underflowing.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 74a1d3e1d52be..0c81be3cd87e3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -3398,7 +3398,13 @@ svm_range_is_valid(struct kfd_process *p, uint64_t start, uint64_t size)
>  	unsigned long start_unchg = start;
>  
>  	start <<= PAGE_SHIFT;
> -	end = start + (size << PAGE_SHIFT);
> +
> +	if (size == 0)
> +		return -EINVAL;
> +
> +	if (check_add_overflow(start, size << PAGE_SHIFT, &end))
> +		return -EOVERFLOW;

[Severity: High]
Can start <<= PAGE_SHIFT and size << PAGE_SHIFT still overflow 64-bit
unsigned integers and truncate the upper PAGE_SHIFT bits before
check_add_overflow() runs?

While kfd_ioctl_svm() right-shifts byte-aligned start_addr and size by
PAGE_SHIFT before calling svm_ioctl(), the CRIU restore and resume path
copies raw 64-bit page numbers directly from userspace in
kfd_criu_restore_svm():

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:kfd_criu_restore_svm() {
	...
	ret = copy_from_user(&criu_svm_md->data, user_priv_ptr + *priv_data_offset,
			     svm_priv_data_size);
	...
}

and passes them unshifted in kfd_criu_resume_svm() -> svm_range_set_attr()
-> svm_range_is_valid():

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:kfd_criu_resume_svm() {
	...
		ret = svm_range_set_attr(p, mm, criu_svm_md->data.start_addr,
					 criu_svm_md->data.size, num_attrs + 1,
					 set_attr);
	...
}

If size has upper PAGE_SHIFT bits set (for example, (1ULL << 52) + 1 on
4KB pages), size << PAGE_SHIFT wraps to PAGE_SIZE, so svm_range_is_valid()
only validates 1 page while svm_range_set_attr() passes the untruncated
64-bit size to svm_range_add().

Similarly, if start has upper PAGE_SHIFT bits set (for example,
(1ULL << 52) + valid_pfn with size = 1), start <<= PAGE_SHIFT wraps to a
valid VMA address and svm_range_check_vm(p, start_unchg,
(end - 1) >> PAGE_SHIFT, NULL, NULL) sees start_unchg > last so
interval_tree_iter_first() returns NULL. When svm_range_set_attr() then
creates prange with the untruncated prange->start and calls
svm_range_validate_and_map():

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_validate_and_map() {
	...
		if (!r) {
			offset = (addr >> PAGE_SHIFT) - prange->start;
			r = svm_range_dma_map(prange, ctx->bitmap, offset, npages,
					      hmm_range->hmm_pfns);
	...
}

offset underflows and causes out-of-bounds array indexing (addr += offset)
in svm_range_dma_map_dev().

Should svm_range_is_valid() also check check_shl_overflow() on start and
size (or reject start and size values with upper PAGE_SHIFT bits set)?

> +
>  	do {
>  		vma = vma_lookup(p->mm, start);
>  		if (!vma || (vma->vm_flags & device_vma))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-503-sashal@kernel.org?part=1