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