| Message ID | 20220308000146.534935-1-tadeusz.struk@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipv6: fix invalid alloclen in __ip6_append_data | expand |
From: Tadeusz Struk > Sent: 08 March 2022 00:02 > > Syzbot found a kernel bug in the ipv6 stack: > LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580 > > The reproducer triggers it by sending an invalid message via sendmmsg() call, > which triggers skb_over_panic, and crashes the kernel: > > skbuff: skb_over_panic: text:ffffffff84647fb4 len:65575 put:65575 > head:ffff888109ff0000 data:ffff888109ff0088 tail:0x100af end:0xfec0 > dev:<NULL> > > ------------[ cut here ]------------ > kernel BUG at net/core/skbuff.c:113! > PREEMPT SMP KASAN > CPU: 1 PID: 1818 Comm: repro Not tainted 5.17.0-rc7-dirty #9 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 > RIP: 0010:skb_panic+0x173/0x175 > RSP: 0018:ffffc900015bf3b8 EFLAGS: 00010282 > RAX: 0000000000000090 RBX: ffff88810e848c80 RCX: 0000000000000000 > RDX: ffff88810fd84300 RSI: ffffffff814fa5ef RDI: fffff520002b7e69 > RBP: ffffc900015bf420 R08: 0000000000000090 R09: 0000000000000000 > R10: ffffffff814f55f4 R11: 203a666675626b73 R12: ffffffff855bff80 > R13: ffffffff84647fb4 R14: 0000000000010027 R15: ffffffff855bf420 > FS: 0000000000c8b3c0(0000) GS:ffff88811b100000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000000020000040 CR3: 0000000106b68000 CR4: 0000000000150ea0 > Call Trace: > <TASK> > skb_put.cold+0x23/0x23 > __ip6_append_data.isra.0.cold+0x396/0xe3a > ip6_append_data+0x1e5/0x320 > rawv6_sendmsg.cold+0x1618/0x2ba9 > inet_sendmsg+0x9e/0xe0 > sock_sendmsg+0xd7/0x130 > ____sys_sendmsg+0x381/0x8a0 > ___sys_sendmsg+0x100/0x170 > __sys_sendmmsg+0x26c/0x3b7 > __x64_sys_sendmmsg+0xb2/0xbd > do_syscall_64+0x35/0xb0 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > The reproducer can be found here: > LINK: https://syzkaller.appspot.com/text?tag=ReproC&x=1648c83fb00000 > This can be fixed by increasing the alloclen in case it is smaller than > fraglen in __ip6_append_data(). > > > Reported-by: syzbot+e223cf47ec8ae183f2a0@syzkaller.appspotmail.com > Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> > --- > net/ipv6/ip6_output.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 4788f6b37053..622345af323e 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -1629,6 +1629,13 @@ static int __ip6_append_data(struct sock *sk, > err = -EINVAL; > goto error; > } > + if (unlikely(alloclen < fraglen)) { > + if (printk_ratelimit()) > + pr_warn("%s: wrong alloclen: %d, fraglen: %d", > + __func__, alloclen, fraglen); > + alloclen = fraglen; > + } > + Except that is a valid case, see a few lines higher: alloclen = min_t(int, fraglen, MAX_HEADER); pagedlen = fraglen - alloclen; You need to report the input values that cause the problem later on. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On 3/7/22 18:58, David Laight wrote: >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >> index 4788f6b37053..622345af323e 100644 >> --- a/net/ipv6/ip6_output.c >> +++ b/net/ipv6/ip6_output.c >> @@ -1629,6 +1629,13 @@ static int __ip6_append_data(struct sock *sk, >> err = -EINVAL; >> goto error; >> } >> + if (unlikely(alloclen < fraglen)) { >> + if (printk_ratelimit()) >> + pr_warn("%s: wrong alloclen: %d, fraglen: %d", >> + __func__, alloclen, fraglen); >> + alloclen = fraglen; >> + } >> + > Except that is a valid case, see a few lines higher: > > alloclen = min_t(int, fraglen, MAX_HEADER); > pagedlen = fraglen - alloclen; > > You need to report the input values that cause the problem later on. OK, but in this case it falls into the first if block: https://elixir.bootlin.com/linux/v5.17-rc7/source/net/ipv6/ip6_output.c#L1606 where alloclen is assigned the value of mtu. The values in this case are just before the alloc_skb() are: alloclen = 1480 alloc_extra = 136 datalen = 64095 fragheaderlen = 1480 fraglen = 65575 transhdrlen = 0 mtu = 1480
On 3/8/22 8:43 AM, Tadeusz Struk wrote: > Hi David, > On 3/7/22 18:58, David Laight wrote: >>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >>> index 4788f6b37053..622345af323e 100644 >>> --- a/net/ipv6/ip6_output.c >>> +++ b/net/ipv6/ip6_output.c >>> @@ -1629,6 +1629,13 @@ static int __ip6_append_data(struct sock *sk, >>> err = -EINVAL; >>> goto error; >>> } >>> + if (unlikely(alloclen < fraglen)) { >>> + if (printk_ratelimit()) >>> + pr_warn("%s: wrong alloclen: %d, fraglen: %d", >>> + __func__, alloclen, fraglen); >>> + alloclen = fraglen; >>> + } >>> + >> Except that is a valid case, see a few lines higher: >> >> alloclen = min_t(int, fraglen, MAX_HEADER); >> pagedlen = fraglen - alloclen; >> >> You need to report the input values that cause the problem later on. > > OK, but in this case it falls into the first if block: > https://elixir.bootlin.com/linux/v5.17-rc7/source/net/ipv6/ip6_output.c#L1606 > > where alloclen is assigned the value of mtu. > The values in this case are just before the alloc_skb() are: > > alloclen = 1480 > alloc_extra = 136 > datalen = 64095 > fragheaderlen = 1480 > fraglen = 65575 > transhdrlen = 0 > mtu = 1480 > Does this solve the problem (whitespace damaged on paste, but it is just a code move and removing fraglen getting set twice): diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index e69fac576970..59f036241f1b 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1589,6 +1589,15 @@ static int __ip6_append_data(struct sock *sk, if (datalen > (cork->length <= mtu && !(cork->flags & IPCORK_ALLFRAG) ? mtu : maxfraglen) - fragheaderlen) datalen = maxfraglen - fragheaderlen - rt->dst.trailer_len; + + if (datalen != length + fraggap) { + /* + * this is not the last fragment, the trailer + * space is regarded as data space. + */ + datalen += rt->dst.trailer_len; + } + fraglen = datalen + fragheaderlen; pagedlen = 0; @@ -1615,16 +1624,6 @@ static int __ip6_append_data(struct sock *sk, } alloclen += alloc_extra; - if (datalen != length + fraggap) { - /* - * this is not the last fragment, the trailer - * space is regarded as data space. - */ - datalen += rt->dst.trailer_len; - } - - fraglen = datalen + fragheaderlen; - copy = datalen - transhdrlen - fraggap - pagedlen; if (copy < 0) { err = -EINVAL;
On 3/8/22 21:01, David Ahern wrote: > On 3/8/22 12:46 PM, Tadeusz Struk wrote: >> That fails in the same way: >> >> skbuff: skb_over_panic: text:ffffffff83e7b48b len:65575 put:65575 >> head:ffff888101f8a000 data:ffff888101f8a088 tail:0x100af end:0x6c0 >> dev:<NULL> >> ------------[ cut here ]------------ >> kernel BUG at net/core/skbuff.c:113! >> invalid opcode: 0000 [#1] PREEMPT SMP KASAN >> CPU: 0 PID: 1852 Comm: repro Not tainted >> 5.17.0-rc7-00020-gea4424be1688-dirty #19 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 >> RIP: 0010:skb_panic+0x173/0x175 >> >> I'm not sure how it supposed to help since it doesn't change the >> alloclen at all. > > alloclen is a function of fraglen and fraglen is a function of datalen. Ok, but in this case it doesn't affect the alloclen and it still fails.
On Wed, Mar 9, 2022 at 4:37 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > > On 3/8/22 21:01, David Ahern wrote: > > On 3/8/22 12:46 PM, Tadeusz Struk wrote: > >> That fails in the same way: > >> > >> skbuff: skb_over_panic: text:ffffffff83e7b48b len:65575 put:65575 > >> head:ffff888101f8a000 data:ffff888101f8a088 tail:0x100af end:0x6c0 > >> dev:<NULL> > >> ------------[ cut here ]------------ > >> kernel BUG at net/core/skbuff.c:113! > >> invalid opcode: 0000 [#1] PREEMPT SMP KASAN > >> CPU: 0 PID: 1852 Comm: repro Not tainted > >> 5.17.0-rc7-00020-gea4424be1688-dirty #19 > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 > >> RIP: 0010:skb_panic+0x173/0x175 > >> > >> I'm not sure how it supposed to help since it doesn't change the > >> alloclen at all. > > > > alloclen is a function of fraglen and fraglen is a function of datalen. > > Ok, but in this case it doesn't affect the alloclen and it still fails. This is some kind of non-standard packet that is being constructed. Do we understand how it is different? The .syz reproducer is generally a bit more readable than the .c equivalent. Though not as much as an strace of the binary, if you can share that. r0 = socket$inet6_icmp_raw(0xa, 0x3, 0x3a) connect$inet6(r0, &(0x7f0000000040)={0xa, 0x0, 0x0, @dev, 0x6}, 0x1c) setsockopt$inet6_IPV6_HOPOPTS(r0, 0x29, 0x36, &(0x7f0000000100)=ANY=[@ANYBLOB="52b3"], 0x5a0) sendmmsg$inet(r0, &(0x7f00000002c0)=[{{0x0, 0x0, &(0x7f0000000000)=[{&(0x7f00000000c0)="1d2d", 0xfa5f}], 0x1}}], 0x1, 0xfe80)
On Thu, Mar 10, 2022 at 11:06 AM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > > On 3/10/22 06:39, Willem de Bruijn wrote: > > On Wed, Mar 9, 2022 at 4:37 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > >> > >> On 3/8/22 21:01, David Ahern wrote: > >>> On 3/8/22 12:46 PM, Tadeusz Struk wrote: > >>>> That fails in the same way: > >>>> > >>>> skbuff: skb_over_panic: text:ffffffff83e7b48b len:65575 put:65575 > >>>> head:ffff888101f8a000 data:ffff888101f8a088 tail:0x100af end:0x6c0 > >>>> dev:<NULL> > >>>> ------------[ cut here ]------------ > >>>> kernel BUG at net/core/skbuff.c:113! > >>>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN > >>>> CPU: 0 PID: 1852 Comm: repro Not tainted > >>>> 5.17.0-rc7-00020-gea4424be1688-dirty #19 > >>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 > >>>> RIP: 0010:skb_panic+0x173/0x175 > >>>> > >>>> I'm not sure how it supposed to help since it doesn't change the > >>>> alloclen at all. > >>> > >>> alloclen is a function of fraglen and fraglen is a function of datalen. > >> > >> Ok, but in this case it doesn't affect the alloclen and it still fails. > > > > This is some kind of non-standard packet that is being constructed. Do > > we understand how it is different? > > > > The .syz reproducer is generally a bit more readable than the .c > > equivalent. Though not as much as an strace of the binary, if you > > can share that. > > > > r0 = socket$inet6_icmp_raw(0xa, 0x3, 0x3a) > > connect$inet6(r0, &(0x7f0000000040)={0xa, 0x0, 0x0, @dev, 0x6}, 0x1c) > > setsockopt$inet6_IPV6_HOPOPTS(r0, 0x29, 0x36, > > &(0x7f0000000100)=ANY=[@ANYBLOB="52b3"], 0x5a0) > > sendmmsg$inet(r0, &(0x7f00000002c0)=[{{0x0, 0x0, > > &(0x7f0000000000)=[{&(0x7f00000000c0)="1d2d", 0xfa5f}], 0x1}}], 0x1, > > 0xfe80) > > Here it is: > https://termbin.com/krtr > It won't be of much help, I'm afraid, as the offending sendmmsg() > call isn't fully printed. Thanks. It does offer some hints on the other two syscalls: [pid 644] connect(3, {sa_family=AF_INET6, sin6_port=htons(0), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "fe80::", &sin6_addr), sin6_scope_id=if_nametoindex("tunl0")}, 28) = 0 [pid 644] setsockopt(3, SOL_IPV6, IPV6_HOPOPTS, "R\263\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1440) = 0 IPV6_HOPOPTS is ns_capable CAP_NET_RAW. So this adds 1440 bytes to opt_nflen, which is included in fragheaderlen, causing that to be exactly mtu. This means that the payload can never be sent, as each fragment header eats up the entire mtu? This is without any transport headers that would only be part of the first fragment (which go into opt_flen). If you can maybe catch the error before the skb_put and just return EINVAL, we might see whether sendmmsg is relevant or a simple send would be equivalent. (not super important, that appears unrelated.)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 4788f6b37053..622345af323e 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1629,6 +1629,13 @@ static int __ip6_append_data(struct sock *sk, err = -EINVAL; goto error; } + if (unlikely(alloclen < fraglen)) { + if (printk_ratelimit()) + pr_warn("%s: wrong alloclen: %d, fraglen: %d", + __func__, alloclen, fraglen); + alloclen = fraglen; + } + if (transhdrlen) { skb = sock_alloc_send_skb(sk, alloclen, (flags & MSG_DONTWAIT), &err);
Syzbot found a kernel bug in the ipv6 stack: LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580 The reproducer triggers it by sending an invalid message via sendmmsg() call, which triggers skb_over_panic, and crashes the kernel: skbuff: skb_over_panic: text:ffffffff84647fb4 len:65575 put:65575 head:ffff888109ff0000 data:ffff888109ff0088 tail:0x100af end:0xfec0 dev:<NULL> ------------[ cut here ]------------ kernel BUG at net/core/skbuff.c:113! PREEMPT SMP KASAN CPU: 1 PID: 1818 Comm: repro Not tainted 5.17.0-rc7-dirty #9 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 RIP: 0010:skb_panic+0x173/0x175 RSP: 0018:ffffc900015bf3b8 EFLAGS: 00010282 RAX: 0000000000000090 RBX: ffff88810e848c80 RCX: 0000000000000000 RDX: ffff88810fd84300 RSI: ffffffff814fa5ef RDI: fffff520002b7e69 RBP: ffffc900015bf420 R08: 0000000000000090 R09: 0000000000000000 R10: ffffffff814f55f4 R11: 203a666675626b73 R12: ffffffff855bff80 R13: ffffffff84647fb4 R14: 0000000000010027 R15: ffffffff855bf420 FS: 0000000000c8b3c0(0000) GS:ffff88811b100000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000040 CR3: 0000000106b68000 CR4: 0000000000150ea0 Call Trace: <TASK> skb_put.cold+0x23/0x23 __ip6_append_data.isra.0.cold+0x396/0xe3a ip6_append_data+0x1e5/0x320 rawv6_sendmsg.cold+0x1618/0x2ba9 inet_sendmsg+0x9e/0xe0 sock_sendmsg+0xd7/0x130 ____sys_sendmsg+0x381/0x8a0 ___sys_sendmsg+0x100/0x170 __sys_sendmmsg+0x26c/0x3b7 __x64_sys_sendmmsg+0xb2/0xbd do_syscall_64+0x35/0xb0 entry_SYSCALL_64_after_hwframe+0x44/0xae The reproducer can be found here: LINK: https://syzkaller.appspot.com/text?tag=ReproC&x=1648c83fb00000 This can be fixed by increasing the alloclen in case it is smaller than fraglen in __ip6_append_data(). Cc: David S. Miller <davem@davemloft.net> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> Cc: David Ahern <dsahern@kernel.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Andrii Nakryiko <andrii@kernel.org> Cc: Martin KaFai Lau <kafai@fb.com> Cc: Song Liu <songliubraving@fb.com> Cc: Yonghong Song <yhs@fb.com> Cc: John Fastabend <john.fastabend@gmail.com> Cc: KP Singh <kpsingh@kernel.org> Cc: netdev@vger.kernel.org Cc: bpf@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Reported-by: syzbot+e223cf47ec8ae183f2a0@syzkaller.appspotmail.com Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> --- net/ipv6/ip6_output.c | 7 +++++++ 1 file changed, 7 insertions(+)