net/ipv6/ip6_output.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
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(+)
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);
--
2.35.1From: 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 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.
--
Thanks,
Tadeusz
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)
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>
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 -
--
2.35.1On Thu, Mar 10, 2022 at 6:26 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > > 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> > > 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> Acked-by: Willem de Bruijn <willemb@google.com> small nit: "equal to the fragment" and all these Cc:s aren't really needed in the commit message. I don't think we'll find a commit for a Fixes tag. This goes ways back.
On 3/10/22 17:49, Willem de Bruijn wrote: >> Reported-by:syzbot+e223cf47ec8ae183f2a0@syzkaller.appspotmail.com >> Signed-off-by: Tadeusz Struk<tadeusz.struk@linaro.org> > Acked-by: Willem de Bruijn<willemb@google.com> Thanks! > > small nit: "equal to the fragment" and all these Cc:s aren't really > needed in the commit message. I usually Cc all addresses that the scripts/get_maintainer.pl prints out. > I don't think we'll find a commit for a Fixes tag. This goes ways back. Agree. -- Thanks, Tadeusz
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 10 Mar 2022 15:25:38 -0800 you wrote:
> 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>
>
> [...]
Here is the summary with links:
- [v3] net: ipv6: fix skb_over_panic in __ip6_append_data
https://git.kernel.org/netdev/net/c/5e34af4142ff
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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.
--
Thanks,
Tadeusz
© 2016 - 2026 Red Hat, Inc.