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

>
> On Sun, Aug 16, 2026 at 01:35:05PM +0800, Jia Jia wrote:
> > >
> > > 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'm not sure i understand. Alter the contract how?
>

If I understand your concern correctly, one reason for not simply
returning -EBUSY is that vhost cannot predict how an arbitrary userspace
owner will react to the error, or what state this may leave the
application in.

However, the same concern seems to apply if the transition is accepted
but the kernel clears the IOTLB and the cached vring state. In that case,
the owner must issue VHOST_SET_VRING_ADDR again for every virtqueue, but
an arbitrary userspace owner may not know that this follow-up operation
is required either.


  The approach I am considering is approximately:

          old_iotlb = dev->iotlb;
          dev->iotlb = NULL;

          for_each_vq(vq) {
                  mutex_lock(&vq->mutex);
                  vq->iotlb = NULL;
                  vhost_vq_invalidate_access(vq);
                  mutex_unlock(&vq->mutex);
          }

          vhost_clear_msg(dev);
          vhost_iotlb_free(old_iotlb);
          wake_up readers;

  vhost_vq_invalidate_access() would clear:

          vq->desc
          vq->avail
          vq->used
          vq->log_used
          vq->log_addr
          vq->meta_iotlb[]

  The helper would also be used when initializing the device IOTLB:

          if (d->iotlb)
                  return 0;

          ...
          vq->iotlb = niotlb;
          vhost_vq_invalidate_access(vq);

  The idempotent initialization is needed so that a later feature update
  which keeps ACCESS_PLATFORM enabled does not replace the existing IOTLB
  with a new empty table.

  After a successful live transition, the backend would remain attached,
  but the cached vring addresses would be invalidated. Userspace would
  therefore need to call VHOST_SET_VRING_ADDR again for every virtqueue
  before data processing could resume. If ACCESS_PLATFORM is enabled
  again, userspace would also need to repopulate the new IOTLB through the
  normal MISS/UPDATE protocol.

  I checked the relevant QEMU code. QEMU does not normally withdraw
  ACCESS_PLATFORM in the middle of an active feature-negotiation session.
  If it needs to change that feature, it follows the reset/restart path,
  which reconfigures the vring addresses. Therefore QEMU already performs
  the required sequence. However, we cannot assume that every other
  userspace owner follows the same reset/restart behaviour.

  This does not add a new ioctl, and a successful VHOST_SET_FEATURES call
  would still return zero. The change is in the postcondition of a
  successful call: previously configured vring addresses would no longer
  be preserved across the transition.

  This is different from returning -EBUSY. -EBUSY rejects the transition
  and leaves the previous state intact, while this approach accepts the
  transition and requires a follow-up vring reconfiguration. Both choices
  may affect userspace behaviour, but in different ways.

  This is the userspace contract change I was referring to.

 This is the approach I can currently think of for safely handling the
transition even when a backend is attached.
 Please let me know if I have misunderstood anything.


> > 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
>