Commit a499b03b authored by Florian Westphal's avatar Florian Westphal Committed by Pablo Neira Ayuso
Browse files

netfilter: nf_tables: unlink table before deleting it 



syzbot reports following UAF:
BUG: KASAN: use-after-free in memcmp+0x18f/0x1c0 lib/string.c:955
 nla_strcmp+0xf2/0x130 lib/nlattr.c:836
 nft_table_lookup.part.0+0x1a2/0x460 net/netfilter/nf_tables_api.c:570
 nft_table_lookup net/netfilter/nf_tables_api.c:4064 [inline]
 nf_tables_getset+0x1b3/0x860 net/netfilter/nf_tables_api.c:4064
 nfnetlink_rcv_msg+0x659/0x13f0 net/netfilter/nfnetlink.c:285
 netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2504

Problem is that all get operations are lockless, so the commit_mutex
held by nft_rcv_nl_event() isn't enough to stop a parallel GET request
from doing read-accesses to the table object even after synchronize_rcu().

To avoid this, unlink the table first and store the table objects in
on-stack scratch space.

Fixes: 6001a930 ("netfilter: nftables: introduce table ownership")
Reported-and-tested-by: default avatar <syzbot+f31660cf279b0557160c@syzkaller.appspotmail.com>
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent cb89f63b

net/netfilter/nf_tables_api.c

+18 −10
Original line number Diff line number Diff line
@@ -9599,7 +9599,6 @@ static void __nft_release_table(struct net *net, struct nft_table *table) 
		table->use--;

		nf_tables_chain_destroy(&ctx);

	}

	list_del(&table->list);

	nf_tables_table_destroy(&ctx);

}
@@ -9612,6 +9611,8 @@ static void __nft_release_tables(struct net *net) 
		if (nft_table_has_owner(table))

			continue;



		list_del(&table->list);



		__nft_release_table(net, table);

	}

}
@@ -9619,31 +9620,38 @@ static void __nft_release_tables(struct net *net) 
static int nft_rcv_nl_event(struct notifier_block *this, unsigned long event,

			    void *ptr)

{

	struct nft_table *table, *to_delete[8];

	struct nftables_pernet *nft_net;

	struct netlink_notify *n = ptr;

	struct nft_table *table, *nt;

	struct net *net = n->net;

	bool release = false;

	unsigned int deleted;

	bool restart = false;



	if (event != NETLINK_URELEASE || n->protocol != NETLINK_NETFILTER)

		return NOTIFY_DONE;



	nft_net = nft_pernet(net);

	deleted = 0;

	mutex_lock(&nft_net->commit_mutex);

again:

	list_for_each_entry(table, &nft_net->tables, list) {

		if (nft_table_has_owner(table) &&

		    n->portid == table->nlpid) {

			__nft_release_hook(net, table);

			release = true;

			list_del_rcu(&table->list);

			to_delete[deleted++] = table;

			if (deleted >= ARRAY_SIZE(to_delete))

				break;

		}

	}

	if (release) {

	if (deleted) {

		restart = deleted >= ARRAY_SIZE(to_delete);

		synchronize_rcu();

		list_for_each_entry_safe(table, nt, &nft_net->tables, list) {

			if (nft_table_has_owner(table) &&

			    n->portid == table->nlpid)

				__nft_release_table(net, table);

		}

		while (deleted)

			__nft_release_table(net, to_delete[--deleted]);



		if (restart)

			goto again;

	}

	mutex_unlock(&nft_net->commit_mutex);