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 <[email protected]> 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/[email protected]?part=1
