Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530702 > unrolled thread

net: GPF in eth_header

Started byDmitry Vyukov <dvyukov@google.com>
First post2016-11-26 18:40 +0100
Last post2016-11-29 00:20 +0100
Articles 19 — 6 participants

Back to article view | Back to linux.kernel

Contents

  net: GPF in eth_header Dmitry Vyukov <dvyukov@google.com> - 2016-11-26 18:40 +0100
    Re: net: GPF in eth_header Eric Dumazet <edumazet@google.com> - 2016-11-26 19:30 +0100
      Re: net: GPF in eth_header Andrey Konovalov <andreyknvl@google.com> - 2016-11-26 20:10 +0100
        Re: net: GPF in eth_header Eric Dumazet <erdlkml@gmail.com> - 2016-11-26 21:10 +0100
          Re: net: GPF in eth_header Eric Dumazet <erdlkml@gmail.com> - 2016-11-26 21:40 +0100
          Re: net: GPF in eth_header Andrey Konovalov <andreyknvl@google.com> - 2016-11-29 11:30 +0100
            Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-29 16:00 +0100
              Re: net: GPF in eth_header Andrey Konovalov <andreyknvl@google.com> - 2016-11-29 16:40 +0100
                Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-29 17:20 +0100
        Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 20:00 +0100
          Re: net: GPF in eth_header Andrey Konovalov <andreyknvl@google.com> - 2016-11-28 20:10 +0100
            Re: net: GPF in eth_header Dmitry Vyukov <dvyukov@google.com> - 2016-11-28 20:40 +0100
              Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 20:50 +0100
                Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 22:10 +0100
                  Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 22:20 +0100
                    Re: net: GPF in eth_header Florian Westphal <fw@strlen.de> - 2016-11-28 22:40 +0100
                      Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-28 23:20 +0100
                        Re: net: GPF in eth_header Florian Westphal <fw@strlen.de> - 2016-11-28 23:30 +0100
                          Re: net: GPF in eth_header Eric Dumazet <eric.dumazet@gmail.com> - 2016-11-29 00:20 +0100

#1530702 — net: GPF in eth_header

FromDmitry Vyukov <dvyukov@google.com>
Date2016-11-26 18:40 +0100
Subjectnet: GPF in eth_header
Message-ID<sHP7b-189-13@gated-at.bofh.it>
Hello,

The following program triggers GPF in eth_header:

https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt

On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).

BUG: unable to handle kernel paging request at ffffed002d14d74a
IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
PGD 7fff6067 [   50.787819] PUD 7fff5067
PMD 0 [   50.787819]
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
Modules linked in:
CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88003a1841c0 task.stack: ffff880034d08000
RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
eth_header+0x75/0x260 net/ethernet/eth.c:88
RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
Stack:
 000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
 0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
 ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
Call Trace:
 [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
 [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
 [<     inline     >] dst_neigh_output ./include/net/dst.h:464
 [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
 [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
 [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
 [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
 [<     inline     >] dst_output ./include/net/dst.h:501
 [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
 [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
 [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
net/ipv6/ip6_output.c:1732
 [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
 [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
 [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
 [<     inline     >] sock_sendmsg_nosec net/socket.c:621
 [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
 [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
 [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
 [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
 [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
 [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
 [<     inline     >] SYSC_writev fs/read_write.c:1017
 [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
 [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
arch/x86/entry/entry_64.S:209
Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
 RSP <ffff880034d0eb68>
CR2: ffffed002d14d74a
---[ end trace a73fedfdc11bd60c ]---

[toc] | [next] | [standalone]

#1530758

FromEric Dumazet <edumazet@google.com>
Date2016-11-26 19:30 +0100
Message-ID<sHPTz-1JW-9@gated-at.bofh.it>
In reply to#1530702
On Sat, Nov 26, 2016 at 9:30 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Hello,
>
> The following program triggers GPF in eth_header:
>
> https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt
>
> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>
> BUG: unable to handle kernel paging request at ffffed002d14d74a
> IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
> PGD 7fff6067 [   50.787819] PUD 7fff5067
> PMD 0 [   50.787819]
> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
> Modules linked in:
> CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff88003a1841c0 task.stack: ffff880034d08000
> RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
> eth_header+0x75/0x260 net/ethernet/eth.c:88
> RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
> RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
> RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
> RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
> R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
> FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
> Stack:
>  000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
>  0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
>  ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
> Call Trace:
>  [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
>  [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
>  [<     inline     >] dst_neigh_output ./include/net/dst.h:464
>  [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
>  [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
>  [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
>  [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
>  [<     inline     >] dst_output ./include/net/dst.h:501
>  [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
>  [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
>  [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
> net/ipv6/ip6_output.c:1732
>  [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
>  [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
>  [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
>  [<     inline     >] sock_sendmsg_nosec net/socket.c:621
>  [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
>  [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
>  [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
>  [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
>  [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
>  [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
>  [<     inline     >] SYSC_writev fs/read_write.c:1017
>  [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
>  [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
> arch/x86/entry/entry_64.S:209
> Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
> 00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
> b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
> RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>  RSP <ffff880034d0eb68>
> CR2: ffffed002d14d74a
> ---[ end trace a73fedfdc11bd60c ]---


Hi Dmitry

I could not reproduce the issue. Might need some specific configuration...

loopback device has proper ethernet header (all 0)

Fault happens in :

0f b6 0c 30             movzbl (%rax,%rsi,1),%ecx

RAX=1ffff1002d14d74a  which is RDI>>3, and RSI=dffffc0000000000

Could this be a KASAN problem ?

[toc] | [prev] | [next] | [standalone]

#1530763

FromAndrey Konovalov <andreyknvl@google.com>
Date2016-11-26 20:10 +0100
Message-ID<sHQwi-2dp-13@gated-at.bofh.it>
In reply to#1530758

[Multipart message — attachments visible in raw view] — view raw

On Sat, Nov 26, 2016 at 7:28 PM, 'Eric Dumazet' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Sat, Nov 26, 2016 at 9:30 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> Hello,
>>
>> The following program triggers GPF in eth_header:
>>
>> https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt
>>
>> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>>
>> BUG: unable to handle kernel paging request at ffffed002d14d74a
>> IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>> PGD 7fff6067 [   50.787819] PUD 7fff5067
>> PMD 0 [   50.787819]
>> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
>> Modules linked in:
>> CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> task: ffff88003a1841c0 task.stack: ffff880034d08000
>> RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
>> eth_header+0x75/0x260 net/ethernet/eth.c:88
>> RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
>> RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
>> RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
>> RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
>> R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
>> R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
>> FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
>> Stack:
>>  000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
>>  0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
>>  ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
>> Call Trace:
>>  [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
>>  [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
>>  [<     inline     >] dst_neigh_output ./include/net/dst.h:464
>>  [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
>>  [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
>>  [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
>>  [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
>>  [<     inline     >] dst_output ./include/net/dst.h:501
>>  [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
>>  [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
>>  [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
>> net/ipv6/ip6_output.c:1732
>>  [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
>>  [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
>>  [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
>>  [<     inline     >] sock_sendmsg_nosec net/socket.c:621
>>  [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
>>  [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
>>  [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
>>  [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
>>  [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
>>  [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
>>  [<     inline     >] SYSC_writev fs/read_write.c:1017
>>  [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
>>  [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
>> arch/x86/entry/entry_64.S:209
>> Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
>> 00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
>> b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
>> RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>>  RSP <ffff880034d0eb68>
>> CR2: ffffed002d14d74a
>> ---[ end trace a73fedfdc11bd60c ]---
>
>
> Hi Dmitry
>
> I could not reproduce the issue. Might need some specific configuration...
>
> loopback device has proper ethernet header (all 0)
>
> Fault happens in :
>
> 0f b6 0c 30             movzbl (%rax,%rsi,1),%ecx
>
> RAX=1ffff1002d14d74a  which is RDI>>3, and RSI=dffffc0000000000
>
> Could this be a KASAN problem ?

Hi Eric,

The crash happens when the kernel tries to access shadow for nonmapped memory.

The issue here is an integer overflow which happens in neigh_resolve_output().
skb_network_offset(skb) can return negative number, but __skb_pull()
accepts unsigned int as len.
As a result, the least significat bit in higher 32 bits of skb->data
gets set and we get an out-of-bounds with offset of 4 GB.

I've attached a short reproducer, but you either need KASAN or to add
a BUG_ON to see the crash.
In this reproducer skb_network_offset() becomes negative after merging
two ipv6 fragments.

I actually see multiple places where skb_network_offset() is used as
an argument to skb_pull().
So I guess every place can potentially be buggy.

Thanks!


>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1530773

FromEric Dumazet <erdlkml@gmail.com>
Date2016-11-26 21:10 +0100
Message-ID<sHRsl-2MG-5@gated-at.bofh.it>
In reply to#1530763
> Hi Eric,
>
> The crash happens when the kernel tries to access shadow for nonmapped memory.
>
> The issue here is an integer overflow which happens in neigh_resolve_output().
> skb_network_offset(skb) can return negative number, but __skb_pull()
> accepts unsigned int as len.
> As a result, the least significat bit in higher 32 bits of skb->data
> gets set and we get an out-of-bounds with offset of 4 GB.
>
> I've attached a short reproducer, but you either need KASAN or to add
> a BUG_ON to see the crash.
> In this reproducer skb_network_offset() becomes negative after merging
> two ipv6 fragments.
>
> I actually see multiple places where skb_network_offset() is used as
> an argument to skb_pull().
> So I guess every place can potentially be buggy.

Well, I think the intent is to accept a negative number.

This definitely was assumed by commit e1f165032c8bade authors !

I guess they were using a 32bit kernel for their tests.

[toc] | [prev] | [next] | [standalone]

#1530776

FromEric Dumazet <erdlkml@gmail.com>
Date2016-11-26 21:40 +0100
Message-ID<sHRVn-2Za-5@gated-at.bofh.it>
In reply to#1530773
2016-11-26 12:05 GMT-08:00 Eric Dumazet <erdlkml@gmail.com>:
>> Hi Eric,
>>
>> The crash happens when the kernel tries to access shadow for nonmapped memory.
>>
>> The issue here is an integer overflow which happens in neigh_resolve_output().
>> skb_network_offset(skb) can return negative number, but __skb_pull()
>> accepts unsigned int as len.
>> As a result, the least significat bit in higher 32 bits of skb->data
>> gets set and we get an out-of-bounds with offset of 4 GB.
>>
>> I've attached a short reproducer, but you either need KASAN or to add
>> a BUG_ON to see the crash.
>> In this reproducer skb_network_offset() becomes negative after merging
>> two ipv6 fragments.
>>
>> I actually see multiple places where skb_network_offset() is used as
>> an argument to skb_pull().
>> So I guess every place can potentially be buggy.
>
> Well, I think the intent is to accept a negative number.
>
> This definitely was assumed by commit e1f165032c8bade authors !
>
> I guess they were using a 32bit kernel for their tests.

Correct fix would be to use

skb_push(skb, -skb_network_offset(skb));

As done in other locations...

[toc] | [prev] | [next] | [standalone]

#1532105

FromAndrey Konovalov <andreyknvl@google.com>
Date2016-11-29 11:30 +0100
Message-ID<sINPH-7eO-1@gated-at.bofh.it>
In reply to#1530773
On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet <erdlkml@gmail.com> wrote:
>> I actually see multiple places where skb_network_offset() is used as
>> an argument to skb_pull().
>> So I guess every place can potentially be buggy.
>
> Well, I think the intent is to accept a negative number.

I'm not sure that was the intent since it results in a signedness
issue which leads to an out-of-bounds.

A quick grep shows that the same issue can potentially happen in
multiple places across the kernel:

net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));

A similar thing also happened to somebody else (on a receive path!):
https://forums.grsecurity.net/viewtopic.php?f=3&t=4550

Does it make sense to check skb_network_offset() before passing it to
skb_pull() everywhere?

>
> This definitely was assumed by commit e1f165032c8bade authors !
>
> I guess they were using a 32bit kernel for their tests.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1532373

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-29 16:00 +0100
Message-ID<sIS31-1lb-59@gated-at.bofh.it>
In reply to#1532105
On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
> On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet <erdlkml@gmail.com> wrote:
> >> I actually see multiple places where skb_network_offset() is used as
> >> an argument to skb_pull().
> >> So I guess every place can potentially be buggy.
> >
> > Well, I think the intent is to accept a negative number.
> 
> I'm not sure that was the intent since it results in a signedness
> issue which leads to an out-of-bounds.
> 

Hey, I already mentioned where was the bug.

You missed the investigation where I pointed it to FLorian ?

> A quick grep shows that the same issue can potentially happen in
> multiple places across the kernel:
> 
> net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
> net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
> net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
> net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
> net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
> net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
> net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
> net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
> net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
> net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
> drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
> drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
> drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
> drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
> 
> A similar thing also happened to somebody else (on a receive path!):
> https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
> 
> Does it make sense to check skb_network_offset() before passing it to
> skb_pull() everywhere?

Well, sure, we could add safety checks everywhere and slow the kernel
when debugging is requested.

But skb_network_offset() is not the problem here. Why are you focusing
on it ?

The real problem is in __skb_pull() or __skb_push() and all similar
helpers. Lots of added checks and slowdowns.

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c535fbccf2c7dbfae04cee393460e86d588c26b..d6116e37d054fc1536114347ed3c41fc7dc7a882 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1923,6 +1923,7 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 unsigned char *skb_push(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->data -= len;
 	skb->len  += len;
 	return skb->data;
@@ -1931,6 +1932,7 @@ static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
 unsigned char *skb_pull(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->len -= len;
 	BUG_ON(skb->len < skb->data_len);
 	return skb->data += len;
@@ -1938,6 +1940,7 @@ static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
 
 static inline unsigned char *skb_pull_inline(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d1d1a5a5ad24ded523fc12ffba8c602b03bd0830..7bf098c848fd857ba5d287fc91d43f62f381bd55 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1450,6 +1450,7 @@ EXPORT_SYMBOL(skb_put);
  */
 unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->data -= len;
 	skb->len  += len;
 	if (unlikely(skb->data<skb->head))

[toc] | [prev] | [next] | [standalone]

#1532423

FromAndrey Konovalov <andreyknvl@google.com>
Date2016-11-29 16:40 +0100
Message-ID<sISFI-1NU-3@gated-at.bofh.it>
In reply to#1532373
On Tue, Nov 29, 2016 at 3:58 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
>> On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet <erdlkml@gmail.com> wrote:
>> >> I actually see multiple places where skb_network_offset() is used as
>> >> an argument to skb_pull().
>> >> So I guess every place can potentially be buggy.
>> >
>> > Well, I think the intent is to accept a negative number.
>>
>> I'm not sure that was the intent since it results in a signedness
>> issue which leads to an out-of-bounds.
>>
>
> Hey, I already mentioned where was the bug.
>
> You missed the investigation where I pointed it to FLorian ?
>
>> A quick grep shows that the same issue can potentially happen in
>> multiple places across the kernel:
>>
>> net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
>> net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
>> net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
>> net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
>> net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
>> net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
>> net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
>> net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
>> net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
>> net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
>> drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
>> drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
>> drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
>> drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
>>
>> A similar thing also happened to somebody else (on a receive path!):
>> https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
>>
>> Does it make sense to check skb_network_offset() before passing it to
>> skb_pull() everywhere?
>
> Well, sure, we could add safety checks everywhere and slow the kernel
> when debugging is requested.
>
> But skb_network_offset() is not the problem here. Why are you focusing
> on it ?
>
> The real problem is in __skb_pull() or __skb_push() and all similar
> helpers. Lots of added checks and slowdowns.

The issue is not with skb_network_offset(), but with __skb_pull()
using skb_network_offset() as an argument.

I'm not sure what would be the beast way to fix this, to add a check
before every __skb_pull(skb_network_offset()), to fix __skb_pull() to
work with signed ints, to add BUG_ON()'s in __skb_pull, or something
else.

What I meant is that you fixed this very instance of the bug, and I'm
pointing out that a similar one might hit us again.

>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 9c535fbccf2c7dbfae04cee393460e86d588c26b..d6116e37d054fc1536114347ed3c41fc7dc7a882 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1923,6 +1923,7 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
>  unsigned char *skb_push(struct sk_buff *skb, unsigned int len);
>  static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
>  {
> +       BUG_ON((int)len < 0);
>         skb->data -= len;
>         skb->len  += len;
>         return skb->data;
> @@ -1931,6 +1932,7 @@ static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
>  unsigned char *skb_pull(struct sk_buff *skb, unsigned int len);
>  static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
>  {
> +       BUG_ON((int)len < 0);
>         skb->len -= len;
>         BUG_ON(skb->len < skb->data_len);
>         return skb->data += len;
> @@ -1938,6 +1940,7 @@ static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
>
>  static inline unsigned char *skb_pull_inline(struct sk_buff *skb, unsigned int len)
>  {
> +       BUG_ON((int)len < 0);
>         return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
>  }
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d1d1a5a5ad24ded523fc12ffba8c602b03bd0830..7bf098c848fd857ba5d287fc91d43f62f381bd55 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1450,6 +1450,7 @@ EXPORT_SYMBOL(skb_put);
>   */
>  unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
>  {
> +       BUG_ON((int)len < 0);
>         skb->data -= len;
>         skb->len  += len;
>         if (unlikely(skb->data<skb->head))
>
>
>
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1532468

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-29 17:20 +0100
Message-ID<sITip-2kT-21@gated-at.bofh.it>
In reply to#1532423
On Tue, 2016-11-29 at 16:31 +0100, Andrey Konovalov wrote:
=
> The issue is not with skb_network_offset(), but with __skb_pull()
> using skb_network_offset() as an argument.
> 

No. The issue can happen with _any_ __skb_pull() with a 'negative'
argument, on 64bit arches.

skb_network_offset() is only one of the many cases this could happen if
a bug is added at some random place, including memory corruption from
a different kernel layer, or buggy hardware.

> I'm not sure what would be the beast way to fix this, to add a check
> before every __skb_pull(skb_network_offset()), to fix __skb_pull() to
> work with signed ints, to add BUG_ON()'s in __skb_pull, or something
> else.
> 
> What I meant is that you fixed this very instance of the bug, and I'm
> pointing out that a similar one might hit us again.

As I said, adding a check in skb_network_offset() would not be generic
enough.

Sure, we can be proactive and add tests everywhere in the kernel, but we
also want to keep it reasonably fast.

[toc] | [prev] | [next] | [standalone]

#1531537

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 20:00 +0100
Message-ID<sIzjI-5Tv-23@gated-at.bofh.it>
In reply to#1530763
On Sat, 2016-11-26 at 20:07 +0100, Andrey Konovalov wrote:
> On Sat, Nov 26, 2016 at 7:28 PM, 'Eric Dumazet' via syzkaller
> <syzkaller@googlegroups.com> wrote:
> > On Sat, Nov 26, 2016 at 9:30 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> >> Hello,
> >>
> >> The following program triggers GPF in eth_header:
> >>
> >> https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt
> >>
> >> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
> >>
> >> BUG: unable to handle kernel paging request at ffffed002d14d74a
> >> IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
> >> PGD 7fff6067 [   50.787819] PUD 7fff5067
> >> PMD 0 [   50.787819]
> >> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
> >> Modules linked in:
> >> CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> task: ffff88003a1841c0 task.stack: ffff880034d08000
> >> RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
> >> eth_header+0x75/0x260 net/ethernet/eth.c:88
> >> RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
> >> RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
> >> RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
> >> RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
> >> R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
> >> R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
> >> FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
> >> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
> >> Stack:
> >>  000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
> >>  0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
> >>  ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
> >> Call Trace:
> >>  [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
> >>  [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
> >>  [<     inline     >] dst_neigh_output ./include/net/dst.h:464
> >>  [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
> >>  [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
> >>  [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
> >>  [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
> >>  [<     inline     >] dst_output ./include/net/dst.h:501
> >>  [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
> >>  [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
> >>  [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
> >> net/ipv6/ip6_output.c:1732
> >>  [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
> >>  [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
> >>  [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
> >>  [<     inline     >] sock_sendmsg_nosec net/socket.c:621
> >>  [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
> >>  [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
> >>  [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
> >>  [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
> >>  [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
> >>  [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
> >>  [<     inline     >] SYSC_writev fs/read_write.c:1017
> >>  [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
> >>  [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
> >> arch/x86/entry/entry_64.S:209
> >> Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
> >> 00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
> >> b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
> >> RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
> >>  RSP <ffff880034d0eb68>
> >> CR2: ffffed002d14d74a
> >> ---[ end trace a73fedfdc11bd60c ]---
> >
> >
> > Hi Dmitry
> >
> > I could not reproduce the issue. Might need some specific configuration...
> >
> > loopback device has proper ethernet header (all 0)
> >
> > Fault happens in :
> >
> > 0f b6 0c 30             movzbl (%rax,%rsi,1),%ecx
> >
> > RAX=1ffff1002d14d74a  which is RDI>>3, and RSI=dffffc0000000000
> >
> > Could this be a KASAN problem ?
> 
> Hi Eric,
> 
> The crash happens when the kernel tries to access shadow for nonmapped memory.
> 
> The issue here is an integer overflow which happens in neigh_resolve_output().
> skb_network_offset(skb) can return negative number, but __skb_pull()
> accepts unsigned int as len.
> As a result, the least significat bit in higher 32 bits of skb->data
> gets set and we get an out-of-bounds with offset of 4 GB.
> 
> I've attached a short reproducer, but you either need KASAN or to add
> a BUG_ON to see the crash.
> In this reproducer skb_network_offset() becomes negative after merging
> two ipv6 fragments.
> 
> I actually see multiple places where skb_network_offset() is used as
> an argument to skb_pull().
> So I guess every place can potentially be buggy.
> 
> Thanks!

I can not reproduce the bug on my hosts.
Quite hard to debug for me.

skb_network_offset() can not be negative at this point, unless there is
a bug upper in the stack.

Hannes, do you have an idea of what could be wrong in IPv6 stack ?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1531540

FromAndrey Konovalov <andreyknvl@google.com>
Date2016-11-28 20:10 +0100
Message-ID<sIztn-6c0-1@gated-at.bofh.it>
In reply to#1531537
On Mon, Nov 28, 2016 at 7:50 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sat, 2016-11-26 at 20:07 +0100, Andrey Konovalov wrote:
>> On Sat, Nov 26, 2016 at 7:28 PM, 'Eric Dumazet' via syzkaller
>> <syzkaller@googlegroups.com> wrote:
>> > On Sat, Nov 26, 2016 at 9:30 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> >> Hello,
>> >>
>> >> The following program triggers GPF in eth_header:
>> >>
>> >> https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt
>> >>
>> >> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>> >>
>> >> BUG: unable to handle kernel paging request at ffffed002d14d74a
>> >> IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>> >> PGD 7fff6067 [   50.787819] PUD 7fff5067
>> >> PMD 0 [   50.787819]
>> >> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
>> >> Modules linked in:
>> >> CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
>> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> >> task: ffff88003a1841c0 task.stack: ffff880034d08000
>> >> RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
>> >> eth_header+0x75/0x260 net/ethernet/eth.c:88
>> >> RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
>> >> RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
>> >> RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
>> >> RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
>> >> R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
>> >> R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
>> >> FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
>> >> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> >> CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
>> >> Stack:
>> >>  000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
>> >>  0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
>> >>  ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
>> >> Call Trace:
>> >>  [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
>> >>  [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
>> >>  [<     inline     >] dst_neigh_output ./include/net/dst.h:464
>> >>  [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
>> >>  [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
>> >>  [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
>> >>  [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
>> >>  [<     inline     >] dst_output ./include/net/dst.h:501
>> >>  [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
>> >>  [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
>> >>  [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
>> >> net/ipv6/ip6_output.c:1732
>> >>  [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
>> >>  [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
>> >>  [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
>> >>  [<     inline     >] sock_sendmsg_nosec net/socket.c:621
>> >>  [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
>> >>  [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
>> >>  [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
>> >>  [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
>> >>  [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
>> >>  [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
>> >>  [<     inline     >] SYSC_writev fs/read_write.c:1017
>> >>  [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
>> >>  [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
>> >> arch/x86/entry/entry_64.S:209
>> >> Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
>> >> 00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
>> >> b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
>> >> RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>> >>  RSP <ffff880034d0eb68>
>> >> CR2: ffffed002d14d74a
>> >> ---[ end trace a73fedfdc11bd60c ]---
>> >
>> >
>> > Hi Dmitry
>> >
>> > I could not reproduce the issue. Might need some specific configuration...
>> >
>> > loopback device has proper ethernet header (all 0)
>> >
>> > Fault happens in :
>> >
>> > 0f b6 0c 30             movzbl (%rax,%rsi,1),%ecx
>> >
>> > RAX=1ffff1002d14d74a  which is RDI>>3, and RSI=dffffc0000000000
>> >
>> > Could this be a KASAN problem ?
>>
>> Hi Eric,
>>
>> The crash happens when the kernel tries to access shadow for nonmapped memory.
>>
>> The issue here is an integer overflow which happens in neigh_resolve_output().
>> skb_network_offset(skb) can return negative number, but __skb_pull()
>> accepts unsigned int as len.
>> As a result, the least significat bit in higher 32 bits of skb->data
>> gets set and we get an out-of-bounds with offset of 4 GB.
>>
>> I've attached a short reproducer, but you either need KASAN or to add
>> a BUG_ON to see the crash.
>> In this reproducer skb_network_offset() becomes negative after merging
>> two ipv6 fragments.
>>
>> I actually see multiple places where skb_network_offset() is used as
>> an argument to skb_pull().
>> So I guess every place can potentially be buggy.
>>
>> Thanks!
>
> I can not reproduce the bug on my hosts.
> Quite hard to debug for me.
>
> skb_network_offset() can not be negative at this point, unless there is
> a bug upper in the stack.

Hi Eric,

As far as I can see, skb_network_offset() becomes negative after
pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
At least I'm able to detect that with a BUG_ON().

Also it seems that the issue is only reproducible (at least with the
poc I provided) for a short time after boot.

I hope that helps.

>
> Hannes, do you have an idea of what could be wrong in IPv6 stack ?
>
> Thanks.
>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[toc] | [prev] | [next] | [standalone]

#1531559

FromDmitry Vyukov <dvyukov@google.com>
Date2016-11-28 20:40 +0100
Message-ID<sIzWq-6lO-41@gated-at.bofh.it>
In reply to#1531540
On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
<syzkaller@googlegroups.com> wrote:
> On Mon, Nov 28, 2016 at 7:50 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Sat, 2016-11-26 at 20:07 +0100, Andrey Konovalov wrote:
>>> On Sat, Nov 26, 2016 at 7:28 PM, 'Eric Dumazet' via syzkaller
>>> <syzkaller@googlegroups.com> wrote:
>>> > On Sat, Nov 26, 2016 at 9:30 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> >> Hello,
>>> >>
>>> >> The following program triggers GPF in eth_header:
>>> >>
>>> >> https://gist.githubusercontent.com/dvyukov/613cadf05543b55a419f237e419cd495/raw/5471231523d1a07c3de55f11f87472c2816ee06c/gistfile1.txt
>>> >>
>>> >> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>>> >>
>>> >> BUG: unable to handle kernel paging request at ffffed002d14d74a
>>> >> IP: [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>>> >> PGD 7fff6067 [   50.787819] PUD 7fff5067
>>> >> PMD 0 [   50.787819]
>>> >> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN
>>> >> Modules linked in:
>>> >> CPU: 2 PID: 6712 Comm: a.out Not tainted 4.9.0-rc6+ #55
>>> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>> >> task: ffff88003a1841c0 task.stack: ffff880034d08000
>>> >> RIP: 0010:[<ffffffff86be3295>]  [<ffffffff86be3295>]
>>> >> eth_header+0x75/0x260 net/ethernet/eth.c:88
>>> >> RSP: 0018:ffff880034d0eb68  EFLAGS: 00010a03
>>> >> RAX: 1ffff1002d14d74a RBX: ffff880168a6ba4a RCX: ffff88006a9c7858
>>> >> RDX: 000000000000dd86 RSI: dffffc0000000000 RDI: ffff880168a6ba56
>>> >> RBP: ffff880034d0eb98 R08: 0000000000000000 R09: 0000000000000031
>>> >> R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000000
>>> >> R13: ffff88006c208d80 R14: 00000000000086dd R15: ffff88006a9c7858
>>> >> FS:  0000000001a02940(0000) GS:ffff88006d000000(0000) knlGS:0000000000000000
>>> >> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> >> CR2: ffffed002d14d74a CR3: 0000000037373000 CR4: 00000000000006e0
>>> >> Stack:
>>> >>  000000316881ab40 ffff88006a9c76c0 ffff88006881ab40 ffff88006a9c77f8
>>> >>  0000000000000000 dffffc0000000000 ffff880034d0ee98 ffffffff86b31af9
>>> >>  ffffffff8719605c ffff880034d0f0f8 ffffffff000086dd ffffffff86be3220
>>> >> Call Trace:
>>> >>  [<     inline     >] dev_hard_header ./include/linux/netdevice.h:2762
>>> >>  [<ffffffff86b31af9>] neigh_resolve_output+0x659/0xb20 net/core/neighbour.c:1302
>>> >>  [<     inline     >] dst_neigh_output ./include/net/dst.h:464
>>> >>  [<ffffffff8719605c>] ip6_finish_output2+0xb3c/0x2500 net/ipv6/ip6_output.c:121
>>> >>  [<ffffffff871a0b0b>] ip6_finish_output+0x2eb/0x760 net/ipv6/ip6_output.c:139
>>> >>  [<     inline     >] NF_HOOK_COND ./include/linux/netfilter.h:246
>>> >>  [<ffffffff871a1157>] ip6_output+0x1d7/0x9a0 net/ipv6/ip6_output.c:153
>>> >>  [<     inline     >] dst_output ./include/net/dst.h:501
>>> >>  [<ffffffff873312ea>] ip6_local_out+0x9a/0x180 net/ipv6/output_core.c:170
>>> >>  [<ffffffff871a3886>] ip6_send_skb+0xa6/0x340 net/ipv6/ip6_output.c:1712
>>> >>  [<ffffffff871a3bd8>] ip6_push_pending_frames+0xb8/0xe0
>>> >> net/ipv6/ip6_output.c:1732
>>> >>  [<     inline     >] rawv6_push_pending_frames net/ipv6/raw.c:607
>>> >>  [<ffffffff8722acfb>] rawv6_sendmsg+0x250b/0x2c20 net/ipv6/raw.c:920
>>> >>  [<ffffffff8701c4f5>] inet_sendmsg+0x385/0x590 net/ipv4/af_inet.c:734
>>> >>  [<     inline     >] sock_sendmsg_nosec net/socket.c:621
>>> >>  [<ffffffff86a6ea9f>] sock_sendmsg+0xcf/0x110 net/socket.c:631
>>> >>  [<ffffffff86a6ee0b>] sock_write_iter+0x32b/0x620 net/socket.c:829
>>> >>  [<ffffffff81a6f153>] do_iter_readv_writev+0x363/0x670 fs/read_write.c:695
>>> >>  [<ffffffff81a71ba1>] do_readv_writev+0x431/0x9b0 fs/read_write.c:872
>>> >>  [<ffffffff81a726dc>] vfs_writev+0x8c/0xc0 fs/read_write.c:911
>>> >>  [<ffffffff81a72825>] do_writev+0x115/0x2d0 fs/read_write.c:944
>>> >>  [<     inline     >] SYSC_writev fs/read_write.c:1017
>>> >>  [<ffffffff81a75fdc>] SyS_writev+0x2c/0x40 fs/read_write.c:1014
>>> >>  [<ffffffff8814cf85>] entry_SYSCALL_64_fastpath+0x23/0xc6
>>> >> arch/x86/entry/entry_64.S:209
>>> >> Code: 41 83 fe 04 0f 84 aa 00 00 00 e8 17 4e b0 fa 48 8d 7b 0c 48 be
>>> >> 00 00 00 00 00 fc ff df 44 89 f2 66 c1 c2 08 48 89 f8 48 c1 e8 03 <0f>
>>> >> b6 0c 30 48 8d 43 0d 49 89 c0 49 c1 e8 03 41 0f b6 34 30 49
>>> >> RIP  [<ffffffff86be3295>] eth_header+0x75/0x260 net/ethernet/eth.c:88
>>> >>  RSP <ffff880034d0eb68>
>>> >> CR2: ffffed002d14d74a
>>> >> ---[ end trace a73fedfdc11bd60c ]---
>>> >
>>> >
>>> > Hi Dmitry
>>> >
>>> > I could not reproduce the issue. Might need some specific configuration...
>>> >
>>> > loopback device has proper ethernet header (all 0)
>>> >
>>> > Fault happens in :
>>> >
>>> > 0f b6 0c 30             movzbl (%rax,%rsi,1),%ecx
>>> >
>>> > RAX=1ffff1002d14d74a  which is RDI>>3, and RSI=dffffc0000000000
>>> >
>>> > Could this be a KASAN problem ?
>>>
>>> Hi Eric,
>>>
>>> The crash happens when the kernel tries to access shadow for nonmapped memory.
>>>
>>> The issue here is an integer overflow which happens in neigh_resolve_output().
>>> skb_network_offset(skb) can return negative number, but __skb_pull()
>>> accepts unsigned int as len.
>>> As a result, the least significat bit in higher 32 bits of skb->data
>>> gets set and we get an out-of-bounds with offset of 4 GB.
>>>
>>> I've attached a short reproducer, but you either need KASAN or to add
>>> a BUG_ON to see the crash.
>>> In this reproducer skb_network_offset() becomes negative after merging
>>> two ipv6 fragments.
>>>
>>> I actually see multiple places where skb_network_offset() is used as
>>> an argument to skb_pull().
>>> So I guess every place can potentially be buggy.
>>>
>>> Thanks!
>>
>> I can not reproduce the bug on my hosts.
>> Quite hard to debug for me.
>>
>> skb_network_offset() can not be negative at this point, unless there is
>> a bug upper in the stack.
>
> Hi Eric,
>
> As far as I can see, skb_network_offset() becomes negative after
> pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
> At least I'm able to detect that with a BUG_ON().
>
> Also it seems that the issue is only reproducible (at least with the
> poc I provided) for a short time after boot.


Eric,

Is it enough to debug? Or maybe Andrey can trace some values for you.

[toc] | [prev] | [next] | [standalone]

#1531561

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 20:50 +0100
Message-ID<sIA66-6pe-7@gated-at.bofh.it>
In reply to#1531559
On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
> On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller

> > Hi Eric,
> >
> > As far as I can see, skb_network_offset() becomes negative after
> > pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
> > At least I'm able to detect that with a BUG_ON().
> >
> > Also it seems that the issue is only reproducible (at least with the
> > poc I provided) for a short time after boot.
> 
> 
> Eric,
> 
> Is it enough to debug? Or maybe Andrey can trace some values for you.

Well, now we are talking, if you tell me how many modules you load, it
might help ;)

nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
explain why I could not reproduce the bug.

Let me try ;)

[toc] | [prev] | [next] | [standalone]

#1531667

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 22:10 +0100
Message-ID<sIBlw-7lp-45@gated-at.bofh.it>
In reply to#1531561
On Mon, 2016-11-28 at 11:47 -0800, Eric Dumazet wrote:
> On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
> > On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
> 
> > > Hi Eric,
> > >
> > > As far as I can see, skb_network_offset() becomes negative after
> > > pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
> > > At least I'm able to detect that with a BUG_ON().
> > >
> > > Also it seems that the issue is only reproducible (at least with the
> > > poc I provided) for a short time after boot.
> > 
> > 
> > Eric,
> > 
> > Is it enough to debug? Or maybe Andrey can trace some values for you.
> 
> Well, now we are talking, if you tell me how many modules you load, it
> might help ;)
> 
> nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
> explain why I could not reproduce the bug.
> 
> Let me try ;)
> 

Might be a bug added in commit daaa7d647f81f3
("netfilter: ipv6: avoid nf_iterate recursion")

Florian, what do you think of dropping a packet that presumably was
mangled badly by nf_ct_frag6_queue() ?

(Like about 48 byte pulled :(, and/or skb->csum changed )

diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
index f7aab5ab93a5..31aa947332d8 100644
--- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
+++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
@@ -65,8 +65,8 @@ static unsigned int ipv6_defrag(void *priv,
 
 	err = nf_ct_frag6_gather(state->net, skb,
 				 nf_ct6_defrag_user(state->hook, skb));
-	/* queued */
-	if (err == -EINPROGRESS)
+	/* queued or mangled ... */
+	if (err)
 		return NF_STOLEN;
 
 	return NF_ACCEPT;

[toc] | [prev] | [next] | [standalone]

#1531670

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 22:20 +0100
Message-ID<sIBvb-7oU-1@gated-at.bofh.it>
In reply to#1531667
On Mon, 2016-11-28 at 13:05 -0800, Eric Dumazet wrote:
> On Mon, 2016-11-28 at 11:47 -0800, Eric Dumazet wrote:
> > On Mon, 2016-11-28 at 20:34 +0100, Dmitry Vyukov wrote:
> > > On Mon, Nov 28, 2016 at 8:04 PM, 'Andrey Konovalov' via syzkaller
> > 
> > > > Hi Eric,
> > > >
> > > > As far as I can see, skb_network_offset() becomes negative after
> > > > pskb_pull(skb, (u8 *) (fhdr + 1) - skb->data) in nf_ct_frag6_queue().
> > > > At least I'm able to detect that with a BUG_ON().
> > > >
> > > > Also it seems that the issue is only reproducible (at least with the
> > > > poc I provided) for a short time after boot.
> > > 
> > > 
> > > Eric,
> > > 
> > > Is it enough to debug? Or maybe Andrey can trace some values for you.
> > 
> > Well, now we are talking, if you tell me how many modules you load, it
> > might help ;)
> > 
> > nf_ct_frag6_queue is nowhere to be seen in my kernels, that might
> > explain why I could not reproduce the bug.
> > 
> > Let me try ;)
> > 
> 
> Might be a bug added in commit daaa7d647f81f3
> ("netfilter: ipv6: avoid nf_iterate recursion")
> 
> Florian, what do you think of dropping a packet that presumably was
> mangled badly by nf_ct_frag6_queue() ?
> 
> (Like about 48 byte pulled :(, and/or skb->csum changed )
> 
> diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> index f7aab5ab93a5..31aa947332d8 100644
> --- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> +++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> @@ -65,8 +65,8 @@ static unsigned int ipv6_defrag(void *priv,
>  
>  	err = nf_ct_frag6_gather(state->net, skb,
>  				 nf_ct6_defrag_user(state->hook, skb));
> -	/* queued */
> -	if (err == -EINPROGRESS)
> +	/* queued or mangled ... */
> +	if (err)
>  		return NF_STOLEN;

Or more exactly , returning NF_DROP so that skb is really freed ;)


diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
index f7aab5ab93a5..508739a3ca2a 100644
--- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
+++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
@@ -65,9 +65,9 @@ static unsigned int ipv6_defrag(void *priv,
 
 	err = nf_ct_frag6_gather(state->net, skb,
 				 nf_ct6_defrag_user(state->hook, skb));
-	/* queued */
-	if (err == -EINPROGRESS)
-		return NF_STOLEN;
+	/* queued or mangled ... */
+	if (err)
+		return (err == -EINPROGRESS) ? NF_STOLEN : NF_DROP;
 
 	return NF_ACCEPT;
 }

[toc] | [prev] | [next] | [standalone]

#1531691

FromFlorian Westphal <fw@strlen.de>
Date2016-11-28 22:40 +0100
Message-ID<sIBOy-7vt-41@gated-at.bofh.it>
In reply to#1531670
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Might be a bug added in commit daaa7d647f81f3
> > ("netfilter: ipv6: avoid nf_iterate recursion")
> > 
> > Florian, what do you think of dropping a packet that presumably was
> > mangled badly by nf_ct_frag6_queue() ?

ipv4 definitely frees malformed packets.
In general, I think netfilter should avoid 'silent' drops if possible
and let skb continue, but of course such skbs should not be made worse
as what we ate to begin with...

> > (Like about 48 byte pulled :(, and/or skb->csum changed )

I think this warrants a review of ipv6 reassembly too, bug reported here
is because ipv6 nf defrag is also done on output.

> diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> index f7aab5ab93a5..508739a3ca2a 100644
> --- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> +++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
> @@ -65,9 +65,9 @@ static unsigned int ipv6_defrag(void *priv,
>  
>  	err = nf_ct_frag6_gather(state->net, skb,
>  				 nf_ct6_defrag_user(state->hook, skb));
> -	/* queued */
> -	if (err == -EINPROGRESS)
> -		return NF_STOLEN;
> +	/* queued or mangled ... */
> +	if (err)
> +		return (err == -EINPROGRESS) ? NF_STOLEN : NF_DROP;
>  
>  	return NF_ACCEPT;

Looks good, we'll need to change some of the errno return codes in
nf_ct_frag6_gather to 0 though for this to work, which should not be too
hard ;)

[toc] | [prev] | [next] | [standalone]

#1531738

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-28 23:20 +0100
Message-ID<sICrg-82A-21@gated-at.bofh.it>
In reply to#1531691
On Mon, 2016-11-28 at 22:34 +0100, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > Might be a bug added in commit daaa7d647f81f3
> > > ("netfilter: ipv6: avoid nf_iterate recursion")
> > > 
> > > Florian, what do you think of dropping a packet that presumably was
> > > mangled badly by nf_ct_frag6_queue() ?
> 
> ipv4 definitely frees malformed packets.
> In general, I think netfilter should avoid 'silent' drops if possible
> and let skb continue, but of course such skbs should not be made worse
> as what we ate to begin with...
> 
> > > (Like about 48 byte pulled :(, and/or skb->csum changed )
> 
> I think this warrants a review of ipv6 reassembly too, bug reported here
> is because ipv6 nf defrag is also done on output.


ip6_frag_queue() definitely frees bad/mangled skbs()


> Looks good, we'll need to change some of the errno return codes in
> nf_ct_frag6_gather to 0 though for this to work, which should not be too
> hard ;)

If the goal is to let buggy packets pass, then we might need to undo
changes in nf_ct_frag6_queue()

[toc] | [prev] | [next] | [standalone]

#1531743

FromFlorian Westphal <fw@strlen.de>
Date2016-11-28 23:30 +0100
Message-ID<sICAW-85L-17@gated-at.bofh.it>
In reply to#1531738
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2016-11-28 at 22:34 +0100, Florian Westphal wrote:
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > > Might be a bug added in commit daaa7d647f81f3
> > > > ("netfilter: ipv6: avoid nf_iterate recursion")
> > > > 
> > > > Florian, what do you think of dropping a packet that presumably was
> > > > mangled badly by nf_ct_frag6_queue() ?
> > 
> > ipv4 definitely frees malformed packets.
> > In general, I think netfilter should avoid 'silent' drops if possible
> > and let skb continue, but of course such skbs should not be made worse
> > as what we ate to begin with...
> > 
> > > > (Like about 48 byte pulled :(, and/or skb->csum changed )
> > 
> > I think this warrants a review of ipv6 reassembly too, bug reported here
> > is because ipv6 nf defrag is also done on output.
> 
> 
> ip6_frag_queue() definitely frees bad/mangled skbs()

Yes, sorry.  nf_ct_frag6_queue is mostly derived from ip6_frag_queue
so any bugs in one might also exist in other.
Thats all I wanted to say here.  I'll check this tomorrow.

> > Looks good, we'll need to change some of the errno return codes in
> > nf_ct_frag6_gather to 0 though for this to work, which should not be too
> > hard ;)
> 
> If the goal is to let buggy packets pass, then we might need to undo
> changes in nf_ct_frag6_queue()

It currently returns -EINVAL in cases where skb wasn't changed/altered
(e.g. because it doesn't have a fragment header), so we should ACCEPT in
that case.

As for 'buggy' packet, I think its ok to mimic ip6_frag_queue, i.e.
if it tosses returning NF_DROP under same circumstance seems ok.

(Passing however will -- on ingress side -- cause snmp stat increments
 in ipv6 reassembly, this still might be desireable).

I'll check where undo might be possible/not too hard.

Thanks Eric for debugging this!

[toc] | [prev] | [next] | [standalone]

#1531802

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-11-29 00:20 +0100
Message-ID<sIDnj-aD-31@gated-at.bofh.it>
In reply to#1531743
On Mon, 2016-11-28 at 23:19 +0100, Florian Westphal wrote:

> It currently returns -EINVAL in cases where skb wasn't changed/altered
> (e.g. because it doesn't have a fragment header), so we should ACCEPT in
> that case.
> 

Maybe nf_ct_frag6_queue() should return direct NF_ codes then ...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web