All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
@ 2022-03-08  0:01 Tadeusz Struk
  2022-03-08  2:58 ` David Laight
  0 siblings, 1 reply; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-08  0:01 UTC (permalink / raw)
  To: davem
  Cc: Tadeusz Struk, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

Syzbot found a kernel bug in the ipv6 stack:
LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580

The reproducer triggers it by sending an invalid 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>

------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:113!
PREEMPT SMP KASAN
CPU: 1 PID: 1818 Comm: repro Not tainted 5.17.0-rc7-dirty #9
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35
RIP: 0010:skb_panic+0x173/0x175
RSP: 0018:ffffc900015bf3b8 EFLAGS: 00010282
RAX: 0000000000000090 RBX: ffff88810e848c80 RCX: 0000000000000000
RDX: ffff88810fd84300 RSI: ffffffff814fa5ef RDI: fffff520002b7e69
RBP: ffffc900015bf420 R08: 0000000000000090 R09: 0000000000000000
R10: ffffffff814f55f4 R11: 203a666675626b73 R12: ffffffff855bff80
R13: ffffffff84647fb4 R14: 0000000000010027 R15: ffffffff855bf420
FS:  0000000000c8b3c0(0000) GS:ffff88811b100000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000040 CR3: 0000000106b68000 CR4: 0000000000150ea0
Call Trace:
 <TASK>
 skb_put.cold+0x23/0x23
 __ip6_append_data.isra.0.cold+0x396/0xe3a
 ip6_append_data+0x1e5/0x320
 rawv6_sendmsg.cold+0x1618/0x2ba9
 inet_sendmsg+0x9e/0xe0
 sock_sendmsg+0xd7/0x130
 ____sys_sendmsg+0x381/0x8a0
 ___sys_sendmsg+0x100/0x170
 __sys_sendmmsg+0x26c/0x3b7
 __x64_sys_sendmmsg+0xb2/0xbd
 do_syscall_64+0x35/0xb0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

The reproducer can be found here:
LINK: https://syzkaller.appspot.com/text?tag=ReproC&x=1648c83fb00000
This can be fixed by increasing the alloclen in case it is smaller than
fraglen in __ip6_append_data().

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

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;
+			}
+
 			if (transhdrlen) {
 				skb = sock_alloc_send_skb(sk, alloclen,
 						(flags & MSG_DONTWAIT), &err);
-- 
2.35.1

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* RE: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-08  0:01 [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data Tadeusz Struk
@ 2022-03-08  2:58 ` David Laight
  2022-03-08 15:43   ` Tadeusz Struk
  0 siblings, 1 reply; 22+ messages in thread
From: David Laight @ 2022-03-08  2:58 UTC (permalink / raw)
  To: 'Tadeusz Struk', davem
  Cc: Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

From: Tadeusz Struk
> Sent: 08 March 2022 00:02
> 
> Syzbot found a kernel bug in the ipv6 stack:
> LINK: https://syzkaller.appspot.com/bug?id=205d6f11d72329ab8d62a610c44c5e7e25415580
> 
> The reproducer triggers it by sending an invalid 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>
> 
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:113!
> PREEMPT SMP KASAN
> CPU: 1 PID: 1818 Comm: repro Not tainted 5.17.0-rc7-dirty #9
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35
> RIP: 0010:skb_panic+0x173/0x175
> RSP: 0018:ffffc900015bf3b8 EFLAGS: 00010282
> RAX: 0000000000000090 RBX: ffff88810e848c80 RCX: 0000000000000000
> RDX: ffff88810fd84300 RSI: ffffffff814fa5ef RDI: fffff520002b7e69
> RBP: ffffc900015bf420 R08: 0000000000000090 R09: 0000000000000000
> R10: ffffffff814f55f4 R11: 203a666675626b73 R12: ffffffff855bff80
> R13: ffffffff84647fb4 R14: 0000000000010027 R15: ffffffff855bf420
> FS:  0000000000c8b3c0(0000) GS:ffff88811b100000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020000040 CR3: 0000000106b68000 CR4: 0000000000150ea0
> Call Trace:
>  <TASK>
>  skb_put.cold+0x23/0x23
>  __ip6_append_data.isra.0.cold+0x396/0xe3a
>  ip6_append_data+0x1e5/0x320
>  rawv6_sendmsg.cold+0x1618/0x2ba9
>  inet_sendmsg+0x9e/0xe0
>  sock_sendmsg+0xd7/0x130
>  ____sys_sendmsg+0x381/0x8a0
>  ___sys_sendmsg+0x100/0x170
>  __sys_sendmmsg+0x26c/0x3b7
>  __x64_sys_sendmmsg+0xb2/0xbd
>  do_syscall_64+0x35/0xb0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> The reproducer can be found here:
> LINK: https://syzkaller.appspot.com/text?tag=ReproC&x=1648c83fb00000
> This can be fixed by increasing the alloclen in case it is smaller than
> fraglen in __ip6_append_data().
> 
> 
> Reported-by: syzbot+e223cf47ec8ae183f2a0@syzkaller.appspotmail.com
> Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
> ---
>  net/ipv6/ip6_output.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 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.

	David

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-08  2:58 ` David Laight
@ 2022-03-08 15:43   ` Tadeusz Struk
  2022-03-08 18:18     ` David Ahern
  0 siblings, 1 reply; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-08 15:43 UTC (permalink / raw)
  To: David Laight, davem
  Cc: Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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#L1606
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

-- 
Thanks,
Tadeusz

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-08 15:43   ` Tadeusz Struk
@ 2022-03-08 18:18     ` David Ahern
  2022-03-08 19:46       ` Tadeusz Struk
  0 siblings, 1 reply; 22+ messages in thread
From: David Ahern @ 2022-03-08 18:18 UTC (permalink / raw)
  To: Tadeusz Struk, David Laight, davem
  Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

On 3/8/22 8:43 AM, Tadeusz Struk wrote:
> 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#L1606
> 
> 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
> 

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;

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-08 18:18     ` David Ahern
@ 2022-03-08 19:46       ` Tadeusz Struk
  2022-03-09  5:01         ` David Ahern
  0 siblings, 1 reply; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-08 19:46 UTC (permalink / raw)
  To: David Ahern, David Laight, davem
  Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

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.

-- 
Thanks,
Tadeusz

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-08 19:46       ` Tadeusz Struk
@ 2022-03-09  5:01         ` David Ahern
  2022-03-09 21:37           ` Tadeusz Struk
  0 siblings, 1 reply; 22+ messages in thread
From: David Ahern @ 2022-03-09  5:01 UTC (permalink / raw)
  To: Tadeusz Struk, David Laight, davem
  Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

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.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-09  5:01         ` David Ahern
@ 2022-03-09 21:37           ` Tadeusz Struk
  2022-03-10 14:39             ` Willem de Bruijn
  0 siblings, 1 reply; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-09 21:37 UTC (permalink / raw)
  To: David Ahern, David Laight, davem
  Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

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.

-- 
Thanks,
Tadeusz

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-09 21:37           ` Tadeusz Struk
@ 2022-03-10 14:39             ` Willem de Bruijn
  2022-03-10 16:06               ` Tadeusz Struk
  0 siblings, 1 reply; 22+ messages in thread
From: Willem de Bruijn @ 2022-03-10 14:39 UTC (permalink / raw)
  To: Tadeusz Struk
  Cc: David Ahern, David Laight, davem, Hideaki YOSHIFUJI,
	Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-10 14:39             ` Willem de Bruijn
@ 2022-03-10 16:06               ` Tadeusz Struk
  2022-03-10 17:32                 ` Willem de Bruijn
  0 siblings, 1 reply; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 16:06 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: David Ahern, David Laight, davem, Hideaki YOSHIFUJI,
	Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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,
Tadeusz

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-10 16:06               ` Tadeusz Struk
@ 2022-03-10 17:32                 ` Willem de Bruijn
  2022-03-10 21:14                   ` Tadeusz Struk
  2022-03-10 22:13                   ` [PATCH v2] net: ipv6: fix skb_over_panic " Tadeusz Struk
  0 siblings, 2 replies; 22+ messages in thread
From: Willem de Bruijn @ 2022-03-10 17:32 UTC (permalink / raw)
  To: Tadeusz Struk
  Cc: Willem de Bruijn, David Ahern, David Laight, davem,
	Hideaki YOSHIFUJI, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data
  2022-03-10 17:32                 ` Willem de Bruijn
@ 2022-03-10 21:14                   ` Tadeusz Struk
  2022-03-10 22:13                   ` [PATCH v2] net: ipv6: fix skb_over_panic " Tadeusz Struk
  1 sibling, 0 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 21:14 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: David Ahern, David Laight, davem, Hideaki YOSHIFUJI,
	Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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.
-- 
Thanks,
Tadeusz

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 17:32                 ` Willem de Bruijn
  2022-03-10 21:14                   ` Tadeusz Struk
@ 2022-03-10 22:13                   ` Tadeusz Struk
  2022-03-10 22:18                     ` David Laight
  2022-03-10 22:30                     ` Jakub Kicinski
  1 sibling, 2 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 22:13 UTC (permalink / raw)
  To: davem
  Cc: Tadeusz Struk, Willem de Bruijn, Hideaki YOSHIFUJI, David Ahern,
	Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* RE: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:13                   ` [PATCH v2] net: ipv6: fix skb_over_panic " Tadeusz Struk
@ 2022-03-10 22:18                     ` David Laight
  2022-03-10 22:30                     ` Jakub Kicinski
  1 sibling, 0 replies; 22+ messages in thread
From: David Laight @ 2022-03-10 22:18 UTC (permalink / raw)
  To: 'Tadeusz Struk', davem
  Cc: Willem de Bruijn, Hideaki YOSHIFUJI, David Ahern, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:13                   ` [PATCH v2] net: ipv6: fix skb_over_panic " Tadeusz Struk
  2022-03-10 22:18                     ` David Laight
@ 2022-03-10 22:30                     ` Jakub Kicinski
  2022-03-10 22:42                       ` Tadeusz Struk
  2022-03-10 22:43                       ` Willem de Bruijn
  1 sibling, 2 replies; 22+ messages in thread
From: Jakub Kicinski @ 2022-03-10 22:30 UTC (permalink / raw)
  To: Tadeusz Struk
  Cc: davem, Willem de Bruijn, Hideaki YOSHIFUJI, David Ahern,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:30                     ` Jakub Kicinski
@ 2022-03-10 22:42                       ` Tadeusz Struk
  2022-03-10 22:43                       ` Willem de Bruijn
  1 sibling, 0 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 22:42 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, Willem de Bruijn, Hideaki YOSHIFUJI, David Ahern,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:30                     ` Jakub Kicinski
  2022-03-10 22:42                       ` Tadeusz Struk
@ 2022-03-10 22:43                       ` Willem de Bruijn
  2022-03-10 23:04                         ` Tadeusz Struk
                                           ` (2 more replies)
  1 sibling, 3 replies; 22+ messages in thread
From: Willem de Bruijn @ 2022-03-10 22:43 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Tadeusz Struk, David Miller, Hideaki YOSHIFUJI, David Ahern,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, LKML, stable,
	syzbot+e223cf47ec8ae183f2a0, Willem de Bruijn

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;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:43                       ` Willem de Bruijn
@ 2022-03-10 23:04                         ` Tadeusz Struk
  2022-03-10 23:05                         ` David Laight
  2022-03-10 23:25                         ` [PATCH v3] " Tadeusz Struk
  2 siblings, 0 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 23:04 UTC (permalink / raw)
  To: Willem de Bruijn, Jakub Kicinski
  Cc: David Miller, Hideaki YOSHIFUJI, David Ahern, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Network Development,
	bpf, LKML, stable, syzbot+e223cf47ec8ae183f2a0, Willem de Bruijn

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* RE: [PATCH v2] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:43                       ` Willem de Bruijn
  2022-03-10 23:04                         ` Tadeusz Struk
@ 2022-03-10 23:05                         ` David Laight
  2022-03-10 23:25                         ` [PATCH v3] " Tadeusz Struk
  2 siblings, 0 replies; 22+ messages in thread
From: David Laight @ 2022-03-10 23:05 UTC (permalink / raw)
  To: 'Willem de Bruijn', Jakub Kicinski
  Cc: Tadeusz Struk, David Miller, Hideaki YOSHIFUJI, David Ahern,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Network Development, bpf, LKML, stable,
	syzbot+e223cf47ec8ae183f2a0, Willem de Bruijn

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)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 22:43                       ` Willem de Bruijn
  2022-03-10 23:04                         ` Tadeusz Struk
  2022-03-10 23:05                         ` David Laight
@ 2022-03-10 23:25                         ` Tadeusz Struk
  2022-03-11  1:49                           ` Willem de Bruijn
  2022-03-12  1:40                           ` patchwork-bot+netdevbpf
  2 siblings, 2 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-10 23:25 UTC (permalink / raw)
  To: kuba
  Cc: Tadeusz Struk, Willem de Bruijn, David S . Miller,
	Hideaki YOSHIFUJI, David Ahern, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, netdev, bpf,
	linux-kernel, stable, syzbot+e223cf47ec8ae183f2a0

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v3] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 23:25                         ` [PATCH v3] " Tadeusz Struk
@ 2022-03-11  1:49                           ` Willem de Bruijn
  2022-03-11  3:43                             ` Tadeusz Struk
  2022-03-12  1:40                           ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 22+ messages in thread
From: Willem de Bruijn @ 2022-03-11  1:49 UTC (permalink / raw)
  To: Tadeusz Struk
  Cc: kuba, Willem de Bruijn, David S . Miller, Hideaki YOSHIFUJI,
	David Ahern, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-11  1:49                           ` Willem de Bruijn
@ 2022-03-11  3:43                             ` Tadeusz Struk
  0 siblings, 0 replies; 22+ messages in thread
From: Tadeusz Struk @ 2022-03-11  3:43 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: kuba, David S . Miller, Hideaki YOSHIFUJI, David Ahern,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3] net: ipv6: fix skb_over_panic in __ip6_append_data
  2022-03-10 23:25                         ` [PATCH v3] " Tadeusz Struk
  2022-03-11  1:49                           ` Willem de Bruijn
@ 2022-03-12  1:40                           ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 22+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-03-12  1:40 UTC (permalink / raw)
  To: Tadeusz Struk
  Cc: kuba, willemdebruijn.kernel, davem, yoshfuji, dsahern, ast,
	daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, netdev, bpf, linux-kernel, stable,
	syzbot+e223cf47ec8ae183f2a0

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-03-12  1:40 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-08  0:01 [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data Tadeusz Struk
2022-03-08  2:58 ` David Laight
2022-03-08 15:43   ` Tadeusz Struk
2022-03-08 18:18     ` David Ahern
2022-03-08 19:46       ` Tadeusz Struk
2022-03-09  5:01         ` David Ahern
2022-03-09 21:37           ` Tadeusz Struk
2022-03-10 14:39             ` Willem de Bruijn
2022-03-10 16:06               ` Tadeusz Struk
2022-03-10 17:32                 ` Willem de Bruijn
2022-03-10 21:14                   ` Tadeusz Struk
2022-03-10 22:13                   ` [PATCH v2] net: ipv6: fix skb_over_panic " Tadeusz Struk
2022-03-10 22:18                     ` David Laight
2022-03-10 22:30                     ` Jakub Kicinski
2022-03-10 22:42                       ` Tadeusz Struk
2022-03-10 22:43                       ` Willem de Bruijn
2022-03-10 23:04                         ` Tadeusz Struk
2022-03-10 23:05                         ` David Laight
2022-03-10 23:25                         ` [PATCH v3] " Tadeusz Struk
2022-03-11  1:49                           ` Willem de Bruijn
2022-03-11  3:43                             ` Tadeusz Struk
2022-03-12  1:40                           ` patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.