On Tue, Sep 19, 2023 at 10:27 AM Tom Parkin <tparkin@xxxxxxxxxxx> wrote: > > On Mon, Sep 18, 2023 at 16:04:49 +0200, Eric Dumazet wrote: > > On Mon, Sep 18, 2023 at 3:58 PM Willem de Bruijn > > <willemdebruijn.kernel@xxxxxxxxx> wrote: > > > > > > David Howells wrote: > > > > David Howells <dhowells@xxxxxxxxxx> wrote: > > > > > > > > > I think the attached is probably an equivalent cleaned up reproducer. Note > > > > > that if the length given to sendfile() is less than 65536, it fails with > > > > > EINVAL before it gets into __ip6_append_data(). > > > > > > > > Actually, it only fails with EINVAL if the size is not a multiple of the block > > > > size of the source file because it's open O_DIRECT so, say, 65536-512 is fine > > > > (and works). > > > > > > > > But thinking more on this further, is this even a bug in my code, I wonder? > > > > The length passed is 65536 - but a UDP packet can't carry that, so it > > > > shouldn't it have errored out before getting that far? (which is what it > > > > seems to do when I try it). > > > > > > > > I don't see how we get past the length check in ip6_append_data() with the > > > > reproducer we're given unless the MTU is somewhat bigger than 65536 (is that > > > > even possible?) > > > > > > An ipv6 packet can carry 64KB of payload, so maxnonfragsize of 65535 + 40 > > > sounds correct. But payload length passed of 65536 is not (ignoring ipv6 > > > jumbograms). So that should probably trigger an EINVAL -- if that is indeed > > > what the repro does. > > > > l2tp_ip6_sendmsg() claims ip6_append_data() can make better checks, > > but what about simply replacing INT_MAX by 65535 ? > > Slightly OT but I think the l2tp_ip6.c approach was probably cribbed > from net/ipv6/udp.c's udpv6_sendmsg originally: > > > /* Rough check on arithmetic overflow, > better check is made in ip6_append_data(). > */ > if (len > INT_MAX - sizeof(struct udphdr)) > return -EMSGSIZE; > > > Should the udp code be modified similarly? > Unfortunately both l2tp and udp support CORK (MSG_MORE), so a modified check like that will not be enough to prevent syzbot reports. Better than nothing, of course. I also note that ipv4 size of l2tp does not have any check, an overflow seems possible with carefully chosen size.
