On Thu, Mar 10, 2022 at 5:30 PM Jakub Kicinski <[email protected]> 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 <[email protected]> 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
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 <[email protected]>
Cc: David S. Miller <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Martin KaFai Lau <[email protected]>
Cc: Song Liu <[email protected]>
Cc: Yonghong Song <[email protected]>
Cc: John Fastabend <[email protected]>
Cc: KP Singh <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reported-by: [email protected]
Signed-off-by: Tadeusz Struk <[email protected]>
---
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 <[email protected]>
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.1
On 3/10/22 17:49, Willem de Bruijn wrote:
>> Reported-by:[email protected]
>> Signed-off-by: Tadeusz Struk<[email protected]>
> Acked-by: Willem de Bruijn<[email protected]>
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
From: Willem de Bruijn
> Sent: 10 March 2022 22:43
>
> On Thu, Mar 10, 2022 at 5:30 PM Jakub Kicinski <[email protected]> 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)
On Thu, Mar 10, 2022 at 6:26 PM Tadeusz Struk <[email protected]> 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 <[email protected]>
> Cc: David S. Miller <[email protected]>
> Cc: Hideaki YOSHIFUJI <[email protected]>
> Cc: David Ahern <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: John Fastabend <[email protected]>
> Cc: KP Singh <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Reported-by: [email protected]
> Signed-off-by: Tadeusz Struk <[email protected]>
Acked-by: Willem de Bruijn <[email protected]>
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.
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <[email protected]>:
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