[v2] net: ipv6: fix skb_over_panic in __ip6_append_data

Syzbot found a kernel bug in the ipv6 stack:
LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580
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>

Add a 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>
---
 net/ipv6/ip6_output.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

From: Tadeusz Struk
> Sent: 10 March 2022 22:13
> 
> Syzbot found a kernel bug in the ipv6 stack:
> LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580
> 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>
> 
> Add a check that prevents an invalid packet with MTU equall to the
> fregment header size to eat up all the space for payload.

There probably ought to be a check much earlier that stops
the option that makes the iphdr being to big being accepted
in the first place.

Much better to do the check in the option generation code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On 3/10/22 14:30, Jakub Kicinski wrote:
>> +
>> +			/*
>> +			 *	Check if there is still room for payload
>> +			 */
> TBH I think the check is self-explanatory. Not worth a banner comment,
> for sure.

Ok

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

He said to check it before the skb_put() and so I did.
The fragheaderlen is known early, but mtu can be updated inside the loop
by ip6_append_data_mtu() so I'm not sure we can do the check before that.

From: Willem de Bruijn
> Sent: 10 March 2022 22:43
> 
> 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;

Both those < should be <=

But I think I'd check:
	if (fragheaderlen >= 1280 - sizeof (struct frag_hdr))
		goto emsgsize;
first (or only!)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)