Re: [PATCH v4 1/3] vhost: add helper to clear device IOTLB

>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Setting `d->iotlb = NULL` before acquiring `vq->mutex` in `vhost_clear_device_iotlb` creates a race condition with active vhost worker threads. This exposes the host kernel to a NULL pointer dereference or an IOMMU bypass (TOCTOU vulnerability) leading to arbitrary host memory corruption.
> --
>
> --- Patch [1]: [PATCH v4 1/3] vhost: add helper to clear device IOTLB ---
> commit 721258d5c0d5f3c1aeaefcc8dc997b5bbedcd9c1
> Author: Jia Jia <[email protected]>
>
> vhost: add helper to clear device IOTLB
>
> This patch adds a common helper for tearing down the device IOTLB shared
> by vhost backends. It drops each virtqueue's IOTLB pointer and metadata
> cache before freeing the old table.
>
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index a0c1d54019aac..0861406f3246f 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> [ ... ]
> > @@ -2298,6 +2298,30 @@ 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;
> > +     d->iotlb = NULL;
> > +
> > +     for (i = 0; i < d->nvqs; ++i) {
>
> [Severity: High]
> Does setting d->iotlb = NULL before acquiring vq->mutex create a race
> condition with active vhost worker threads?
>
> If a worker thread is concurrently executing vhost_copy_to_user() while
> holding vq->mutex, it could call translate_desc(), which locklessly
> reads dev->iotlb:
>
>     struct vhost_iotlb *umem = dev->iotlb ? dev->iotlb : dev->umem;
>
> Since d->iotlb was already set to NULL by vhost_clear_device_iotlb(),
> the worker thread might mistakenly fall back to a potentially NULL
> dev->umem.
>
> This could bypass the IOTLB translation and expose the host to memory
> corruption via forged GIOVAs or result in a NULL pointer dereference.
>

This is a known issue and appears to be the same issue discussed in
<[email protected]>  I tried to reproduce
it, but was
unable to do so. Therefore, I retained the current approach.

> > +             struct vhost_virtqueue *vq = d->vqs[i];
> > +
> > +             mutex_lock(&vq->mutex);
> > +             vq->iotlb = NULL;
> > +             __vhost_vq_meta_reset(vq);
> > +             mutex_unlock(&vq->mutex);
> > +     }
> > +
> > +     vhost_clear_msg(d);
> > +     vhost_iotlb_free(iotlb);
> > +     wake_up_interruptible_poll(&d->wait, EPOLLIN | EPOLLRDNORM);
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1