Re: [PATCH v4 2/3] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Arbitrary kernel write and privilege escalation due to bypassed `access_ok()` bounds checking when VIRTIO_F_ACCESS_PLATFORM is cleared dynamically.
- [High] Data race causing NULL pointer dereference in `translate_desc()` due to lockless `dev->iotlb` teardown.

Pre-existing issues:
- [High] The vhost-net subsystem suffers from the exact same bug that this patch fixes in vhost_vsock, but `vhost_net_set_features()` was not updated.
--

--- Patch [2]: [PATCH v4 2/3] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared ---
commit e884f4b90201c9e0d904641890e2f9e4ea704c9a
Author: Jia Jia <physicalmtea@gmail.com>

vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

This commit addresses an issue in vhost_vsock_set_features where the device
iotlb remains attached when userspace clears VIRTIO_F_ACCESS_PLATFORM. It
introduces a call to vhost_clear_device_iotlb to detach the iotlb and clear
the metadata cache for each virtqueue. This prevents descriptors from using
stale translations installed prior to the feature change.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 86f25ff80722d..a18fe1335349b 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
[ ... ]
> @@ -882,6 +882,10 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  		goto err;
>  	}
>  
> +	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
> +	    vsock->dev.iotlb)
> +		vhost_clear_device_iotlb(&vsock->dev);

[Severity: Critical]
If userspace clears the VIRTIO_F_ACCESS_PLATFORM feature while the device is
running, can this allow arbitrary writes to host kernel memory?

When VIRTIO_F_ACCESS_PLATFORM is enabled, vq_access_ok() intentionally
returns true without validating addresses, deferring validation to the
iotlb.

If userspace provides arbitrary kernel addresses via VHOST_SET_VRING_ADDR,
starts the device, and then dynamically clears the feature,
vhost_clear_device_iotlb() sets vq->iotlb to NULL on the fly.

Since the existing vring addresses are never re-validated, a concurrent
worker thread calling vhost_put_used() -> vhost_copy_to_user() will
observe !vq->iotlb and fall back to __copy_to_user().

Because __copy_to_user() omits bounds checking, it will write to the
previously configured unvalidated kernel addresses.

Should the vring addresses be re-validated, or the device stopped, before
dropping the iotlb?

[Severity: High]
Could clearing the iotlb here create a data race with concurrent worker
threads?

Looking at vhost_clear_device_iotlb() in drivers/vhost/vhost.c, it
locklessly sets d->iotlb to NULL before acquiring vq->mutex for the
virtqueues:

drivers/vhost/vhost.c:vhost_clear_device_iotlb() {
    iotlb = d->iotlb;
    d->iotlb = NULL;

    for (i = 0; i < d->nvqs; ++i) {
        struct vhost_virtqueue *vq = d->vqs[i];
        mutex_lock(&vq->mutex);
        ...
}

If a worker thread is concurrently executing vhost_copy_to_user() ->
translate_desc(), it holds vq->mutex. The ioctl thread will block on
the mutex but has already cleared d->iotlb locklessly.

In translate_desc(), the code evaluates the iotlb pointer:

drivers/vhost/vhost.c:translate_desc() {
    ...
    struct vhost_iotlb *umem = dev->iotlb ? dev->iotlb : dev->umem;
    ...
}

It observes dev->iotlb as NULL and falls back to dev->umem. Since
dev->umem is NULL when using an iotlb, passing umem to
vhost_iotlb_itree_first() will unconditionally dereference iotlb->root,
crashing the host kernel.

Would it be safer to clear d->iotlb after the virtqueues are quiesced?

[Severity: High]
This is a pre-existing issue, but is vhost_net_set_features() in
drivers/vhost/net.c missing the exact same cleanup introduced here?

When userspace clears VIRTIO_F_ACCESS_PLATFORM via the
VHOST_SET_FEATURES ioctl on a vhost-net device, n->dev.iotlb is left
attached. This completely misses the newly introduced
vhost_clear_device_iotlb() cleanup.

Should the identical fix be applied to vhost-net to prevent memory
isolation bypasses via stale translation entries?

> +
>  	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) {
>  		if (vhost_init_device_iotlb(&vsock->dev))
>  			goto err;

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