netdev - RE: [PATCH net v2] udp: Fix __ip{,6}_append_data()'s handling of MSG_SPLICE

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: <64c97684ee5e3_1d7aa3294da@willemb.c.googlers.com.notmuch>
Date: Tue, 01 Aug 2023 17:17:56 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: David Howells <dhowells@...hat.com>, 
 netdev@...r.kernel.org, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: dhowells@...hat.com, 
 syzbot+f527b971b4bdc8e79f9e@...kaller.appspotmail.com, 
 bpf@...r.kernel.org, 
 brauner@...nel.org, 
 davem@...emloft.net, 
 dsahern@...nel.org, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 axboe@...nel.dk, 
 viro@...iv.linux.org.uk, 
 linux-fsdevel@...r.kernel.org, 
 syzkaller-bugs@...glegroups.com, 
 linux-kernel@...r.kernel.org
Subject: RE: [PATCH net v2] udp: Fix __ip{,6}_append_data()'s handling of
 MSG_SPLICE_PAGES

David Howells wrote:
> __ip_append_data() can get into an infinite loop when asked to splice into
> a partially-built UDP message that has more than the frag-limit data and up
> to the MTU limit.  Something like:
> 
>         pipe(pfd);
>         sfd = socket(AF_INET, SOCK_DGRAM, 0);
>         connect(sfd, ...);
>         send(sfd, buffer, 8161, MSG_CONFIRM|MSG_MORE);
>         write(pfd[1], buffer, 8);
>         splice(pfd[0], 0, sfd, 0, 0x4ffe0ul, 0);
> 
> where the amount of data given to send() is dependent on the MTU size (in
> this instance an interface with an MTU of 8192).
> 
> The problem is that the calculation of the amount to copy in
> __ip_append_data() goes negative in two places, and, in the second place,
> this gets subtracted from the length remaining, thereby increasing it.
> 
> This happens when pagedlen > 0 (which happens for MSG_ZEROCOPY and
> MSG_SPLICE_PAGES), the terms in:
> 
>         copy = datalen - transhdrlen - fraggap - pagedlen;
> 
> then mostly cancel when pagedlen is substituted for, leaving just -fraggap.
> This causes:
> 
>         length -= copy + transhdrlen;
> 
> to increase the length to more than the amount of data in msg->msg_iter,
> which causes skb_splice_from_iter() to be unable to fill the request and it
> returns less than 'copied' - which means that length never gets to 0 and we
> never exit the loop.
> 
> Fix this by:
> 
>  (1) Insert a note about the dodgy calculation of 'copy'.
> 
>  (2) If MSG_SPLICE_PAGES, clear copy if it is negative from the above
>      equation, so that 'offset' isn't regressed and 'length' isn't
>      increased, which will mean that length and thus copy should match the
>      amount left in the iterator.
> 
>  (3) When handling MSG_SPLICE_PAGES, give a warning and return -EIO if
>      we're asked to splice more than is in the iterator.  It might be
>      better to not give the warning or even just give a 'short' write.
> 
> The same problem occurs in __ip6_append_data(), except that there's a check
> in there that errors out with EINVAL if copy < 0.  Fix this function in
> much the same way as the ipv4 variant but also skip the erroring out if
> copy < 0.

I don't think the two should be combined.

Removing that branch actually opens up to new bugs, e.g., in MSG_ZEROCOPY.

Since the ipv6 stack is not subject to this bug in MSG_SPLICE_PAGES,
I think v1 on its own is correct, and has the right Fixes tag.

If we think this IPv6 branch is mistaken and a bug, then that would
have a different Fixes tag, going back to ancient history. But arguably
safer to limit that net-next, if touching that at all.

It makes sense to update MSG_SPLICE_PAGES in the IPv6 path to be
equivalent to the IPv4 path, and to be correct if that branch is
ever removed.