Re: [PATCH net v2] vsock/virtio: clear vqs[] to avoid a UAF on a failed PM restore

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

On Fri, Oct 30, 2026 at 02:51:26PM +0100, Stefano Garzarella wrote:
> On Wed, Oct 28, 2026 at 07:22:43PM +0100, Jerome Mohm via B4 Relay wrote:
> > From: Jerome Mohm <jrmmhm.kernel@eldare.de>
> > 
> > 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 on
> > virtio-pci, 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
> > 
> > That second virtio_vsock_vqs_del() also repeats del_vqs(), which not
> > every transport tolerates. virtio-mmio's vm_del_vqs() always ends in
> > free_irq(), but after the failed vm_find_vqs() the device no longer
> > holds its IRQ, so on virtio-mmio the second call also warns:
> > 
> >  Trying to free already-free IRQ 51
> >  WARNING: kernel/irq/manage.c:1894 at free_irq+0x23e/0x840
> >   vm_del_vqs
> >   virtio_vsock_vqs_del
> >   virtio_vsock_remove          (or virtio_vsock_freeze)
> > 
> > virtio-pci's vp_del_vqs() is idempotent, so the PCI case shows only
> > the use-after-free.
> > 
> > Clear vsock->vqs[] after del_vqs() and when virtio_find_vqs() fails in
> > virtio_vsock_vqs_init(), and treat an empty vqs[] as "no virtqueues
> > are live": virtio_vsock_vqs_del() then still resets the device
> > (virtio_dev_remove() expects a reset device) and purges the send
> > queue, but skips the detach loops and del_vqs(). find_vqs() cleans up
> > after itself on failure, so skipping the second del_vqs() leaks
> > nothing. Clearing the stale pointers follows virtio_blk
> > commit 0739c2c6a015 ("virtio_blk: NULL out vqs to avoid double free on
> > failed resume").
> > 
> > The use-after-free was found with the help of an LLM coding assistant
> > (Anthropic's Claude); the free_irq() warning on virtio-mmio was reported
> > by the netdev Sashiko AI review of an earlier version of this patch.
> > 
> > Testing: on a private VM with a KASAN fuzz kernel, over virtio-pci and
> > over virtio-mmio (QEMU microvm). 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, or runs a second freeze/restore
> > cycle. Without a fix (v7.3-rc5 based) the PCI run reports the
> > slab-use-after-free above. With only the vqs[] clearing (v1 of this
> > patch, on net 363c8c79220d) the MMIO runs report the free_irq warning on
> > remove and on the second freeze. With this patch on the same base,
> > restore still fails with -ENOMEM on both transports and neither report
> > appears; the device works again after a second, successful
> > freeze/restore cycle and after unbind/bind, and a normal freeze/restore
> > and unbind/bind still leave a working device. A find_vqs() failure after
> > some virtqueues were set up was not exercised (each restore attempt
> > failed at its first allocation, before any virtqueue was set up); that
> > clear is covered by code reading only. The three single-VM tests of
> > tools/testing/selftests/vsock/vmtest.sh (vm_server_host_client,
> > vm_client_host_server, vm_loopback; 44 vsock_test cases each) pass on
> > this tree, as on net; on both kernels the host-side runs skip the SO_INQ
> > cases (SO_INQ not supported by the host kernel). The ns_* tests of
> > vmtest.sh were not run (they need root on the host). checkpatch.pl
> > --strict is clean and the fuzz-config build is warning-free. The
> 
> Do we really need to write in a commit message that checkpatch.pl --strict
> is clean?
> 
> Descriptive commits are great, but we're going a bit too far here.

It's just what AI writes if you tell it to make it checkpatch.pl
clean in the prompt.

> Please
> include only the relevant information; for example, I think the part after
> "Testing:" could be summarized in a few lines or removed altogether.


Yea please write it yourself and make it concise. My eyes glazed over
in the 1st paragraph.

> > reproducer needs guest root, CONFIG_FAILSLAB with
> > CONFIG_FAULT_INJECTION_DEBUG_FS and
> > CONFIG_FAULT_INJECTION_STACKTRACE_FILTER, and CONFIG_HIBERNATION with
> > CONFIG_PM_DEBUG for pm_test; it is available on request.
> > 
> > Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>
> > ---
> > The use-after-free was found with the help of Anthropic's Claude; the
> > virtio-mmio warning comes from the netdev Sashiko review.
> 
> This is already in the commit description, no?
> 
> > 
> > Changes in v2:
> > - Skip the detach loops and del_vqs() in virtio_vsock_vqs_del() when no
> >  virtqueues are live, so the teardown after a failed restore does not
> >  free virtio-mmio's IRQ a second time (Sashiko review).
> > - Use an empty vqs[] as the 'no live virtqueues' marker instead of v1's
> >  per-loop NULL checks, and purge the send queue before the early
> >  return.
> > - Commit message: cite only virtio_blk 0739c2c6a015; drop the wrong
> >  virtio_rtc and virtio_console references (Sashiko review). Drop the
> >  commit ID from the code comment.
> > - Tested on virtio-mmio (QEMU microvm) as well as virtio-pci.
> > - Drop the Assisted-by trailer (removed upstream); tool use is
> >  disclosed above instead.
> > - Link to v1: https://patch.msgid.link/20261001-vsock-restore-uaf-send-v1-1-5da60f707506@eldare.de
> > ---
> > net/vmw_vsock/virtio_transport.c | 24 +++++++++++++++++++++---
> > 1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index 4f9aa9c4c3aa..bba594b7fc14 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -714,8 +714,14 @@ 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 failure virtio_find_vqs() can leave freed virtqueues or
> > +		 * error pointers in vsock->vqs[]. Clear it: an empty vqs[]
> > +		 * tells virtio_vsock_vqs_del() that no virtqueues are live.
> > +		 */
> > +		memset(vsock->vqs, 0, sizeof(vsock->vqs));
> 
> Should we move this in the virtio_find_vqs() ?
> 
> Maybe not for this patch, but for long term it could be a good idea IMO.
> 
> > 		return ret;
> > +	}
> > 
> > 	virtio_vsock_update_guest_cid(vsock);
> > 
> > @@ -781,6 +787,15 @@ static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
> > 	 */
> > 	virtio_reset_device(vdev);
> > 
> > +	virtio_vsock_skb_queue_purge(&vsock->send_pkt_queue);
> > +
> > +	/* No virtqueues are live, e.g. after a failed restore: they were
> > +	 * already deleted, and a second del_vqs() is not safe on every
> > +	 * transport (virtio-mmio would free its IRQ again).
> > +	 */
> > +	if (!vsock->vqs[VSOCK_VQ_RX])
> > +		return;
> > +
> > 	mutex_lock(&vsock->rx_lock);
> > 	while ((skb = virtqueue_detach_unused_buf(vsock->vqs[VSOCK_VQ_RX])))
> > 		kfree_skb(skb);
> > @@ -791,10 +806,13 @@ static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
> > 		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);
> > +
> > +	/* The virtqueues are freed: clear the stale pointers, so that a
> > +	 * virtio_vsock_vqs_del() after a failed restore skips them.
> > +	 */
> > +	memset(vsock->vqs, 0, sizeof(vsock->vqs));
> 
> Same above, should we introduce a virtio_del_vqs() to do both and call it in
> all drivers?
> 
> Or skip del_vqs() if vdev->vqs is empty.
> 
> I guess this issue could be spread around virtio drivers, so would be nice
> to add common API in the virtio core to be used in all of them.
> 
> That said, for a fix, this patch LGTM, my comments were more for a long term
> work.
> 
> With a fixed commit message I can give my R-b.
> 
> Thanks,
> Stefano
> 
> > }
> > 
> > static int virtio_vsock_probe(struct virtio_device *vdev)
> > 
> > ---
> > base-commit: 363c8c79220d0db4d65dcd570529ddda15067cb5
> > change-id: 20261001-vsock-restore-uaf-send-1fc7396fa1bd
> > 
> > Best regards,
> > --
> > Jerome Mohm <jrmmhm.kernel@eldare.de>
> > 
> >