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

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

[Paolo Abeni]

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.

/P

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

[Stefano Garzarella]

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?


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

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

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

[Arseniy Krasnov]

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

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

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

[Stefano Garzarella]

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

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

[Arseniy Krasnov]

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
> 

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

[Bobby Eshleman]

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.

Best,
Bobby

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

[Stefano Garzarella]

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

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

[Stefano Garzarella]

On Thu, Apr 30, 2026 at 03:11:10PM +0800, 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(-)

Hi Yiqi Sun, thanks for this patch, but as I mentioned in the sub-thread 
I found another issue and also another way to fix this, please can you 
test the series I just sent: https://lore.kernel.org/netdev/20260508164411.261440-1-sgarzare@redhat.com/

Thanks,
Stefano

>
>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);
> }
>-- 
>2.34.1
>