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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Thu, Oct 01, 2026 at 07:13:29PM +0200, 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, 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.
> +		 */

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 <jrmmhm.kernel@eldare.de>
> 
> 

The rest LGTM.

Best,
Bobby