netdev - Re: Endless loop in udp with MSG_SPLICE_READ - Re: [syzbot] [fs?] INFO: task hung in pipe

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 01 Aug 2023 08:58:16 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: David Howells <dhowells@...hat.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: dhowells@...hat.com, 
 Jakub Kicinski <kuba@...nel.org>, 
 syzbot <syzbot+f527b971b4bdc8e79f9e@...kaller.appspotmail.com>, 
 bpf@...r.kernel.org, 
 brauner@...nel.org, 
 davem@...emloft.net, 
 dsahern@...nel.org, 
 edumazet@...gle.com, 
 linux-fsdevel@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 netdev@...r.kernel.org, 
 pabeni@...hat.com, 
 syzkaller-bugs@...glegroups.com, 
 viro@...iv.linux.org.uk
Subject: Re: Endless loop in udp with MSG_SPLICE_READ - Re: [syzbot] [fs?]
 INFO: task hung in pipe_release (4)

David Howells wrote:
> The more I look at __ip_append_data(), the more I think the maths is wrong.
> In the bit that allocates a new skbuff:
> 
> 	if (copy <= 0) {
> 	...
> 		datalen = length + fraggap;
> 		if (datalen > mtu - fragheaderlen)
> 			datalen = maxfraglen - fragheaderlen;
> 		fraglen = datalen + fragheaderlen;
> 		pagedlen = 0;
> 	...
> 		if ((flags & MSG_MORE) &&
> 		    !(rt->dst.dev->features&NETIF_F_SG))
> 	...
> 		else if (!paged &&
> 			 (fraglen + alloc_extra < SKB_MAX_ALLOC ||
> 			  !(rt->dst.dev->features & NETIF_F_SG)))
> 	...
> 		else {
> 			alloclen = fragheaderlen + transhdrlen;
> 			pagedlen = datalen - transhdrlen;
> 		}
> 	...
> 
> In the MSG_SPLICE_READ but not MSG_MORE case, we go through that else clause.
> The values used here, a few lines further along:
> 
> 		copy = datalen - transhdrlen - fraggap - pagedlen;
> 
> are constant over the intervening span.  This means that, provided the splice
> isn't going to exceed the MTU on the second fragment, the calculation of
> 'copy' can then be simplified algebraically thus:
> 
> 		copy = (length + fraggap) - transhdrlen - fraggap - pagedlen;
> 
> 		copy = length - transhdrlen - pagedlen;
> 
> 		copy = length - transhdrlen - (datalen - transhdrlen);
> 
> 		copy = length - transhdrlen - datalen + transhdrlen;
> 
> 		copy = length - datalen;
> 
> 		copy = length - (length + fraggap);
> 
> 		copy = length - length - fraggap;
> 
> 		copy = -fraggap;
> 
> I think we might need to recalculate copy after the conditional call to
> getfrag().  Possibly we should skip that entirely for MSG_SPLICE_READ.  The
> root seems to be that we're subtracting pagedlen from datalen - but probably
> we shouldn't be doing getfrag() if pagedlen > 0.

q