Re: [PATCH v8] vhost: invalidate vring access on IOTLB transitions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Concurrent execution of `vhost_clear_device_iotlb()` and a vhost worker thread calling `translate_desc()` results in a NULL pointer dereference or arbitrary host memory access due to lockless clearing of `dev->iotlb`.
--

--- Patch [1]: [PATCH v8] vhost: invalidate vring access on IOTLB transitions ---
commit 601cfcb65f9457e49ce60311ed18a7ca3496a8ba
Author: Jia Jia <[email protected]>

vhost: invalidate vring access on IOTLB transitions

This commit introduces a helper to clear the device IOTLB when the
VIRTIO_F_ACCESS_PLATFORM feature is cleared. It drops the device-wide
view first and then drops the per-VQ views under their respective mutexes.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
>
[ ... ]
>
> +/* Caller must hold the device mutex. */
> +void vhost_clear_device_iotlb(struct vhost_dev *d)
> +{
> +	struct vhost_iotlb *iotlb;
> +	int i;
> +
> +	iotlb = d->iotlb;
> +	if (!iotlb)
> +		return;
> +
> +	/*
> +	 * Drop the device-wide view first.  Each VQ then drops its
> +	 * per-VQ view and its cached ring access under its own mutex.
> +	 * Keep the old table alive until every VQ has completed this
> +	 * handoff, since a worker may still be using it while waiting
> +	 * for its VQ mutex.
> +	 */
> +	d->iotlb = NULL;

[Severity: Critical]
Could setting d->iotlb = NULL here race with a running vhost worker thread?

While this update is protected by dev->mutex, it does not hold vq->mutex.
If a worker thread is concurrently calling translate_desc(), it evaluates
the device IOTLB locklessly:

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

Since dev->iotlb is cleared without vq->mutex, could the worker erroneously
fall back to dev->umem?

If dev->umem is NULL, this looks like it could trigger a NULL pointer
dereference when accessing umem->root. If dev->umem is populated, could
it map GIOVAs using the GPA table and lead to arbitrary host memory access?

> +
> +	for (i = 0; i < d->nvqs; ++i) {
> +		struct vhost_virtqueue *vq = d->vqs[i];
> +
> +		mutex_lock(&vq->mutex);
> +		vq->iotlb = NULL;
> +		vhost_vq_invalidate_access(vq);
> +		mutex_unlock(&vq->mutex);
> +	}
> +
> +	vhost_clear_msg(d);
> +	vhost_iotlb_free(iotlb);
> +	wake_up_interruptible_poll(&d->wait, EPOLLIN | EPOLLRDNORM);
> +}
> +EXPORT_SYMBOL_GPL(vhost_clear_device_iotlb);

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