Re: [PATCH v3] vhost/vdpa: reject overflowing PA map page counts on 32-bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Pinned user pages are permanently leaked in the error path of `vhost_vdpa_pa_map()` when `pin_user_pages()` or `vhost_vdpa_map()` fails after a successful chunk mapping due to flawed `nchunks` logic.
- [High] Integer truncation of `uaddr` in `vhost_vdpa_va_map()` allows mapping arbitrary massive ranges on 32-bit systems, bypassing VMA boundaries.
- [High] On 32-bit systems, `iova &= PAGE_MASK` silently truncates IOVAs greater than 4GB, leading to incorrect IOMMU mappings and memory leaks.
- [High] `vhost_vdpa_pa_map` incorrectly uses `0` to represent an uninitialized `last_pfn`, which can cause the function to erroneously map arbitrary, unpinned physical memory if a valid page happens to reside at PFN 0.
--

commit 1f4f03872bda69e8780226c7f6b39b4e2b451c4f
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

vhost/vdpa: reject overflowing PA map page counts on 32-bit
    
vhost_vdpa_pa_map() adds the IOVA page offset to the user-controlled map
size before computing the number of pages to pin. On 32-bit systems,
where unsigned long is narrower than u64, that addition can overflow and
the code can pin and map fewer pages than the requested IOTLB range. This
commit rejects sizes that overflow the unsigned long page-count calculation.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ac55275fa0d0a..ed47b1140703e 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1102,6 +1102,7 @@ static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
>  	unsigned int gup_flags = FOLL_LONGTERM;
>  	unsigned long npages, cur_base, map_pfn, last_pfn = 0;

[Severity: High]
This isn't a bug introduced by this patch, but could the initialization of
last_pfn to 0 cause regressions if a valid page happens to reside at PFN 0?

If PFN 0 is pinned, the continuity check if (last_pfn && (this_pfn != last_pfn + 1))
fails, causing the code to map arbitrary, unpinned physical memory.

>  	unsigned long lock_limit, sz2pin, nchunks, i;
> +	unsigned long page_offset;
>  	u64 start = iova;

[ ... ]

Further down in the same function:

drivers/vhost/vdpa.c:vhost_vdpa_pa_map() {
    ...
				map_pfn = this_pfn;
				iova += csize;
				nchunks = 0;
			}

			last_pfn = this_pfn;
		}
    ...
out:
	if (ret) {
		if (nchunks) {
			unsigned long pfn;
    ...
			WARN_ON(!last_pfn);
			for (pfn = map_pfn; pfn <= last_pfn; pfn++)
				unpin_user_page(pfn_to_page(pfn));
		}
		vhost_vdpa_unmap(v, iotlb, start, size);
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but are pinned user pages permanently leaked
in this error path when pin_user_pages() or vhost_vdpa_map() fails?

When a chunk is successfully mapped, nchunks is zeroed but map_pfn and
last_pfn track the new outstanding unmapped page. If a subsequent operation
fails, the error path skips unpinning these outstanding pages because
nchunks is 0.

Also in vhost_vdpa_pa_map():

drivers/vhost/vdpa.c:vhost_vdpa_pa_map() {
    ...
	cur_base = uaddr & PAGE_MASK;
	iova &= PAGE_MASK;
	nchunks = 0;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but might iova &= PAGE_MASK
silently truncate IOVAs greater than 4GB on 32-bit systems?

Since PAGE_MASK is an unsigned 32-bit constant, bitwise ANDing it with a
64-bit iova zero-extends the mask and clears the upper 32 bits, causing
unmap operations to fail to find the original mapping.

Finally, in vhost_vdpa_va_map():

drivers/vhost/vdpa.c:vhost_vdpa_va_map() {
    ...
		vma = find_vma(dev->mm, uaddr);
    ...
		map_size = min(size, vma->vm_end - uaddr);
    ...
}

[Severity: High]
This is a pre-existing issue, but will the truncation of the 64-bit uaddr
to 32 bits on 32-bit systems allow out-of-bounds mapping?

When uaddr is passed to find_vma() and then used in the bounds check, the
subtraction uses the untruncated 64-bit uaddr against a promoted 32-bit
vma->vm_end. This can result in a massive underflow that bypasses bounds
checks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CAMuQ4bV8OeSTOVnAPRh6ygKdogFjqEiDNj1Vbh623KBBkZgxiw@mail.gmail.com?part=1