Re: [PATCH] vhost-scsi: do not relock event vq mutex on send_evt fallback

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 15, 2026 at 09:30:27PM +0800, Jia Jia wrote:
> >
> > On Mon, Sep 14, 2026 at 04:38:57PM +0800, Jia Jia wrote:
> > > >
> > > > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > > >
> > > > Pre-existing issues:
> > > > - [Critical] vhost_scsi_do_evt_work() accesses vq->iov[out] without verifying that the guest actually provided an input buffer, leading to arbitrary user-space memory corruption.
> > > > - [High] Events are processed in LIFO order instead of FIFO, which can cause state corruption for hotplug/hotunplug sequences.
> > > > --
> > > >
> > > > commit 05830c30fc2e4dc682c270a9b487ae5abb66218b
> > > > Author: Jia Jia <physicalmtea@gmail.com>
> > > >
> > > > vhost-scsi: do not relock event vq mutex on send_evt fallback
> > > >
> > > > This commit resolves a deadlock during fallback event handling when the vhost
> > > > worker has disconnected. It splits event completion into an internal unlocked
> > > > helper and a locked wrapper to safely reuse the completion path.
> > > >
> > > > Link: https://lore.kernel.org/all/20260905005352.1E5B01F00A3D@smtp.kernel.org/
> > > >
> > > > > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > > > > index 4f8c0260bc9e7..42abf18291b75 100644
> > > > > --- a/drivers/vhost/scsi.c
> > > > > +++ b/drivers/vhost/scsi.c
> > > > > @@ -660,19 +660,26 @@ vhost_scsi_do_evt_work(struct vhost_scsi *vs, struct vhost_scsi_evt *evt)
> > > > >       vhost_scsi_log_write(vq, vq_log, log_num);
> > > > >  }
> > > > >
> > > > > -static void vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
> > > > > +/* Caller must hold the event virtqueue mutex. */
> > > > > +static void __vhost_scsi_complete_events(struct vhost_scsi *vs, bool drop)
> > > > >  {
> > > > > -     struct vhost_virtqueue *vq = &vs->vqs[VHOST_SCSI_VQ_EVT].vq;
> > > > >       struct vhost_scsi_evt *evt, *t;
> > > > >       struct llist_node *llnode;
> > > > >
> > > > > -     mutex_lock(&vq->mutex);
> > > > >       llnode = llist_del_all(&vs->vs_event_list);
> > > > >       llist_for_each_entry_safe(evt, t, llnode, list) {
> > > >
> > > > [Severity: High]
> > > > This is a pre-existing issue, but does this loop deliver events to the guest
> > > > in reverse order?
> > > >
> > > > The events are added to vs_event_list using llist_add(), which constructs a
> > > > last-in, first-out list. Because lnode is retrieved locklessly with
> > > > llist_del_all(), and the iteration happens without first calling
> > > > llist_reverse_order(), the events appear to be processed from newest to
> > > > oldest.
> > > >
> > > > Could this cause state corruption for hotplug and hotunplug sequences? For
> > > > example, if a device is plugged and then rapidly unplugged, the guest might
> > > > receive the unplug event first and the plug event second, leaving it
> > > > erroneously believing the device is still present.
> > > >
> > >
> > > Looks like this could get the acquire order wrong? Need to verify.
> >
> >
> > didn't look into this yet - did u reach any conclusion?
> >
> 
> Already reproduced this using a test program. To hit what Sashiko AI described,
> need multiple vhost_scsi_send_evt->llist_add (at least 2) before the
> worker schedules
> vhost_scsi_complete_events(false).
> 
> I personally think this is not very easy to hit, first this kind of
> hotplug/unplug event is not very frequent,
> even if there are multiple calls, as long as the worker is idle it
> will run vhost_scsi_complete_events(false)
> quickly, so pending is often only 1.
> 
> So to reproduce, I created multiple LUNs, and concurrently called 8 ln
> -s (only triggers RESCAN,
> no unplug, extra LUNs are just so we can see the lun numbers better).
> This increases the chance that
> vhost_scsi_send_evt races in before vhost_scsi_complete_events(false)
> runs, so there are at least 2+ pending llist.
> Finally when vhost_scsi_complete_events(false) runs, the guest sees
> the later-linked LUN first.
> (before testing I thought this needed a lot of SCSI I/O to reproduce,
> actually it doesn't)
> I only tested plug, the guest did see add LUN order reversed.

okay... so llist_reverse_order ? Will that fix it?

> 
> > > > >               if (!drop)
> > > > >                       vhost_scsi_do_evt_work(vs, evt);
> > > >
> > > > [Severity: Critical]
> > > > This is a pre-existing issue, but can this function access out-of-bounds or
> > > > stale memory if the guest provides an empty input buffer?
> > > >
> > > > Looking at vhost_scsi_do_evt_work() in drivers/vhost/scsi.c, the code uses
> > > > vhost_get_vq_desc() which populates vq->iov:
> > > >
> > > >     head = vhost_get_vq_desc(vq, vq->iov,
> > > >             ARRAY_SIZE(vq->iov), &out, &in,
> > > >             vq_log, &log_num);
> > > >     ...
> > > >     if ((vq->iov[out].iov_len != sizeof(struct virtio_scsi_event))) {
> > > >         vq_err(vq, "Expecting virtio_scsi_event, got %zu bytes\n",
> > > >                 vq->iov[out].iov_len);
> > > >
> > > > If a guest submits a descriptor chain with only read-only descriptors,
> > > > vhost_get_vq_desc() sets in = 0. The out variable holds the count of
> > > > read-only descriptors, meaning the first writable descriptor expected by the
> > > > host would be at index out.
> > > >
> > >
> > > This is a dup -- an earlier patch already fixed it:
> > > https://lore.kernel.org/all/20260904005240.502787-1-physicalmtea@gmail.com/
> > > It was while fixing that earlier issue that Sashiko spotted this
> > > adjacent problem.
> > >
> > > > Does the length check above access vq->iov[out].iov_len without verifying
> > > > that in > 0?
> > > >
> > > > If in is 0, vq->iov[out] was not populated by vhost_get_vq_desc() and
> > > > contains stale data from a previous request. Could a malicious guest use
> > > > this to force the kernel to write the event structure to a previously
> > > > legitimate but now hot-unplugged host virtual address via the
> > > > __copy_to_user() call later in the function?
> > > >
> > > > >               vhost_scsi_free_evt(vs, evt);
> > > > >       }
> > > > > +}
> > > >
> > > > [ ... ]
> > > >
> > > > --
> > > > Sashiko AI review · https://sashiko.dev/#/patchset/20260912090420.47439-1-physicalmtea@gmail.com?part=1
> >