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

>
> 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 <[email protected]>
>
> 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?
>

 I was able to reproduce the vring issue described here with a userspace test.

 A complete fix may require broader changes and could alter the
existing userspace contract,
so I am not sure that the additional scope is justified. The
conservative approach

I am considering is to allow the transition only when no VQ backend is attached:
clear the IOTLB, invalidate the cached vring addresses, and require
userspace to configure them again.
If a backend is attached, the helper would return -EBUSY before
modifying any state.

This differs from the suggestion in
<[email protected]> to discard
the IOTLB and let the kernel rebuild it. I am unsure whether returning
-EBUSY while a backend is active is compatible
with existing userspace flows, or whether live transitions should
instead be preserved by quiescing the queues first.

I plan to prepare v6 along this conservative line, but I would
appreciate guidance on whether this is the right approach and
whether -EBUSY for an active backend is acceptable.

Thanks.

> [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/[email protected]?part=2