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

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

New issues:
- [High] The newly added validation for VHOST_SET_VRING_NUM introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability due to a double fetch from user memory, which completely bypasses the security fix.

Pre-existing issues:
- [High] The virtqueue index (`idx`) is fetched from userspace twice, allowing concurrent modification to desynchronize the vhost core and vDPA backend states.
--

--- Patch [1]: [PATCH] vhost/vdpa: reject VRING_NUM larger than device max ---
commit a17536b4ee293947d6ce500193f7ea4d0e4cae10
Author: Jia Jia <[email protected]>

vhost/vdpa: reject VRING_NUM larger than device max

vhost_vring_set_num() only requires a non-zero power-of-two that fits
in 16 bits. This patch adds a check in vhost_vdpa_vring_ioctl() to
reject requested queue sizes larger than get_vq_num_max() before calling
vhost_vring_ioctl().

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> --- 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 validation introduce a time-of-check to time-of-use vulnerability?

The requested size is fetched and validated here in vhost_vdpa_vring_ioctl(),
but the subsequent call to vhost_vring_ioctl() ultimately delegates to
vhost_vring_set_num(), which fetches the same struct from userspace a second
time:

drivers/vhost/vhost.c:vhost_vring_set_num() {
    ...
    if (copy_from_user(&s, argp, sizeof s))
    ...
    vq->num = s.num;
}

Since a concurrent thread could modify the userspace memory between the two
fetches, could this second fetch overwrite the validated size with an
excessively large value? If so, this unvalidated value would then be passed
to ops->set_vq_num() and completely bypass the security fix.

>  	}
>  
>  	r = vhost_vring_ioctl(&v->vdev, cmd, argp);

[Severity: High]
This is a pre-existing issue, but I noticed a potential state mismatch between
the vhost core and the vDPA backend.

The virtqueue index is initially fetched at the top of vhost_vdpa_vring_ioctl():

drivers/vhost/vdpa.c:vhost_vdpa_vring_ioctl() {
    ...
    r = get_user(idx, (u32 __user *)argp);
    ...
    vq = &v->vqs[idx];
    ...
}

However, the call to vhost_vring_ioctl() performs a second fetch for the index:

drivers/vhost/vhost.c:vhost_vring_ioctl() {
    ...
    r = vhost_get_vq_from_user(d, argp, &vq, &idx);
    ...
}

If a concurrent thread modifies the index in userspace between these two
fetches, could the vhost core end up updating a different queue than the one
vhost_vdpa_vring_ioctl() originally resolved? This might cause the vDPA
backend configuration (applied later in this function using the original
unverified vq and idx) to become desynchronized from the vhost core state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1