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

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

[Jerome Mohm via B4 Relay]

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, 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: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>
---
 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.
+		 */
+		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.
+	 */
+	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 <jrmmhm.kernel@eldare.de>

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

[sashiko-bot]

> 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, 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)
> [ ... ]
> 
> Fixes: bd50c5dc182b ("vsock/virtio: add support for device suspend/resume")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-vsock-restore-uaf-send-v1-1-5da60f707506@eldare.de?part=1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes only the RX/TX detach loops in
  virtio_vsock_vqs_del() (net/vmw_vsock/virtio_transport.c) tolerate a…
- [Low] The commit message says the change 'mirrors' virtio_rtc commit
  548d2208455f and that 'virtio_console carries the same stale-pointers-…

--

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

This patch clears vsock->vqs[] after del_vqs() in virtio_vsock_vqs_del().
It also clears the array when virtio_find_vqs() fails in
virtio_vsock_vqs_init(). The RX/TX detach loops are skipped when the vq
pointer is NULL, so a remove or freeze after a failed restore no longer
touches freed virtqueues.

> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 4f9aa9c4c3aa5..0553c0641fd28 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c

[ ... ]

> @@ -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);

[ ... ]

>  	/* Delete virtqueues and flush outstanding callbacks if any */
>  	vdev->config->del_vqs(vdev);

[Severity: Medium]
The detach loops now handle NULL vqs, but del_vqs() still runs
unconditionally. Is a second del_vqs() safe on every transport?

On virtio-mmio, vm_del_vqs() always frees the IRQ:

drivers/virtio/virtio_mmio.c:vm_del_vqs() {
    ...
	list_for_each_entry_safe(vq, n, &vdev->vqs, list)
		vm_del_vq(vq);

	if (vm_dev->wake_irq_enabled) {
		disable_irq_wake(irq);
		vm_dev->wake_irq_enabled = false;
	}

	free_irq(irq, vm_dev);
}

Take the sequence from the commit message:

virtio_vsock_freeze()
  virtio_vsock_vqs_del()
    vm_del_vqs()            <- vqs deleted, IRQ freed

virtio_vsock_restore()
  virtio_vsock_vqs_init()
    vm_find_vqs()           <- fails

vm_find_vqs() can fail in two ways:

- request_irq() fails, so the IRQ is never re-requested.
- vm_setup_vq() fails, and vm_find_vqs() calls vm_del_vqs() itself.

Either way the IRQ is no longer held. virtio_device_restore_priv() only sets
VIRTIO_CONFIG_S_FAILED, so the driver stays bound.

A later virtio_vsock_remove() or a second virtio_vsock_freeze() then reaches
virtio_vsock_vqs_del() again. The new NULL checks skip the detach loops, but
vm_del_vqs() calls free_irq(irq, vm_dev) for an IRQ that vm_dev no longer
owns.

Won't __free_irq() hit WARN(1, "Trying to free already-free IRQ %d") here?

The virtio-pci reproducer doesn't show this because vp_del_vqs() is
idempotent. Would it be better to skip the transport teardown in
virtio_vsock_vqs_del() when no vqs are live? An early return, or a flag
that records whether the vqs were set up, would both work.

> +
> +	/*
> +	 * 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.
> +	 */

[Severity: Low]
This isn't a bug, but the commit message says:

    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.

Are the virtio_rtc and virtio_console references accurate?

Commit 548d2208455f fixes a different problem. viortc_freeze() kept the old
vqs alive, so that commit calls del_vqs() at the start of viortc_restore().
It doesn't clear any driver pointers after del_vqs().

virtio_console doesn't seem to have this fix either. remove_vqs() calls
kfree() on portdev->in_vqs and portdev->out_vqs without clearing them:

drivers/char/virtio_console.c:remove_vqs() {
    ...
	portdev->vdev->config->del_vqs(portdev->vdev);
	kfree(portdev->in_vqs);
	kfree(portdev->out_vqs);
}

The free: error path in init_vqs(), which virtcons_restore() reaches, does
the same.

Only the virtio_blk commit matches what this patch does. Could the other
two references be corrected?

Also, the new comment in virtio_vsock_vqs_del() hard-codes a commit ID
("Mirrors virtio_blk commit 0739c2c6a015"). Could that history stay in
the changelog only?

> +	memset(vsock->vqs, 0, sizeof(vsock->vqs));
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-vsock-restore-uaf-send-v1-1-5da60f707506%40eldare.de