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#L160... 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 10:18, David Ahern wrote:

...

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;
  

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. I think the problem here is that the size of the allocated skb is too small.

On 3/10/22 09:32, Willem de Bruijn wrote:

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.)

Do you mean something like this: diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 622345af323e..6d45112322a0 100644 @@ -1656,6 +1649,16 @@ static int __ip6_append_data(struct sock *sk, skb->protocol = htons(ETH_P_IPV6); skb->ip_summed = csummode; skb->csum = 0; + + /* + * Check if there is still room for the payload + */ + if (fragheaderlen >= mtu) { + err = -EMSGSIZE; + kfree_skb(skb); + goto error; + } + /* reserve for fragmentation and ipsec header */ skb_reserve(skb, hh_len + sizeof(struct frag_hdr) + dst_exthdrlen);

That works as well.

On 3/10/22 14:43, Willem de Bruijn wrote:

On Thu, Mar 10, 2022 at 5:30 PM Jakub Kicinski kuba@kernel.org wrote:

...

On Thu, 10 Mar 2022 14:13:28 -0800 Tadeusz Struk wrote:

...

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 4788f6b37053..6d45112322a0 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1649,6 +1649,16 @@ static int __ip6_append_data(struct sock *sk, skb->protocol = htons(ETH_P_IPV6); skb->ip_summed = csummode; skb->csum = 0;

•                 /*
  

•                  *      Check if there is still room for payload
  

•                  */
  

TBH I think the check is self-explanatory. Not worth a banner comment, for sure.

...

•                 if (fragheaderlen >= mtu) {
  

•                         err = -EMSGSIZE;
  

•                         kfree_skb(skb);
  

•                         goto error;
  

•                 }
  

Not sure if Willem prefers this placement, but seems like we can lift this check out of the loop, as soon as fragheaderlen and mtu are known.

...

                   /* reserve for fragmentation and ipsec header */
                   skb_reserve(skb, hh_len + sizeof(struct frag_hdr) +
                               dst_exthdrlen);

Just updating this boundary check will do?

     if (mtu < fragheaderlen ||
         ((mtu - fragheaderlen) & ~7) + fragheaderlen <

sizeof(struct frag_hdr)) goto emsgsize;

Yes, it will. v3 on its way.

Syzbot found a kernel bug in the ipv6 stack: LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e2541558... The reproducer triggers it by sending a crafted 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>

Update the check that prevents an invalid packet with MTU equall to the fregment header size to eat up all the space for payload.

The reproducer can be found here: LINK: https://syzkaller.appspot.com/text?tag=ReproC&x=1648c83fb00000

Cc: Willem de Bruijn willemdebruijn.kernel@gmail.com 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 --- v2: Instead of updating the alloclen add a check that prevents an invalid packet with MTU equall to the fregment header size to eat up all the space for payload. Fix suggested by Willem de Bruijn willemdebruijn.kernel@gmail.com

v3: Update existing check outside of the while loop. --- net/ipv6/ip6_output.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 4788f6b37053..194832663d85 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1476,8 +1476,8 @@ static int __ip6_append_data(struct sock *sk, sizeof(struct frag_hdr) : 0) + rt->rt6i_nfheader_len;

- if (mtu < fragheaderlen || - ((mtu - fragheaderlen) & ~7) + fragheaderlen < sizeof(struct frag_hdr)) + if (mtu <= fragheaderlen || + ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr)) goto emsgsize;

maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen -