Re: [PATCH] vhost/vdpa: reject overflowing PA map page counts

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The variable `last_pfn` uses `0` as a sentinel value, which breaks the physical discontinuity check if PFN 0 is pinned, allowing an attacker to map arbitrary physical memory into the device IOMMU.
- [High] Permanent leak of pinned memory pages in the error handling path due to flawed state tracking with the `nchunks` variable.
- [High] TOCTOU race in RLIMIT_MEMLOCK check allows users to bypass the locked memory limit by concurrently mapping pages across multiple vhost-vdpa devices.
--

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

vhost/vdpa: reject overflowing PA map page counts

This commit adds a check in vhost_vdpa_pa_map to prevent integer overflows
when adding the size and page offset. It also converts the pinned memory
limit check to use subtraction to prevent wrapping.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ac55275fa0d0a..090cb8693b7ce 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1102,6 +1102,8 @@ 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: Critical]
This is a pre-existing issue, but does using 0 as a sentinel for last_pfn
break the discontinuity check if PFN 0 is pinned?

If PFN 0 is pinned, last_pfn becomes 0, and this check is silently bypassed
for all subsequent pages:

vhost_vdpa_pa_map() {
    ...
    if (last_pfn && (this_pfn != last_pfn + 1)) {
    ...
}

Could this allow discontiguous physical pages to be incorrectly grouped
together and map arbitrary physical memory into the device IOMMU?

[ ... ]

> @@ -1123,7 +1130,8 @@ static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
>  	mmap_read_lock(dev->mm);
>  
>  	lock_limit = PFN_DOWN(rlimit(RLIMIT_MEMLOCK));
> -	if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) {
> +	pinned_vm = atomic64_read(&dev->mm->pinned_vm);
> +	if (npages > lock_limit || pinned_vm > lock_limit - npages) {

[Severity: High]
This isn't a bug introduced by this patch, but does this limit check contain
a race condition?

Because pinned_vm is read without global serialization across the shared
mm_struct, multiple threads could concurrently issue ioctls to update the
IOTLB. Each thread takes its own device mutex and a shared mmap_read_lock.

If all threads concurrently read pinned_vm here, couldn't they all pass the
limit check, pin memory, and increment pinned_vm later, effectively bypassing
the limit?

>  		ret = -ENOMEM;
>  		goto unlock;
>  	}

[Severity: High]
This is a pre-existing issue, but is there a leak of pinned memory pages in
the error handling path of this function?

When a discontiguous page is encountered, the previous chunk is mapped and
nchunks is cleared to 0:

vhost_vdpa_pa_map() {
    ...
    if (last_pfn && (this_pfn != last_pfn + 1)) {
        ...
        ret = vhost_vdpa_map(...)
        ...
        nchunks = 0;
    }
    ...
}

However, the inner loop immediately continues processing the new page, making
it the start of a new outstanding chunk. If a subsequent pin_user_pages()
fails in the next iteration of the outer loop, the code jumps to the out
label:

vhost_vdpa_pa_map() {
    ...
out:
    if (ret) {
        if (nchunks) {
            ...
            for (pfn = map_pfn; pfn <= last_pfn; pfn++)
                unpin_user_page(pfn_to_page(pfn));
        }
        ...
}

Because nchunks was left at 0, doesn't it skip unpinning the outstanding
pages?

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