On Thu, Oct 01, 2026 at 07:13:29PM +0200, Jerome Mohm via B4 Relay wrote: > From: Jerome Mohm <[email protected]> > > virtio_vsock_vqs_del() frees the virtqueues via del_vqs() but never > clears vsock->vqs[]. If a later virtio_vsock_restore() fails to > reallocate them (its error path is a bare "goto out" that only unlocks > and returns), the driver is left bound with a dangling vqs[]. The next > virtio_vsock_vqs_del() - on a second freeze or on remove/unbind - then > passes a freed vring_virtqueue to virtqueue_detach_unused_buf(), a slab > use-after-free. KASAN on a guest with a virtio-vsock device, after a > restore made to fail by fault injection: > > BUG: KASAN: slab-use-after-free in virtqueue_detach_unused_buf > Read of size 4 at addr ... by task repro > virtqueue_detach_unused_buf > virtio_vsock_vqs_del (net/vmw_vsock/virtio_transport.c:785) > virtio_vsock_remove > virtio_dev_remove > ... unbind_store > Freed by task ...: > kfree > vp_del_vqs > virtio_vsock_freeze <- the preceding freeze freed the vq > Allocated by task 1: > vring_create_virtqueue > virtio_vsock_vqs_init > virtio_vsock_probe <- original allocation at probe > > The object is the RX vring_virtqueue freed during the freeze; > vsock->vqs[RX] still points at it. virtqueue_detach_unused_buf() does > not guard a NULL vq, so a bare NULL-out is not enough on its own. > > Clear vsock->vqs[] after del_vqs() in virtio_vsock_vqs_del(), skip the > detach loops when a vq pointer is NULL, and also clear the array when > virtio_find_vqs() fails partway in virtio_vsock_vqs_init() (which can > leave freed pointers behind). This mirrors virtio_blk > commit 0739c2c6a015 ("virtio_blk: NULL out vqs to avoid double free on > failed resume") and virtio_rtc commit 548d2208455f ("virtio: rtc: tear > down old virtqueues before restore"); virtio_console carries the same > stale-pointers-after-failed-restore fix. > > Testing: reproduced on a private VM with a KASAN fuzz kernel based on > v7.3-rc5. The restore allocation failure was forced with CONFIG_FAILSLAB > steered by the fault-injection stacktrace filter to the virtio-vsock > restore path only; a guest-root program drives a pm_test=devices > freeze/restore cycle (so .restore returns -ENOMEM) and then unbinds the > device. The unfixed kernel reports the slab-use-after-free above; with > this patch the same run makes restore fail identically yet produces no > KASAN report. checkpatch.pl --strict is clean and the build is > warning-free. The reproducer is available on request. > > Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume") > Cc: [email protected] > Assisted-by: LLM > Signed-off-by: Jerome Mohm <[email protected]> > --- > net/vmw_vsock/virtio_transport.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport.c > b/net/vmw_vsock/virtio_transport.c > index 4f9aa9c4c3aa..0553c0641fd2 100644 > --- a/net/vmw_vsock/virtio_transport.c > +++ b/net/vmw_vsock/virtio_transport.c > @@ -714,8 +714,15 @@ static int virtio_vsock_vqs_init(struct virtio_vsock > *vsock) > atomic_set(&vsock->queued_replies, 0); > > ret = virtio_find_vqs(vdev, VSOCK_VQ_MAX, vsock->vqs, vqs_info, NULL); > - if (ret < 0) > + if (ret < 0) { > + /* > + * On a partial failure virtio_find_vqs() can leave freed > + * virtqueue pointers in vsock->vqs[]; clear them so a later > + * virtio_vsock_vqs_del() does not detach a freed virtqueue. > + */
IMHO we can drop this comment. Clearing after failure is common enough. > + memset(vsock->vqs, 0, sizeof(vsock->vqs)); > return ret; > + } > > virtio_vsock_update_guest_cid(vsock); > > @@ -782,19 +789,30 @@ static void virtio_vsock_vqs_del(struct virtio_vsock > *vsock) > virtio_reset_device(vdev); > > mutex_lock(&vsock->rx_lock); > - while ((skb = virtqueue_detach_unused_buf(vsock->vqs[VSOCK_VQ_RX]))) > - kfree_skb(skb); > + if (vsock->vqs[VSOCK_VQ_RX]) > + while ((skb = > virtqueue_detach_unused_buf(vsock->vqs[VSOCK_VQ_RX]))) > + kfree_skb(skb); > mutex_unlock(&vsock->rx_lock); > > mutex_lock(&vsock->tx_lock); > - while ((skb = virtqueue_detach_unused_buf(vsock->vqs[VSOCK_VQ_TX]))) > - kfree_skb(skb); > + if (vsock->vqs[VSOCK_VQ_TX]) > + while ((skb = > virtqueue_detach_unused_buf(vsock->vqs[VSOCK_VQ_TX]))) > + kfree_skb(skb); > mutex_unlock(&vsock->tx_lock); > > virtio_vsock_skb_queue_purge(&vsock->send_pkt_queue); > > /* Delete virtqueues and flush outstanding callbacks if any */ > vdev->config->del_vqs(vdev); > + > + /* > + * del_vqs() has freed the virtqueues. Clear the stale pointers: if a > + * later virtio_vsock_restore() fails to allocate new ones, the driver > + * stays bound with a dangling vqs[] and the next virtio_vsock_vqs_del() > + * would detach a freed virtqueue (use-after-free). Mirrors virtio_blk > + * commit 0739c2c6a015. > + */ Ditto here. > + memset(vsock->vqs, 0, sizeof(vsock->vqs)); > } > > static int virtio_vsock_probe(struct virtio_device *vdev) > > --- > base-commit: a7bfaba4823e3c165bb2004c74eff7c096672bc7 > change-id: 20261001-vsock-restore-uaf-send-1fc7396fa1bd > > Best regards, > -- > Jerome Mohm <[email protected]> > > The rest LGTM. Best, Bobby
