>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.
>
>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
