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

[Arseniy Krasnov <avkrasnov@salutedevices.com>]

On 08/05/2026 18:50, Stefano Garzarella wrote:
> 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 :-)

Sure, thanks!

> 
> Thanks,
> Stefano
>