linux-kernel - Re: [syzbot] [kvm?] [net?] [virt?] WARNING in virtio_transport_send_pkt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250816063301-mutt-send-email-mst@kernel.org>
Date: Sat, 16 Aug 2025 06:34:29 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Will Deacon <will@...nel.org>
Cc: syzbot <syzbot+b4d960daf7a3c7c2b7b1@...kaller.appspotmail.com>,
	davem@...emloft.net, edumazet@...gle.com, eperezma@...hat.com,
	horms@...nel.org, jasowang@...hat.com, kuba@...nel.org,
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, pabeni@...hat.com, sgarzare@...hat.com,
	stefanha@...hat.com, syzkaller-bugs@...glegroups.com,
	virtualization@...ts.linux.dev, xuanzhuo@...ux.alibaba.com
Subject: Re: [syzbot] [kvm?] [net?] [virt?] WARNING in
 virtio_transport_send_pkt_info

On Fri, Aug 15, 2025 at 04:48:00PM +0100, Will Deacon wrote:
> On Fri, Aug 15, 2025 at 01:00:59PM +0100, Will Deacon wrote:
> > On Fri, Aug 15, 2025 at 06:44:47AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Aug 15, 2025 at 11:09:24AM +0100, Will Deacon wrote:
> > > > On Tue, Aug 12, 2025 at 06:15:46AM -0400, Michael S. Tsirkin wrote:
> > > > > On Tue, Aug 12, 2025 at 03:03:02AM -0700, syzbot wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > syzbot has tested the proposed patch but the reproducer is still triggering an issue:
> > > > > > WARNING in virtio_transport_send_pkt_info
> > > > > 
> > > > > OK so the issue triggers on
> > > > > commit 6693731487a8145a9b039bc983d77edc47693855
> > > > > Author: Will Deacon <will@...nel.org>
> > > > > Date:   Thu Jul 17 10:01:16 2025 +0100
> > > > > 
> > > > >     vsock/virtio: Allocate nonlinear SKBs for handling large transmit buffers
> > > > >     
> > > > > 
> > > > > but does not trigger on:
> > > > > 
> > > > > commit 8ca76151d2c8219edea82f1925a2a25907ff6a9d
> > > > > Author: Will Deacon <will@...nel.org>
> > > > > Date:   Thu Jul 17 10:01:15 2025 +0100
> > > > > 
> > > > >     vsock/virtio: Rename virtio_vsock_skb_rx_put()
> > > > >     
> > > > > 
> > > > > 
> > > > > Will, I suspect your patch merely uncovers a latent bug
> > > > > in zero copy handling elsewhere.
> > 
> > I'm still looking at this, but I'm not sure zero-copy is the right place
> > to focus on.
> > 
> > The bisected patch 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs
> > for handling large transmit buffers") only has two hunks. The first is
> > for the non-zcopy case and the latter is a no-op for zcopy, as
> > skb_len == VIRTIO_VSOCK_SKB_HEADROOM and so we end up with a linear SKB
> > regardless.
> 
> It's looking like this is caused by moving from memcpy_from_msg() to
> skb_copy_datagram_from_iter(), which is necessary to handle non-linear
> SKBs correctly.
> 
> In the case of failure (i.e. faulting on the source and returning
> -EFAULT), memcpy_from_msg() rewinds the message iterator whereas
> skb_copy_datagram_from_iter() does not. If we have previously managed to
> transmit some of the packet, then I think
> virtio_transport_send_pkt_info() can end up returning a positive "bytes
> written" error code and the caller will call it again. If we've advanced
> the message iterator, then this can end up with the reported warning if
> we run out of input data.
> 
> As a hack (see below), I tried rewinding the iterator in the error path
> of skb_copy_datagram_from_iter() but I'm not sure whether other callers
> would be happy with that. If not, then we could save/restore the
> iterator state in virtio_transport_fill_skb() if the copy fails. Or we
> could add a variant of skb_copy_datagram_from_iter(), say
> skb_copy_datagram_from_iter_full(), which has the rewind behaviour.
> 
> What do you think?
> 
> Will

It is, at least, self-contained. I don't much like hacking around
it in virtio_transport_fill_skb. If your patch isn't acceptable,
skb_copy_datagram_from_iter_full seem like a better approach, I think.


> --->8
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 94cc4705e91d..62e44ab136b7 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -551,7 +551,7 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
>  				 int len)
>  {
>  	int start = skb_headlen(skb);
> -	int i, copy = start - offset;
> +	int i, copy = start - offset, start_off = offset;
>  	struct sk_buff *frag_iter;
>  
>  	/* Copy header. */
> @@ -614,6 +614,7 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
>  		return 0;
>  
>  fault:
> +	iov_iter_revert(from, offset - start_off);
>  	return -EFAULT;
>  }
>  EXPORT_SYMBOL(skb_copy_datagram_from_iter);