* general protection fault in nft_chain_parse_hook @ 2020-01-15 20:51 syzbot 2020-01-16 18:03 ` syzbot ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: syzbot @ 2020-01-15 20:51 UTC (permalink / raw) To: coreteam, davem, fw, kadlec, linux-kernel, netdev, netfilter-devel, pablo, syzkaller-bugs Hello, syzbot found the following crash on: HEAD commit: 4e2fa6b9 Merge branch 'bridge-add-vlan-notifications-and-r.. git tree: net-next console output: https://syzkaller.appspot.com/x/log.txt?x=142f4a21e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=66d8660c57ff3c98 dashboard link: https://syzkaller.appspot.com/bug?extid=156a04714799b1d480bc compiler: gcc (GCC) 9.0.0 20181231 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN CPU: 1 PID: 20602 Comm: syz-executor.1 Not tainted 5.5.0-rc5-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:nft_chain_parse_hook+0x386/0xa10 net/netfilter/nf_tables_api.c:1767 Code: e8 7f ae 09 fb 41 83 fd 05 0f 87 62 05 00 00 e8 f0 ac 09 fb 49 8d 7c 24 18 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e a6 05 00 00 44 89 e9 be 01 00 RSP: 0018:ffffc90001627100 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: ffffc900016272b0 RCX: ffffc90004e59000 RDX: 0000000000000003 RSI: ffffffff866b5350 RDI: 0000000000000018 RBP: ffffc900016271f0 R08: ffff8880648b4240 R09: 0000000000000000 R10: fffff520002c4e2f R11: ffffc9000162717f R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: ffffc900016271c8 FS: 00007f0770558700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fcdd829cdb8 CR3: 00000000a483c000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: nf_tables_addchain.constprop.0+0x1c1/0x1520 net/netfilter/nf_tables_api.c:1888 nf_tables_newchain+0x1033/0x1820 net/netfilter/nf_tables_api.c:2196 nfnetlink_rcv_batch+0xf42/0x17a0 net/netfilter/nfnetlink.c:433 nfnetlink_rcv_skb_batch net/netfilter/nfnetlink.c:543 [inline] nfnetlink_rcv+0x3e7/0x460 net/netfilter/nfnetlink.c:561 netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline] netlink_unicast+0x59e/0x7e0 net/netlink/af_netlink.c:1328 netlink_sendmsg+0x91c/0xea0 net/netlink/af_netlink.c:1917 sock_sendmsg_nosec net/socket.c:652 [inline] sock_sendmsg+0xd7/0x130 net/socket.c:672 ____sys_sendmsg+0x753/0x880 net/socket.c:2343 ___sys_sendmsg+0x100/0x170 net/socket.c:2397 __sys_sendmsg+0x105/0x1d0 net/socket.c:2430 __do_sys_sendmsg net/socket.c:2439 [inline] __se_sys_sendmsg net/socket.c:2437 [inline] __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2437 do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x45aff9 Code: ad b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f0770557c78 EFLAGS: 00000246 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 00007f07705586d4 RCX: 000000000045aff9 RDX: 00000000040c4080 RSI: 000000002000c2c0 RDI: 0000000000000003 RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff R13: 0000000000000901 R14: 00000000004ca2fe R15: 000000000075bf2c Modules linked in: ---[ end trace c2d6a3781de0914d ]--- RIP: 0010:nft_chain_parse_hook+0x386/0xa10 net/netfilter/nf_tables_api.c:1767 Code: e8 7f ae 09 fb 41 83 fd 05 0f 87 62 05 00 00 e8 f0 ac 09 fb 49 8d 7c 24 18 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e a6 05 00 00 44 89 e9 be 01 00 RSP: 0018:ffffc90001627100 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: ffffc900016272b0 RCX: ffffc90004e59000 RDX: 0000000000000003 RSI: ffffffff866b5350 RDI: 0000000000000018 RBP: ffffc900016271f0 R08: ffff8880648b4240 R09: 0000000000000000 R10: fffff520002c4e2f R11: ffffc9000162717f R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: ffffc900016271c8 FS: 00007f0770558700(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fd461cd0000 CR3: 00000000a483c000 CR4: 00000000001406e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: general protection fault in nft_chain_parse_hook 2020-01-15 20:51 general protection fault in nft_chain_parse_hook syzbot @ 2020-01-16 18:03 ` syzbot 2020-01-16 21:11 ` [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference Florian Westphal 2020-01-17 18:57 ` general protection fault in nft_chain_parse_hook syzbot 2 siblings, 0 replies; 8+ messages in thread From: syzbot @ 2020-01-16 18:03 UTC (permalink / raw) To: coreteam, davem, fw, kadlec, linux-kernel, netdev, netfilter-devel, pablo, syzkaller-bugs syzbot has found a reproducer for the following crash on: HEAD commit: f5ae2ea6 Fix built-in early-load Intel microcode alignment git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=112c92d1e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=d9290aeb7e6cf1c4 dashboard link: https://syzkaller.appspot.com/bug?extid=156a04714799b1d480bc compiler: gcc (GCC) 9.0.0 20181231 (experimental) userspace arch: i386 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=110253aee00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=170d8159e00000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com kasan: CONFIG_KASAN_INLINE enabled kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 9678 Comm: syz-executor546 Not tainted 5.5.0-rc6-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:nft_chain_parse_hook+0x386/0xa10 net/netfilter/nf_tables_api.c:1767 Code: e8 5f 27 0e fb 41 83 fd 05 0f 87 62 05 00 00 e8 d0 25 0e fb 49 8d 7c 24 18 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e a6 05 00 00 44 89 e9 be 01 00 RSP: 0018:ffffc900021370f0 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: ffffc900021372a0 RCX: ffffffff8666cfa1 RDX: 0000000000000003 RSI: ffffffff8666cfb0 RDI: 0000000000000018 RBP: ffffc900021371e0 R08: ffff88809c7ce380 R09: 0000000000000000 R10: fffff52000426e2d R11: ffffc9000213716f R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: ffffc900021371b8 FS: 0000000000000000(0000) GS:ffff8880ae800000(0063) knlGS:0000000009dfd840 CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033 CR2: 0000000020000280 CR3: 00000000a29dd000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: nf_tables_addchain.constprop.0+0x1c1/0x1520 net/netfilter/nf_tables_api.c:1888 nf_tables_newchain+0x1033/0x1820 net/netfilter/nf_tables_api.c:2196 nfnetlink_rcv_batch+0xf42/0x17a0 net/netfilter/nfnetlink.c:433 nfnetlink_rcv_skb_batch net/netfilter/nfnetlink.c:543 [inline] nfnetlink_rcv+0x3e7/0x460 net/netfilter/nfnetlink.c:561 netlink_unicast_kernel net/netlink/af_netlink.c:1302 [inline] netlink_unicast+0x58c/0x7d0 net/netlink/af_netlink.c:1328 netlink_sendmsg+0x91c/0xea0 net/netlink/af_netlink.c:1917 sock_sendmsg_nosec net/socket.c:639 [inline] sock_sendmsg+0xd7/0x130 net/socket.c:659 ____sys_sendmsg+0x753/0x880 net/socket.c:2330 ___sys_sendmsg+0x100/0x170 net/socket.c:2384 __sys_sendmsg+0x105/0x1d0 net/socket.c:2417 __compat_sys_sendmsg net/compat.c:642 [inline] __do_compat_sys_sendmsg net/compat.c:649 [inline] __se_compat_sys_sendmsg net/compat.c:646 [inline] __ia32_compat_sys_sendmsg+0x7a/0xb0 net/compat.c:646 do_syscall_32_irqs_on arch/x86/entry/common.c:337 [inline] do_fast_syscall_32+0x27b/0xe16 arch/x86/entry/common.c:408 entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139 RIP: 0023:0xf7fafa39 Code: 00 00 00 89 d3 5b 5e 5f 5d c3 b8 80 96 98 00 eb c4 8b 04 24 c3 8b 1c 24 c3 8b 34 24 c3 8b 3c 24 c3 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90 RSP: 002b:00000000ffc60f6c EFLAGS: 00000202 ORIG_RAX: 0000000000000172 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 000000002000d400 RDX: 0000000004000000 RSI: 00000000080ea00c RDI: 0000000000000000 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 Modules linked in: ---[ end trace ef2c8b24d08b7122 ]--- RIP: 0010:nft_chain_parse_hook+0x386/0xa10 net/netfilter/nf_tables_api.c:1767 Code: e8 5f 27 0e fb 41 83 fd 05 0f 87 62 05 00 00 e8 d0 25 0e fb 49 8d 7c 24 18 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e a6 05 00 00 44 89 e9 be 01 00 RSP: 0018:ffffc900021370f0 EFLAGS: 00010206 RAX: dffffc0000000000 RBX: ffffc900021372a0 RCX: ffffffff8666cfa1 RDX: 0000000000000003 RSI: ffffffff8666cfb0 RDI: 0000000000000018 RBP: ffffc900021371e0 R08: ffff88809c7ce380 R09: 0000000000000000 R10: fffff52000426e2d R11: ffffc9000213716f R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: ffffc900021371b8 FS: 0000000000000000(0000) GS:ffff8880ae800000(0063) knlGS:0000000009dfd840 CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033 CR2: 0000000020000280 CR3: 00000000a29dd000 CR4: 00000000001406f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference 2020-01-15 20:51 general protection fault in nft_chain_parse_hook syzbot 2020-01-16 18:03 ` syzbot @ 2020-01-16 21:11 ` Florian Westphal 2020-01-18 20:30 ` Pablo Neira Ayuso 2020-01-17 18:57 ` general protection fault in nft_chain_parse_hook syzbot 2 siblings, 1 reply; 8+ messages in thread From: Florian Westphal @ 2020-01-16 21:11 UTC (permalink / raw) To: netfilter-devel Cc: netdev, syzkaller-bugs, Florian Westphal, syzbot+156a04714799b1d480bc Its possible to create tables in a family that isn't supported/known. Then, when adding a base chain, the table pointer can be NULL. This gets us a NULL ptr dereference in nf_tables_addchain(). Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/nf_tables_api.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 65f51a2e9c2a..e8976128cdb1 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, struct nft_ctx ctx; int err; + if (family >= NFPROTO_NUMPROTO) + return -EAFNOSUPPORT; + lockdep_assert_held(&net->nft.commit_mutex); attr = nla[NFTA_TABLE_NAME]; table = nft_table_lookup(net, attr, family, genmask); @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, ha[NFTA_HOOK_PRIORITY] == NULL) return -EINVAL; + if (family >= NFPROTO_NUMPROTO) + return -EAFNOSUPPORT; + hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, family, autoload); if (IS_ERR(type)) return PTR_ERR(type); + } else if (!type) { + return -EOPNOTSUPP; } if (hook->num > NF_MAX_HOOKS || !(type->hook_mask & (1 << hook->num))) return -EOPNOTSUPP; -- 2.24.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference 2020-01-16 21:11 ` [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference Florian Westphal @ 2020-01-18 20:30 ` Pablo Neira Ayuso 2020-01-18 23:28 ` Florian Westphal 2020-01-21 13:26 ` Pablo Neira Ayuso 0 siblings, 2 replies; 8+ messages in thread From: Pablo Neira Ayuso @ 2020-01-18 20:30 UTC (permalink / raw) To: Florian Westphal Cc: netfilter-devel, netdev, syzkaller-bugs, syzbot+156a04714799b1d480bc On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > Its possible to create tables in a family that isn't supported/known. > Then, when adding a base chain, the table pointer can be NULL. > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/netfilter/nf_tables_api.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 65f51a2e9c2a..e8976128cdb1 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > struct nft_ctx ctx; > int err; > > + if (family >= NFPROTO_NUMPROTO) > + return -EAFNOSUPPORT; > + > lockdep_assert_held(&net->nft.commit_mutex); > attr = nla[NFTA_TABLE_NAME]; > table = nft_table_lookup(net, attr, family, genmask); > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > ha[NFTA_HOOK_PRIORITY] == NULL) > return -EINVAL; > > + if (family >= NFPROTO_NUMPROTO) > + return -EAFNOSUPPORT; > + > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > family, autoload); > if (IS_ERR(type)) > return PTR_ERR(type); > + } else if (!type) { > + return -EOPNOTSUPP; I think this check should be enough. I mean, NFPROTO_NUMPROTO still allows for creating tables for families that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such table. As long as such table does not crash the kernel, I think it's fine. No changes can be attached anymore anyway. Otherwise, if a helper function to check for the families that are really supported could be another alternative. But not sure it is worth? Let me know, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference 2020-01-18 20:30 ` Pablo Neira Ayuso @ 2020-01-18 23:28 ` Florian Westphal 2020-01-21 13:26 ` Pablo Neira Ayuso 1 sibling, 0 replies; 8+ messages in thread From: Florian Westphal @ 2020-01-18 23:28 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: Florian Westphal, netfilter-devel, netdev, syzkaller-bugs, syzbot+156a04714799b1d480bc Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > > Its possible to create tables in a family that isn't supported/known. > > Then, when adding a base chain, the table pointer can be NULL. > > > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/netfilter/nf_tables_api.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 65f51a2e9c2a..e8976128cdb1 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > > struct nft_ctx ctx; > > int err; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > lockdep_assert_held(&net->nft.commit_mutex); > > attr = nla[NFTA_TABLE_NAME]; > > table = nft_table_lookup(net, attr, family, genmask); > > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > > ha[NFTA_HOOK_PRIORITY] == NULL) > > return -EINVAL; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > > family, autoload); > > if (IS_ERR(type)) > > return PTR_ERR(type); > > + } else if (!type) { > > + return -EOPNOTSUPP; > > I think this check should be enough. > > I mean, NFPROTO_NUMPROTO still allows for creating tables for families > that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such > table. As long as such table does not crash the kernel, I think it's > fine. No changes can be attached anymore anyway. I had a previous (not-sent) version that added a stronger validation of nfproto during table creation but I ditched it because it got ugly due to ifdefs. As you alrady point out, creating "bogus" family tables is fine, base chain registration will fail later on from netfilter core. The NFPROTO_NUMPROTO range check is needed, we use it as index to table[] in nft_chain_parse_hook(). > Otherwise, if a helper function to check for the families that are > really supported could be another alternative. But not sure it is > worth? It did not look nice so I did not go for it in the end, but I think doing a simple range check doesn't hurt. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference 2020-01-18 20:30 ` Pablo Neira Ayuso 2020-01-18 23:28 ` Florian Westphal @ 2020-01-21 13:26 ` Pablo Neira Ayuso 2020-01-21 14:35 ` Florian Westphal 1 sibling, 1 reply; 8+ messages in thread From: Pablo Neira Ayuso @ 2020-01-21 13:26 UTC (permalink / raw) To: Florian Westphal Cc: netfilter-devel, netdev, syzkaller-bugs, syzbot+156a04714799b1d480bc [-- Attachment #1: Type: text/plain, Size: 2559 bytes --] On Sat, Jan 18, 2020 at 09:30:57PM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 16, 2020 at 10:11:09PM +0100, Florian Westphal wrote: > > Its possible to create tables in a family that isn't supported/known. > > Then, when adding a base chain, the table pointer can be NULL. > > > > This gets us a NULL ptr dereference in nf_tables_addchain(). > > > > Fixes: baae3e62f31618 ("netfilter: nf_tables: fix chain type module reference handling") > > Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/netfilter/nf_tables_api.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 65f51a2e9c2a..e8976128cdb1 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -953,6 +953,9 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > > struct nft_ctx ctx; > > int err; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > lockdep_assert_held(&net->nft.commit_mutex); > > attr = nla[NFTA_TABLE_NAME]; > > table = nft_table_lookup(net, attr, family, genmask); > > @@ -1765,6 +1768,9 @@ static int nft_chain_parse_hook(struct net *net, > > ha[NFTA_HOOK_PRIORITY] == NULL) > > return -EINVAL; > > > > + if (family >= NFPROTO_NUMPROTO) > > + return -EAFNOSUPPORT; > > + > > hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); > > hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); > > > > @@ -1774,6 +1780,8 @@ static int nft_chain_parse_hook(struct net *net, > > family, autoload); > > if (IS_ERR(type)) > > return PTR_ERR(type); > > + } else if (!type) { > > + return -EOPNOTSUPP; > > I think this check should be enough. > > I mean, NFPROTO_NUMPROTO still allows for creating tables for families > that don't exist (<= NFPROTO_NUMPROTO) and why bother on creating such > table. As long as such table does not crash the kernel, I think it's > fine. No changes can be attached anymore anyway. > > Otherwise, if a helper function to check for the families that are > really supported could be another alternative. But not sure it is > worth? Not worth. Probably this patch instead? Just make sure that access to the chain type array is safe, no direct access to chain_type[][] anymore. This includes the check for the default type too, since it cannot be assume to always have a filter chain for unsupported families. Thanks for explaining. [-- Attachment #2: x.patch --] [-- Type: text/x-diff, Size: 1963 bytes --] diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 65f51a2e9c2a..4aa01c1253b1 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -553,14 +553,27 @@ static inline u64 nf_tables_alloc_handle(struct nft_table *table) static const struct nft_chain_type *chain_type[NFPROTO_NUMPROTO][NFT_CHAIN_T_MAX]; static const struct nft_chain_type * +__nft_chain_type_get(u8 family, enum nft_chain_types type) +{ + if (family >= NFPROTO_NUMPROTO || + type >= NFT_CHAIN_T_MAX) + return NULL; + + return chain_type[family][type]; +} + +static const struct nft_chain_type * __nf_tables_chain_type_lookup(const struct nlattr *nla, u8 family) { + const struct nft_chain_type *type; int i; for (i = 0; i < NFT_CHAIN_T_MAX; i++) { - if (chain_type[family][i] != NULL && - !nla_strcmp(nla, chain_type[family][i]->name)) - return chain_type[family][i]; + type = __nft_chain_type_get(family, i); + if (!type) + continue; + if (!nla_strcmp(nla, type->name)) + return type; } return NULL; } @@ -1162,11 +1175,8 @@ static void nf_tables_table_destroy(struct nft_ctx *ctx) void nft_register_chain_type(const struct nft_chain_type *ctype) { - if (WARN_ON(ctype->family >= NFPROTO_NUMPROTO)) - return; - nfnl_lock(NFNL_SUBSYS_NFTABLES); - if (WARN_ON(chain_type[ctype->family][ctype->type] != NULL)) { + if (WARN_ON(__nft_chain_type_get(ctype->family, ctype->type))) { nfnl_unlock(NFNL_SUBSYS_NFTABLES); return; } @@ -1768,7 +1778,10 @@ static int nft_chain_parse_hook(struct net *net, hook->num = ntohl(nla_get_be32(ha[NFTA_HOOK_HOOKNUM])); hook->priority = ntohl(nla_get_be32(ha[NFTA_HOOK_PRIORITY])); - type = chain_type[family][NFT_CHAIN_T_DEFAULT]; + type = __nft_chain_type_get(family, NFT_CHAIN_T_DEFAULT); + if (!type) + return -EOPNOTSUPP; + if (nla[NFTA_CHAIN_TYPE]) { type = nf_tables_chain_type_lookup(net, nla[NFTA_CHAIN_TYPE], family, autoload); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference 2020-01-21 13:26 ` Pablo Neira Ayuso @ 2020-01-21 14:35 ` Florian Westphal 0 siblings, 0 replies; 8+ messages in thread From: Florian Westphal @ 2020-01-21 14:35 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: Florian Westphal, netfilter-devel, netdev, syzkaller-bugs, syzbot+156a04714799b1d480bc Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Otherwise, if a helper function to check for the families that are > > really supported could be another alternative. But not sure it is > > worth? > > Not worth. > > Probably this patch instead? Just make sure that access to the chain > type array is safe, no direct access to chain_type[][] anymore. > > This includes the check for the default type too, since it cannot be > assume to always have a filter chain for unsupported families. > > Thanks for explaining. LGTM, this will prbably also fix the other sytbot splat wrt. nla_strcmp(). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: general protection fault in nft_chain_parse_hook 2020-01-15 20:51 general protection fault in nft_chain_parse_hook syzbot 2020-01-16 18:03 ` syzbot 2020-01-16 21:11 ` [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference Florian Westphal @ 2020-01-17 18:57 ` syzbot 2 siblings, 0 replies; 8+ messages in thread From: syzbot @ 2020-01-17 18:57 UTC (permalink / raw) To: bridge, coreteam, davem, fw, kadlec, kadlec, kuznet, linux-kernel, netdev, netfilter-devel, pablo, stephen, syzkaller-bugs, yoshfuji syzbot has bisected this bug to: commit 98319cb9089844d76e65a6cce5bfbd165e698735 Author: Pablo Neira Ayuso <pablo@netfilter.org> Date: Tue Jan 9 01:48:47 2018 +0000 netfilter: nf_tables: get rid of struct nft_af_info abstraction bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=13d38159e00000 start commit: f5ae2ea6 Fix built-in early-load Intel microcode alignment git tree: upstream final crash: https://syzkaller.appspot.com/x/report.txt?x=10338159e00000 console output: https://syzkaller.appspot.com/x/log.txt?x=17d38159e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=d9290aeb7e6cf1c4 dashboard link: https://syzkaller.appspot.com/bug?extid=156a04714799b1d480bc syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15a7e669e00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11102356e00000 Reported-by: syzbot+156a04714799b1d480bc@syzkaller.appspotmail.com Fixes: 98319cb90898 ("netfilter: nf_tables: get rid of struct nft_af_info abstraction") For information about bisection process see: https://goo.gl/tpsmEJ#bisection ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-21 14:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-15 20:51 general protection fault in nft_chain_parse_hook syzbot 2020-01-16 18:03 ` syzbot 2020-01-16 21:11 ` [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference Florian Westphal 2020-01-18 20:30 ` Pablo Neira Ayuso 2020-01-18 23:28 ` Florian Westphal 2020-01-21 13:26 ` Pablo Neira Ayuso 2020-01-21 14:35 ` Florian Westphal 2020-01-17 18:57 ` general protection fault in nft_chain_parse_hook syzbot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).