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 13:40:33 +0100
From: David Howells <dhowells@...hat.com>
To: 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)

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.

David