Re: [PATCH] vsock/virtio: fix vsockmon info leak in non-linear tap, copy

[Arseniy Krasnov <avkrasnov@salutedevices.com>]

>CCing Arseniy and Bobby.

Thanks!

>
>On Tue, May 05, 2026 at 12:26:21PM +0200, Paolo Abeni wrote:
>>On 4/30/26 9:11 AM, Yiqi Sun wrote:
>>> vsockmon mirrors packets through virtio_transport_build_skb(), which
>>> builds a new skb and copies the payload into it. For non-linear skbs,
>>> this goes through virtio_transport_copy_nonlinear_skb().
>>>
>>> Helper manually initializes a iov_iter, but leaves iov_iter.count unset.
>>> As a result, skb_copy_datagram_iter() sees zero writable bytes
>>> in the destination iterator and copies no payload data.
>>>
>>> This becomes an info leak because virtio_transport_build_skb() has
>>> already reserved payload_len bytes in the new skb with skb_put(). The
>>> skb is then returned to the tap path with that payload area still
>>> uninitialized, so userspace reading from a vsockmon device can observe
>>> heap contents and potentially kernel address.
>>>
>>> Fix it by initializing iov_iter.count to the number of bytes to copy.
>>>
>>> Fixes: 4b0bf10eb077 ("vsock/virtio: non-linear skb handling for tap")
>>> Signed-off-by: Yiqi Sun <sunyiqixm@gmail.com>
>>> ---
>>>  net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index 416d533f493d..6b26ee57ccab 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -152,7 +152,7 @@ static void virtio_transport_copy_nonlinear_skb(const struct sk_buff *skb,
>>>  	iov_iter.nr_segs = 1;
>>>
>>>  	to_copy = min_t(size_t, len, skb->len);
>>> -
>>> +	iov_iter.count = to_copy;
>>>  	skb_copy_datagram_iter(skb, VIRTIO_VSOCK_SKB_CB(skb)->offset,
>>>  			       &iov_iter, to_copy);
>>
>>@Stefano, @Stefan, the patch LGTM, but sashiko pointed out to a
>>pre-existing issue you should probably want to address:
>>
>>>  	to_copy = min_t(size_t, len, skb->len);
>>Does this length calculation account for the offset when a packet is
>>split across multiple transmissions?
>>If a packet is requeued, VIRTIO_VSOCK_SKB_CB(skb)->offset is increased,
>>but to_copy still evaluates to the full length of the skb.
>
>Yep, I just checked and vhost-vsock is the only place where we call 
>virtio_transport_deliver_tap_pkt() wiht an offset != 0, but I agree that 
>we should also fix it.

Yes, looks like the only place where offset could be non zero is 'vhost_transport_do_send_pkt()'.
And we set valid length in header every attempt to send it:

                /* Set the correct length in the header */
                hdr->len = cpu_to_le32(payload_len);

In all other places we call 'virtio_transport_deliver_tap_pkt()' with offset == 0. And thus
skb->len == hdr->len.

So for me looks ok. E.g. len in header is actual data.

>
>Looking better in net/vmw_vsock/virtio_transport_common.c I think this 
>is a regression, indeed we have this comment in 
>virtio_transport_build_skb():
>
>	/* A packet could be split to fit the RX buffer, so we can retrieve
>	 * the payload length from the header and the buffer pointer taking
>	 * care of the offset in the original packet.
>	 */
>	pkt_hdr = virtio_vsock_hdr(pkt);
>
>Before commit 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with 
>sk_buff") we read the payload lenght from the header that is always set 
>to the right value before delivering the packet to the tap.
>
> From that commit, we don't to consider the offset anymore since we 
>started to use `len` from the skb, so IMO we should go back to what we 
>did before it, I mean:
>
>	payload_len = le32_to_cpu(pkt->hdr.len);
>
>@Bobby do you remember why we did that change? Or if you see any issue 
>going back to what we did initially?
>
>
>Also IMO we should avoid to set all the iov_iter fields by hand and 
>start to use iov_iter_kvec(). Plus, we can just use 
>skb_copy_datagram_iter() in any case, like we already do in vhost-vsock, 
>since it already handles linear vs non linear.
>
>At the end I mean something like this:
>
>@@ -171,7 +150,7 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque)
>  	 * care of the offset in the original packet.
>  	 */
>  	pkt_hdr = virtio_vsock_hdr(pkt);
>-	payload_len = pkt->len;
>+	payload_len = le32_to_cpu(pkt_hdr->len);
>
>  	skb = alloc_skb(sizeof(*hdr) + sizeof(*pkt_hdr) + payload_len,
>  			GFP_ATOMIC);
>@@ -214,13 +193,17 @@ static struct sk_buff *virtio_transport_build_skb(void *opaque)
>  	skb_put_data(skb, pkt_hdr, sizeof(*pkt_hdr));
>
>  	if (payload_len) {
>-		if (skb_is_nonlinear(pkt)) {
>-			void *data = skb_put(skb, payload_len);
>+		struct iov_iter iov_iter;
>+		struct kvec kvec;
>+		void *data = skb_put(skb, payload_len);
>
>-			virtio_transport_copy_nonlinear_skb(pkt, data, payload_len);
>-		} else {
>-			skb_put_data(skb, pkt->data, payload_len);
>-		}
>+		kvec.iov_base = data;
>+		kvec.iov_len = payload_len;
>+		iov_iter_kvec(&iov_iter, READ, &kvec, 1, payload_len);
>+
>+		skb_copy_datagram_iter(pkt,
>+				       VIRTIO_VSOCK_SKB_CB(pkt)->offset,
>+				       &iov_iter, payload_len);
>  	}
>
>  	return skb;
>
>And removing virtio_transport_copy_nonlinear_skb().

Yes, this looks shorter and better.

>
>If you agree, I can send a proper series with these changes that should 
>fix the issue reported by Yiqi Sun introduced by commit 4b0bf10eb077 
>("vsock/virtio: non-linear skb handling for tap") and the issue 
>introduced by commit 71dc9ec9ac7d ("virtio/vsock: replace 
>virtio_vsock_pkt with sk_buff").
>
>Thanks,
>Stefano

Thanks