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

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 <[email protected]>

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