| Message ID | 20220310221328.877987-1-tadeusz.struk@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: ipv6: fix skb_over_panic in __ip6_append_data | expand |
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 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);
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.
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;
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.
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)
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 + */ + 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);
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(+)