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

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, May 07, 2026 at 11:03:57PM +0300, Arseniy Krasnov wrote:
>>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.

Thanks for confirming, I'll send a series soon and CC you.
Please review it :-)

Thanks,
Stefano