linux-kernel - Re: [PATCH net v4 1/4] vsock/virtio: fix potential underflow in virtio_transport_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7oq7ejyud46smrlinz543xrczxyiz5bor62o7xpg6g4eiaa4ad@chcc25mgxc5q>
Date: Thu, 18 Dec 2025 10:19:42 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Melbin K Mathew <mlbnkm1@...il.com>
Cc: stefanha@...hat.com, kvm@...r.kernel.org, netdev@...r.kernel.org, 
	virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org, mst@...hat.com, 
	jasowang@...hat.com, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org
Subject: Re: [PATCH net v4 1/4] vsock/virtio: fix potential underflow in
 virtio_transport_get_credit()

On Wed, Dec 17, 2025 at 07:12:03PM +0100, Melbin K Mathew wrote:
>The credit calculation in virtio_transport_get_credit() uses unsigned
>arithmetic:
>
>  ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
>
>If the peer shrinks its advertised buffer (peer_buf_alloc) while bytes
>are in flight, the subtraction can underflow and produce a large
>positive value, potentially allowing more data to be queued than the
>peer can handle.
>
>Use s64 arithmetic for the subtraction and clamp negative results to
>zero, matching the approach already used in virtio_transport_has_space().
>
>Fixes: 06a8fc78367d ("VSOCK: Introduce virtio_vsock_common.ko")
>Suggested-by: Stefano Garzarella <sgarzare@...hat.com>
>Signed-off-by: Melbin K Mathew <mlbnkm1@...il.com>
>---
> net/vmw_vsock/virtio_transport_common.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index dcc8a1d5851e..d692b227912d 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -494,14 +494,25 @@ EXPORT_SYMBOL_GPL(virtio_transport_consume_skb_sent);
> u32 virtio_transport_get_credit(struct virtio_vsock_sock *vvs, u32 credit)
> {
> 	u32 ret;
>+	u32 inflight;
>+	s64 bytes;
>
> 	if (!credit)
> 		return 0;
>
> 	spin_lock_bh(&vvs->tx_lock);
>-	ret = vvs->peer_buf_alloc - (vvs->tx_cnt - vvs->peer_fwd_cnt);
>-	if (ret > credit)
>-		ret = credit;
>+
>+	/*
>+	 * Compute available space using s64 to avoid underflow if
>+	 * peer_buf_alloc < inflight bytes (can happen if peer shrinks
>+	 * its advertised buffer while data is in flight).
>+	 */
>+	inflight = vvs->tx_cnt - vvs->peer_fwd_cnt;
>+	bytes = (s64)vvs->peer_buf_alloc - inflight;

I'm really confused, in our previous discussion we agreed on re-using
virtio_transport_has_space(), what changend in the mean time?

Stefano

>+	if (bytes < 0)
>+		bytes = 0;
>+
>+	ret = (bytes > credit) ? credit : (u32)bytes;
> 	vvs->tx_cnt += ret;
> 	vvs->bytes_unsent += ret;
> 	spin_unlock_bh(&vvs->tx_lock);
>-- 
>2.34.1
>