Hi Greg, Sasha,
The following list shows the backported patches, I am using original commit IDs for reference:
1) 1240eb93f061 ("netfilter: nf_tables: incorrect error path handling with NFT_MSG_NEWRULE")
2) 26b5a5712eb8 ("netfilter: nf_tables: add NFT_TRANS_PREPARE_ERROR to deal with bound set/chain")
3) 3e70489721b6 ("netfilter: nf_tables: unbind non-anonymous set if rule construction fails")
Please, apply, Thanks.
Pablo Neira Ayuso (3): netfilter: nf_tables: incorrect error path handling with NFT_MSG_NEWRULE netfilter: nf_tables: add NFT_TRANS_PREPARE_ERROR to deal with bound set/chain netfilter: nf_tables: unbind non-anonymous set if rule construction fails
include/net/netfilter/nf_tables.h | 1 + net/netfilter/nf_tables_api.c | 29 +++++++++++++++++++++++++---- 2 files changed, 26 insertions(+), 4 deletions(-)
[ 1240eb93f0616b21c675416516ff3d74798fdc97 ]
In case of error when adding a new rule that refers to an anonymous set, deactivate expressions via NFT_TRANS_PREPARE state, not NFT_TRANS_RELEASE. Thus, the lookup expression marks anonymous sets as inactive in the next generation to ensure it is not reachable in this transaction anymore and decrement the set refcount as introduced by c1592a89942e ("netfilter: nf_tables: deactivate anonymous set from preparation phase"). The abort step takes care of undoing the anonymous set.
This is also consistent with rule deletion, where NFT_TRANS_PREPARE is used. Note that this error path is exercised in the preparation step of the commit protocol. This patch replaces nf_tables_rule_release() by the deactivate and destroy calls, this time with NFT_TRANS_PREPARE.
Due to this incorrect error handling, it is possible to access a dangling pointer to the anonymous set that remains in the transaction list.
[1009.379054] BUG: KASAN: use-after-free in nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379106] Read of size 8 at addr ffff88816c4c8020 by task nft-rule-add/137110 [1009.379116] CPU: 7 PID: 137110 Comm: nft-rule-add Not tainted 6.4.0-rc4+ #256 [1009.379128] Call Trace: [1009.379132] <TASK> [1009.379135] dump_stack_lvl+0x33/0x50 [1009.379146] ? nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379191] print_address_description.constprop.0+0x27/0x300 [1009.379201] kasan_report+0x107/0x120 [1009.379210] ? nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379255] nft_set_lookup_global+0x147/0x1a0 [nf_tables] [1009.379302] nft_lookup_init+0xa5/0x270 [nf_tables] [1009.379350] nf_tables_newrule+0x698/0xe50 [nf_tables] [1009.379397] ? nf_tables_rule_release+0xe0/0xe0 [nf_tables] [1009.379441] ? kasan_unpoison+0x23/0x50 [1009.379450] nfnetlink_rcv_batch+0x97c/0xd90 [nfnetlink] [1009.379470] ? nfnetlink_rcv_msg+0x480/0x480 [nfnetlink] [1009.379485] ? __alloc_skb+0xb8/0x1e0 [1009.379493] ? __alloc_skb+0xb8/0x1e0 [1009.379502] ? entry_SYSCALL_64_after_hwframe+0x46/0xb0 [1009.379509] ? unwind_get_return_address+0x2a/0x40 [1009.379517] ? write_profile+0xc0/0xc0 [1009.379524] ? avc_lookup+0x8f/0xc0 [1009.379532] ? __rcu_read_unlock+0x43/0x60
Fixes: 958bee14d071 ("netfilter: nf_tables: use new transaction infrastructure to handle sets") Signed-off-by: Pablo Neira Ayuso pablo@netfilter.org --- net/netfilter/nf_tables_api.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index e091c552b0b9..a4a04c404944 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2465,7 +2465,8 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk, return 0;
err2: - nf_tables_rule_release(&ctx, rule); + nft_rule_expr_deactivate(&ctx, rule, NFT_TRANS_PREPARE); + nf_tables_rule_destroy(&ctx, rule); err1: for (i = 0; i < n; i++) { if (info[i].ops != NULL)
[ 26b5a5712eb85e253724e56a54c17f8519bd8e4e ]
Add a new state to deal with rule expressions deactivation from the newrule error path, otherwise the anonymous set remains in the list in inactive state for the next generation. Mark the set/chain transaction as unbound so the abort path releases this object, set it as inactive in the next generation so it is not reachable anymore from this transaction and reference counter is dropped.
Fixes: 1240eb93f061 ("netfilter: nf_tables: incorrect error path handling with NFT_MSG_NEWRULE") Signed-off-by: Pablo Neira Ayuso pablo@netfilter.org --- include/net/netfilter/nf_tables.h | 1 + net/netfilter/nf_tables_api.c | 26 ++++++++++++++++++++++---- 2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index 0d625ff7841a..d5e933e6a611 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -725,6 +725,7 @@ struct nft_expr_type {
enum nft_trans_phase { NFT_TRANS_PREPARE, + NFT_TRANS_PREPARE_ERROR, NFT_TRANS_ABORT, NFT_TRANS_COMMIT, NFT_TRANS_RELEASE diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index a4a04c404944..624de5f25557 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -140,7 +140,8 @@ static void nft_trans_destroy(struct nft_trans *trans) kfree(trans); }
-static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set) +static void __nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set, + bool bind) { struct net *net = ctx->net; struct nft_trans *trans; @@ -152,16 +153,26 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set) switch (trans->msg_type) { case NFT_MSG_NEWSET: if (nft_trans_set(trans) == set) - nft_trans_set_bound(trans) = true; + nft_trans_set_bound(trans) = bind; break; case NFT_MSG_NEWSETELEM: if (nft_trans_elem_set(trans) == set) - nft_trans_elem_set_bound(trans) = true; + nft_trans_elem_set_bound(trans) = bind; break; } } }
+static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set) +{ + return __nft_set_trans_bind(ctx, set, true); +} + +static void nft_set_trans_unbind(const struct nft_ctx *ctx, struct nft_set *set) +{ + return __nft_set_trans_bind(ctx, set, false); +} + static int nf_tables_register_hooks(struct net *net, const struct nft_table *table, struct nft_chain *chain, @@ -2465,7 +2476,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk, return 0;
err2: - nft_rule_expr_deactivate(&ctx, rule, NFT_TRANS_PREPARE); + nft_rule_expr_deactivate(&ctx, rule, NFT_TRANS_PREPARE_ERROR); nf_tables_rule_destroy(&ctx, rule); err1: for (i = 0; i < n; i++) { @@ -3446,6 +3457,13 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set, enum nft_trans_phase phase) { switch (phase) { + case NFT_TRANS_PREPARE_ERROR: + nft_set_trans_unbind(ctx, set); + if (set->flags & NFT_SET_ANONYMOUS) + nft_deactivate_next(ctx->net, set); + + set->use--; + break; case NFT_TRANS_PREPARE: if (set->flags & NFT_SET_ANONYMOUS) nft_deactivate_next(ctx->net, set);
[ 3e70489721b6c870252c9082c496703677240f53 ]
Otherwise a dangling reference to a rule object that is gone remains in the set binding list.
Fixes: 26b5a5712eb8 ("netfilter: nf_tables: add NFT_TRANS_PREPARE_ERROR to deal with bound set/chain") Signed-off-by: Pablo Neira Ayuso pablo@netfilter.org --- net/netfilter/nf_tables_api.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 624de5f25557..b016ae68d9db 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3461,6 +3461,8 @@ void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set, nft_set_trans_unbind(ctx, set); if (set->flags & NFT_SET_ANONYMOUS) nft_deactivate_next(ctx->net, set); + else + list_del_rcu(&binding->list);
set->use--; break;
On Wed, Jul 05, 2023 at 06:56:20PM +0200, Pablo Neira Ayuso wrote:
Hi Greg, Sasha,
The following list shows the backported patches, I am using original commit IDs for reference:
1240eb93f061 ("netfilter: nf_tables: incorrect error path handling with NFT_MSG_NEWRULE")
26b5a5712eb8 ("netfilter: nf_tables: add NFT_TRANS_PREPARE_ERROR to deal with bound set/chain")
3e70489721b6 ("netfilter: nf_tables: unbind non-anonymous set if rule construction fails")
Please, apply,
Now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Pablo Neira Ayuso