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:

quoted

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.