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 <[email protected]>
>>>>> ---
>>>>> 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
>
