Re: [PATCH] vhost-vdpa: reject zero-size unmap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pinned page leak in vhost_vdpa_pa_map() error paths due to incorrect nchunks reset.
- [High] Missing vma->vm_start boundary check in vhost_vdpa_va_map() causes integer underflow for map offset calculation.
- [Medium] The core vhost ioctl handler for VHOST_IOTLB_INVALIDATE does not reject zero-size requests, leading to integer underflow.
- [Medium] vhost_vdpa_va_map silently skips VMAs that do not meet its backing criteria instead of returning an error, leaving holes in the mapping.
--

--- Patch [1]: [PATCH] vhost-vdpa: reject zero-size unmap ---
commit d3e42a0021551a09934ef567e35c3c64cdff596d
Author: xiongweimin <xiongweimin@kylinos.cn>
Subject: vhost-vdpa: reject zero-size unmap

This patch rejects unmap requests with a size of zero in vhost_vdpa_unmap.
This prevents an integer underflow that would cause the entire IOTLB range
to be incorrectly unmapped.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ac55275fa0d0a..f1724bfdc46fb 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1032,6 +1032,9 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v,
>  	const struct vdpa_config_ops *ops = vdpa->config;
>  	u32 asid = iotlb_to_asid(iotlb);
>  
> +	if (!size)
> +		return;
> +
>  	vhost_vdpa_iotlb_unmap(v, iotlb, iova, iova + size - 1, asid);

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing this 
zero-size fix, I noticed a similar pattern in the core vhost module.

Does vhost_process_iotlb_msg() suffer from the exact same flaw for
VHOST_IOTLB_INVALIDATE?

drivers/vhost/vhost.c:vhost_process_iotlb_msg()
    case VHOST_IOTLB_INVALIDATE:
        if (!dev->iotlb) {
            ret = -EFAULT;
            break;
        }
        vhost_vq_meta_reset(dev);
        vhost_iotlb_del_range(dev->iotlb, msg->iova,
                      msg->iova + msg->size - 1);
        break;

If userspace sends VHOST_IOTLB_INVALIDATE with size == 0, would this
similarly underflow and wipe out the entire IOTLB tree?

>  	if (ops->set_map) {
>  		if (!v->in_batch)
>  			ops->set_map(vdpa, asid, iotlb);

[Severity: High]
This is a pre-existing issue, but looking at the error rollback path in 
vhost_vdpa_va_map() mentioned in the commit message, is there a missing 
boundary check on vm_start?

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

find_vma() returns the first VMA where vm_end > uaddr, which doesn't 
guarantee that vm_start <= uaddr. If uaddr falls in an unmapped hole 
before the VMA, what happens to the offset calculation later in the 
function?

drivers/vhost/vdpa.c:vhost_vdpa_va_map()
    offset = (vma->vm_pgoff << PAGE_SHIFT) + uaddr - vma->vm_start;
    map_file->offset = offset;

Could subtracting vm_start from uaddr result in an integer underflow 
and produce an incorrect offset when uaddr < vm_start?


[Severity: Medium]
This is another pre-existing issue in the same function. Could 
vhost_vdpa_va_map() be silently dropping unsupported VMAs without 
returning an error?

drivers/vhost/vdpa.c:vhost_vdpa_va_map()
    if (!(vma->vm_file && (vma->vm_flags & VM_SHARED) &&
        !(vma->vm_flags & (VM_IO | VM_PFNMAP))))
        goto next;

When a VMA doesn't meet the backing criteria, the code skips the mapping 
logic and advances the pointers:

drivers/vhost/vdpa.c:vhost_vdpa_va_map()
next:
    size -= map_size;
    uaddr += map_size;
    map_iova += map_size;

Because the size decreases and the loop continues, the function can 
eventually return 0 (success). Will this give userspace a false guarantee 
that the entire requested IOVA range was mapped, leading to unexpected 
faults during DMA?


[Severity: High]
This is also a pre-existing issue, but I noticed a potential resource 
leak in the error paths for vhost_vdpa_pa_map().

When a discontiguous page triggers a successful map, nchunks is reset to 
0 and a new pending chunk begins at this_pfn:

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

If there are remaining pages in the current batch, they are added to the 
pending chunk. If the next batch fails to pin, the code jumps to out. 
Because nchunks is only incremented after a successful pin, it remains 0:

drivers/vhost/vdpa.c:vhost_vdpa_pa_map()
out:
    if (ret) {
        if (nchunks) {
            unsigned long pfn;

            /*
             * Unpin the outstanding pages ...
             */
            WARN_ON(!last_pfn);
            for (pfn = map_pfn; pfn <= last_pfn; pfn++)
                unpin_user_page(pfn_to_page(pfn));

Since the if (nchunks) check evaluates to false, does this bypass the 
unpin loop entirely and permanently leak the pages in the pending chunk?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716030238.124368-1-xiongwm2026@163.com?part=1