general protection fault in nft_chain_parse_hook

general protection fault in nft_chain_parse_hook

[syzbot]

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.

Re: general protection fault in nft_chain_parse_hook

[syzbot]

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

[PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference

[Florian Westphal]

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

Re: general protection fault in nft_chain_parse_hook

[syzbot]

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

Re: [Bridge] general protection fault in nft_chain_parse_hook

[syzbot]

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

Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference

[Pablo Neira Ayuso]

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.

Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference

[Florian Westphal]

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.

Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference

[Pablo Neira Ayuso]

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

Re: [PATCH nf] netfilter: nf_tables: check for valid chain type pointer before dereference

[Florian Westphal]

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