[PATCH v2] vhost/vdpa: reject VRING_NUM larger than device max

[PATCH v2] vhost/vdpa: reject VRING_NUM larger than device max

[Jia Jia]

vhost_vring_set_num() only requires a non-zero power-of-two that fits
in 16 bits. vhost-vdpa then hands that value to the backend through
set_vq_num() without comparing it to get_vq_num_max().

A local process with access to /dev/vhost-vdpa-* can therefore set a
queue size larger than the device advertises. On the vdpa_sim backend,
the worker then walks descriptors beyond the mapped descriptor ring.
KASAN reports a 16-byte out-of-bounds read, corresponding to one
vring_desc, in the vringh IOTLB path:

  BUG: KASAN: out-of-bounds in _copy_from_iter
  Read of size 16
  copy_from_iotlb
  copydesc_iotlb
  vringh_getdesc_iotlb
  vdpasim_net_work

Check the requested size against get_vq_num_max() in
vhost_vdpa_vring_ioctl() before calling vhost_vring_ioctl(), and return
-EINVAL when the request is too large.

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
v2:
- Add the missing Signed-off-by tag.

 drivers/vhost/vdpa.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ac55275fa0d0..b2083e1ef150 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -716,6 +716,13 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
 				vhost_vdpa_unsetup_vq_irq(v, idx);
 		}
 		break;
+	case VHOST_SET_VRING_NUM:
+		/* Reject sizes above the max reported by the device. */
+		if (copy_from_user(&s, argp, sizeof(s)))
+			return -EFAULT;
+		if (s.num > ops->get_vq_num_max(vdpa))
+			return -EINVAL;
+		break;
 	}
 
 	r = vhost_vring_ioctl(&v->vdev, cmd, argp);
-- 
2.34.1

Re: [PATCH v2] vhost/vdpa: reject VRING_NUM larger than device max

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added validation for VHOST_SET_VRING_NUM suffers from a double-fetch Time-of-Check to Time-of-Use (TOCTOU) vulnerability, which completely bypasses the intended security fix.
--

--- Patch [1]: [PATCH v2] vhost/vdpa: reject VRING_NUM larger than device max ---
commit 321003a29055f06433bde7512f4893c5f9acf2a6
Author: Jia Jia <physicalmtea@gmail.com>

vhost/vdpa: reject VRING_NUM larger than device max

This commit adds a bounds check on the requested virtual queue size in
vhost_vdpa_vring_ioctl() to prevent an out-of-bounds read in the backend.
It returns -EINVAL if the requested size is larger than the device's
maximum reported value.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index c3d913bd7cac7..36fc049a4eaf6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -723,6 +723,13 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  				vhost_vdpa_unsetup_vq_irq(v, idx);
>  		}
>  		break;
> +	case VHOST_SET_VRING_NUM:
> +		/* Reject sizes above the max reported by the device. */
> +		if (copy_from_user(&s, argp, sizeof(s)))
> +			return -EFAULT;
> +		if (s.num > ops->get_vq_num_max(vdpa))
> +			return -EINVAL;
> +		break;

[Severity: High]
Does this introduce a double-fetch time-of-check to time-of-use vulnerability?

The size is checked here using copy_from_user(), but the same user space
pointer (argp) is fetched a second time later in the call chain.

After this check, vhost_vdpa_vring_ioctl() calls vhost_vring_ioctl() which
passes the same pointer down:

vhost_vring_ioctl() {
        ...
        if (ioctl == VHOST_SET_VRING_NUM ||
            ioctl == VHOST_SET_VRING_ADDR) {
                return vhost_vring_set_num_addr(d, vq, ioctl, argp);
        }
        ...
}

This calls into vhost_vring_set_num() and performs a second fetch from argp:

vhost_vring_set_num() {
        if (copy_from_user(&s, argp, sizeof s))
                return -EFAULT;

        if (!s.num || s.num > 0xffff || (s.num & (s.num - 1)))
                return -EINVAL;
        vq->num = s.num;
}

If a malicious user space thread modifies the memory at argp between the first
and second fetches, it can bypass the get_vq_num_max() check. 

The inflated vq->num is then pushed to the backend via ops->set_vq_num(),
which appears to trigger the exact out-of-bounds read this patch aims to fix.

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