[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <64ca6764b3c6b_294ce9294bc@willemb.c.googlers.com.notmuch>
Date: Wed, 02 Aug 2023 10:25:40 -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-next] udp6: Fix __ip6_append_data()'s handling of
MSG_SPLICE_PAGES
David Howells wrote:
> __ip6_append_data() can has a similar problem to __ip_append_data()[1] when
> asked to splice into a partially-built UDP message that has more than the
> frag-limit data and up to the MTU limit, but in the ipv6 case, it errors
> out with EINVAL. This can be triggered with something like:
>
> pipe(pfd);
> sfd = socket(AF_INET6, SOCK_DGRAM, 0);
> connect(sfd, ...);
> send(sfd, buffer, 8137, 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
> __ip6_append_data() goes negative in two places, but a check has been put
> in to give an error in this case.
>
> This happens because 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.
>
> 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.
>
> (4) If MSG_SPLICE_PAGES, override the copy<0 check.
>
> [!] Note that this should also affect MSG_ZEROCOPY, but that will return
> -EINVAL for the range of send sizes that requires the skbuff to be split.
>
> Signed-off-by: David Howells <dhowells@...hat.com>
> cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> cc: "David S. Miller" <davem@...emloft.net>
> cc: Eric Dumazet <edumazet@...gle.com>
> cc: Jakub Kicinski <kuba@...nel.org>
> cc: Paolo Abeni <pabeni@...hat.com>
> cc: David Ahern <dsahern@...nel.org>
> cc: Jens Axboe <axboe@...nel.dk>
> cc: Matthew Wilcox <willy@...radead.org>
> cc: netdev@...r.kernel.org
> Link: https://lore.kernel.org/r/000000000000881d0606004541d1@google.com/ [1]
Reviewed-by: Willem de Bruijn <willemb@...gle.com>
I'm beginning to understand your point that the bug is older and copy
should never end up equal to -fraglen. pagedlen includes all of
datalen, which includes fraggap. This is wrong, as fraggap is always
copied to skb->linear. Haven't really thought it through, but would
this solve it as well?
else {
alloclen = fragheaderlen + transhdrlen;
- pagedlen = datalen - transhdrlen;
+ pagedlen = datalen - transhdrlen - fraggap;
After that copy no longer subtracts fraglen twice.
copy = datalen - transhdrlen - fraggap - pagedlen;
But don't mean to delay these targeted fixes for MSG_SPLICE_PAGES any
further.
Powered by blists - more mailing lists