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

[Stefano Garzarella <sgarzare@redhat.com>]

On Thu, May 07, 2026 at 01:12:35PM -0700, Bobby Eshleman wrote:
>On Tue, May 05, 2026 at 02:44:16PM +0200, Stefano Garzarella wrote:
>> CCing Arseniy and Bobby.
>>
>> 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.
>>
>> 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?
>
>I think this was just one that made it through the cracks. I vaguely
>recall a few other instances where I assumed skb->len could stand-in for
>hdr.len, but it didn't hold.
>
>Using hdr.len like the original looks correct to me.

Thanks for confirming, I'll fix it.

Stefano