commit: 49c65cd5536daa462058ae4d2fef3d167b10719c Author: Mike Pagano <mpagano <AT> gentoo <DOT> org> AuthorDate: Wed Mar 27 12:19:55 2019 +0000 Commit: Mike Pagano <mpagano <AT> gentoo <DOT> org> CommitDate: Wed Mar 27 12:19:55 2019 +0000 URL: https://gitweb.gentoo.org/proj/linux-patches.git/commit/?id=49c65cd5
Update of netfilter patch thanks to kerfamil Updated patch: netfilter: nf_tables: fix set double-free in abort path Signed-off-by: Mike Pagano <mpagano <AT> gentoo.org> ..._tables-fix-set-double-free-in-abort-path.patch | 189 +++++++++++---------- 1 file changed, 103 insertions(+), 86 deletions(-) diff --git a/2900_netfilter-patch-nf_tables-fix-set-double-free-in-abort-path.patch b/2900_netfilter-patch-nf_tables-fix-set-double-free-in-abort-path.patch index 8a126bf..3cc4aef 100644 --- a/2900_netfilter-patch-nf_tables-fix-set-double-free-in-abort-path.patch +++ b/2900_netfilter-patch-nf_tables-fix-set-double-free-in-abort-path.patch @@ -1,110 +1,127 @@ -From: Florian Westphal <[email protected]> -To: <[email protected]> -Cc: [email protected], Florian Westphal <[email protected]> -Subject: [PATCH nf] netfilter: nf_tables: fix set double-free in abort path -Date: Thu, 7 Mar 2019 20:30:41 +0100 -X-Mailer: git-send-email 2.19.2 - -The abort path can cause a double-free of an (anon) set. +commit 40ba1d9b4d19796afc9b7ece872f5f3e8f5e2c13 upstream. +The abort path can cause a double-free of an anonymous set. Added-and-to-be-aborted rule looks like this: udp dport { 137, 138 } drop The to-be-aborted transaction list looks like this: + newset newsetelem newsetelem rule -This gets walked in reverse order, so first pass disables -the rule, the set elements, then the set. - -After synchronize_rcu(), we then destroy those in same order: -rule, set element, set element, newset. +This gets walked in reverse order, so first pass disables the rule, the +set elements, then the set. -Problem is that the (anon) set has already been bound to the rule, -so the rule (lookup expression destructor) already frees the set, -when then cause use-after-free when trying to delete the elements -from this set, then try to free the set again when handling the -newset expression. +After synchronize_rcu(), we then destroy those in same order: rule, set +element, set element, newset. -To resolve this, check in first phase if the newset is bound already. -If so, remove the newset transaction from the list, rule destructor -will handle cleanup. +Problem is that the anonymous set has already been bound to the rule, so +the rule (lookup expression destructor) already frees the set, when then +cause use-after-free when trying to delete the elements from this set, +then try to free the set again when handling the newset expression. -This is still causes the use-after-free on set element removal. -To handle this, move all affected set elements to a extra list -and process it first. +Rule releases the bound set in first place from the abort path, this +causes the use-after-free on set element removal when undoing the new +element transactions. To handle this, skip new element transaction if +set is bound from the abort path. -This forces strict 'destroy elements, then set' ordering. +This is still causes the use-after-free on set element removal. To +handle this, remove transaction from the list when the set is already +bound. -Fixes: f6ac8585897684 ("netfilter: nf_tables: unbind set in rule from commit path") +Fixes: f6ac85858976 ("netfilter: nf_tables: unbind set in rule from commit path") Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1325 -Signed-off-by: Florian Westphal <[email protected]> +Signed-off-by: Pablo Neira Ayuso <[email protected]> +--- +Florian, I'm taking your original patch subject and part of the description, +sending this as v2. Please ack if this looks good to you. Thanks. ---- a/net/netfilter/nf_tables_api.c 2019-03-07 21:49:45.776492810 -0000 -+++ b/net/netfilter/nf_tables_api.c 2019-03-07 21:49:57.067493081 -0000 -@@ -6634,10 +6634,39 @@ static void nf_tables_abort_release(stru - kfree(trans); - } + include/net/netfilter/nf_tables.h | 6 ++---- + net/netfilter/nf_tables_api.c | 17 +++++++++++------ + 2 files changed, 13 insertions(+), 10 deletions(-) + +diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h +index b4984bbbe157..3d58acf94dd2 100644 +--- a/include/net/netfilter/nf_tables.h ++++ b/include/net/netfilter/nf_tables.h +@@ -416,7 +416,8 @@ struct nft_set { + unsigned char *udata; + /* runtime data below here */ + const struct nft_set_ops *ops ____cacheline_aligned; +- u16 flags:14, ++ u16 flags:13, ++ bound:1, + genmask:2; + u8 klen; + u8 dlen; +@@ -1329,15 +1330,12 @@ struct nft_trans_rule { + struct nft_trans_set { + struct nft_set *set; + u32 set_id; +- bool bound; + }; -+static void __nf_tables_newset_abort(struct net *net, -+ struct nft_trans *set_trans, -+ struct list_head *set_elements) -+{ -+ const struct nft_set *set = nft_trans_set(set_trans); -+ struct nft_trans *trans, *next; -+ -+ if (!nft_trans_set_bound(set_trans)) -+ return; -+ -+ /* When abort is in progress, NFT_MSG_NEWRULE will remove the -+ * set if its bound, so we need to remove the NEWSET transaction, -+ * else the set is released twice. NEWSETELEM need to be moved -+ * to special list to ensure 'free elements, then set' ordering. -+ */ -+ list_for_each_entry_safe_reverse(trans, next, -+ &net->nft.commit_list, list) { -+ if (trans == set_trans) -+ break; -+ -+ if (trans->msg_type == NFT_MSG_NEWSETELEM && -+ nft_trans_set(trans) == set) -+ list_move(&trans->list, set_elements); -+ } -+ -+ nft_trans_destroy(set_trans); -+} -+ - static int __nf_tables_abort(struct net *net) - { - struct nft_trans *trans, *next; - struct nft_trans_elem *te; -+ LIST_HEAD(set_elements); + #define nft_trans_set(trans) \ + (((struct nft_trans_set *)trans->data)->set) + #define nft_trans_set_id(trans) \ + (((struct nft_trans_set *)trans->data)->set_id) +-#define nft_trans_set_bound(trans) \ +- (((struct nft_trans_set *)trans->data)->bound) - list_for_each_entry_safe_reverse(trans, next, &net->nft.commit_list, - list) { -@@ -6693,6 +6722,8 @@ static int __nf_tables_abort(struct net + struct nft_trans_chain { + bool update; +diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c +index 4893f248dfdc..e1724f9d8b9d 100644 +--- a/net/netfilter/nf_tables_api.c ++++ b/net/netfilter/nf_tables_api.c +@@ -127,7 +127,7 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set) + list_for_each_entry_reverse(trans, &net->nft.commit_list, list) { + if (trans->msg_type == NFT_MSG_NEWSET && + nft_trans_set(trans) == set) { +- nft_trans_set_bound(trans) = true; ++ set->bound = true; + break; + } + } +@@ -6617,8 +6617,7 @@ static void nf_tables_abort_release(struct nft_trans *trans) + nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans)); + break; + case NFT_MSG_NEWSET: +- if (!nft_trans_set_bound(trans)) +- nft_set_destroy(nft_trans_set(trans)); ++ nft_set_destroy(nft_trans_set(trans)); + break; + case NFT_MSG_NEWSETELEM: + nft_set_elem_destroy(nft_trans_elem_set(trans), +@@ -6691,8 +6690,11 @@ static int __nf_tables_abort(struct net *net) + break; + case NFT_MSG_NEWSET: trans->ctx.table->use--; - if (!nft_trans_set_bound(trans)) - list_del_rcu(&nft_trans_set(trans)->list); -+ -+ __nf_tables_newset_abort(net, trans, &set_elements); +- if (!nft_trans_set_bound(trans)) +- list_del_rcu(&nft_trans_set(trans)->list); ++ if (nft_trans_set(trans)->bound) { ++ nft_trans_destroy(trans); ++ break; ++ } ++ list_del_rcu(&nft_trans_set(trans)->list); break; case NFT_MSG_DELSET: trans->ctx.table->use++; -@@ -6739,6 +6770,13 @@ static int __nf_tables_abort(struct net - - synchronize_rcu(); - -+ /* free set elements before the set they belong to is freed */ -+ list_for_each_entry_safe_reverse(trans, next, -+ &set_elements, list) { -+ list_del(&trans->list); -+ nf_tables_abort_release(trans); -+ } -+ - list_for_each_entry_safe_reverse(trans, next, - &net->nft.commit_list, list) { - list_del(&trans->list); +@@ -6700,8 +6702,11 @@ static int __nf_tables_abort(struct net *net) + nft_trans_destroy(trans); + break; + case NFT_MSG_NEWSETELEM: ++ if (nft_trans_elem_set(trans)->bound) { ++ nft_trans_destroy(trans); ++ break; ++ } + te = (struct nft_trans_elem *)trans->data; +- + te->set->ops->remove(net, te->set, &te->elem); + atomic_dec(&te->set->nelems); + break; +-- +2.11.0
