> > 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 <[email protected]> > > > > > > > > > > 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/[email protected]/ > > > > > > > > > > > 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? >
Yes. After reproducing I already sent another patch: <[email protected]> > > > > > > > > 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/[email protected]/ > > > > 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/[email protected]?part=1 > > > >
