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