Re: [PATCH v7 1/3] vhost: invalidate vring access on IOTLB transitions

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in vhost_clear_device_iotlb() causes NULL pointer dereference or IOMMU bypass
--

--- Patch [1]: [PATCH v7 1/3] vhost: invalidate vring access on IOTLB transitions ---
commit da0274e257afa43e8d844051613c8dcc1f9d744f
Author: Jia Jia <[email protected]>

vhost: invalidate vring access on IOTLB transitions

When ACCESS_PLATFORM changes, the addresses cached in desc, avail, and
used change meaning with the address space. Clear the cached vring access
state when the device IOTLB is installed or removed so stale IOVAs cannot
be reused as direct userspace addresses.

> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 14637cff0bd46..31fff98000450 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
[ ... ]
> @@ -2287,11 +2301,48 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
>  }
>  EXPORT_SYMBOL_GPL(vhost_vring_ioctl);
>  
> +/* 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: High]
Does clearing d->iotlb here without holding vq->mutex introduce a race
condition with the vhost worker thread?

If a userspace process dynamically clears the VIRTIO_F_ACCESS_PLATFORM
feature, this function clears d->iotlb without synchronization against
concurrent vhost worker threads.

Meanwhile, an active worker thread holding vq->mutex might be calling
translate_desc():

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

Because there is no READ_ONCE() or other synchronization on dev->iotlb, could
the compiler double-read dev->iotlb, observing a non-NULL value for the
condition but NULL for the assignment, leaving umem = NULL?

Alternatively, if dev->umem was never populated, reading dev->iotlb as NULL
also leaves umem as NULL.  The worker then calls vhost_iotlb_itree_first()
which dereferences umem->root, potentially causing a host kernel panic.

> +
> +	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);
> +	}

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