* [PATCH -stable,4.14 1/6] netfilter: nf_tables: split set destruction in deactivate and destroy phase
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 2/6] netfilter: nf_tables: unbind set in rule from commit path Pablo Neira Ayuso
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
From: Florian Westphal <fw@strlen.de>
[ backport for 4.14 of cd5125d8f51882279f50506bb9c7e5e89dc9bef3 ]
Splits unbind_set into destroy_set and unbinding operation.
Unbinding removes set from lists (so new transaction would not
find it anymore) but keeps memory allocated (so packet path continues
to work).
Rebind function is added to allow unrolling in case transaction
that wants to remove set is aborted.
Destroy function is added to free the memory, but this could occur
outside of transaction in the future.
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 7 +++++-
net/netfilter/nf_tables_api.c | 36 +++++++++++++++++++++----------
net/netfilter/nft_dynset.c | 21 +++++++++++++++++-
net/netfilter/nft_lookup.c | 20 ++++++++++++++++-
net/netfilter/nft_objref.c | 20 ++++++++++++++++-
5 files changed, 89 insertions(+), 15 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3107895115c2..59da90bb840d 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -463,6 +463,9 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding);
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding);
+void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
+ struct nft_set_binding *binding);
+void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set);
/**
* enum nft_set_extensions - set extension type IDs
@@ -716,7 +719,9 @@ struct nft_expr_type {
* @eval: Expression evaluation function
* @size: full expression size, including private data size
* @init: initialization function
- * @destroy: destruction function
+ * @activate: activate expression in the next generation
+ * @deactivate: deactivate expression in next generation
+ * @destroy: destruction function, called after synchronize_rcu
* @dump: function to dump parameters
* @type: expression type
* @validate: validate expression, called during loop detection
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b24c83cf64b9..39416c568d18 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -327,7 +327,7 @@ static int nft_delrule_by_chain(struct nft_ctx *ctx)
return 0;
}
-static int nft_trans_set_add(struct nft_ctx *ctx, int msg_type,
+static int nft_trans_set_add(const struct nft_ctx *ctx, int msg_type,
struct nft_set *set)
{
struct nft_trans *trans;
@@ -347,7 +347,7 @@ static int nft_trans_set_add(struct nft_ctx *ctx, int msg_type,
return 0;
}
-static int nft_delset(struct nft_ctx *ctx, struct nft_set *set)
+static int nft_delset(const struct nft_ctx *ctx, struct nft_set *set)
{
int err;
@@ -3311,13 +3311,6 @@ static void nft_set_destroy(struct nft_set *set)
kvfree(set);
}
-static void nf_tables_set_destroy(const struct nft_ctx *ctx, struct nft_set *set)
-{
- list_del_rcu(&set->list);
- nf_tables_set_notify(ctx, set, NFT_MSG_DELSET, GFP_ATOMIC);
- nft_set_destroy(set);
-}
-
static int nf_tables_delset(struct net *net, struct sock *nlsk,
struct sk_buff *skb, const struct nlmsghdr *nlh,
const struct nlattr * const nla[],
@@ -3400,17 +3393,38 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
}
EXPORT_SYMBOL_GPL(nf_tables_bind_set);
-void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
+void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding)
+{
+ if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
+ nft_is_active(ctx->net, set))
+ list_add_tail_rcu(&set->list, &ctx->table->sets);
+
+ list_add_tail_rcu(&binding->list, &set->bindings);
+}
+EXPORT_SYMBOL_GPL(nf_tables_rebind_set);
+
+void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
+ struct nft_set_binding *binding)
{
list_del_rcu(&binding->list);
if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
nft_is_active(ctx->net, set))
- nf_tables_set_destroy(ctx, set);
+ list_del_rcu(&set->list);
}
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
+void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
+{
+ if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
+ nft_is_active(ctx->net, set)) {
+ nf_tables_set_notify(ctx, set, NFT_MSG_DELSET, GFP_ATOMIC);
+ nft_set_destroy(set);
+ }
+}
+EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
+
const struct nft_set_ext_type nft_set_ext_types[] = {
[NFT_SET_EXT_KEY] = {
.align = __alignof__(u32),
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index f8688f9bf46c..800ca627a457 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -223,14 +223,31 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
return err;
}
+static void nft_dynset_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_dynset *priv = nft_expr_priv(expr);
+
+ nf_tables_rebind_set(ctx, priv->set, &priv->binding);
+}
+
+static void nft_dynset_deactivate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_dynset *priv = nft_expr_priv(expr);
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+}
+
static void nft_dynset_destroy(const struct nft_ctx *ctx,
const struct nft_expr *expr)
{
struct nft_dynset *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
if (priv->expr != NULL)
nft_expr_destroy(ctx, priv->expr);
+
+ nf_tables_destroy_set(ctx, priv->set);
}
static int nft_dynset_dump(struct sk_buff *skb, const struct nft_expr *expr)
@@ -267,6 +284,8 @@ static const struct nft_expr_ops nft_dynset_ops = {
.eval = nft_dynset_eval,
.init = nft_dynset_init,
.destroy = nft_dynset_destroy,
+ .activate = nft_dynset_activate,
+ .deactivate = nft_dynset_deactivate,
.dump = nft_dynset_dump,
};
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 44015a151ad6..c34667f4f48a 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -118,12 +118,28 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
return 0;
}
+static void nft_lookup_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_lookup *priv = nft_expr_priv(expr);
+
+ nf_tables_rebind_set(ctx, priv->set, &priv->binding);
+}
+
+static void nft_lookup_deactivate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_lookup *priv = nft_expr_priv(expr);
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+}
+
static void nft_lookup_destroy(const struct nft_ctx *ctx,
const struct nft_expr *expr)
{
struct nft_lookup *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ nf_tables_destroy_set(ctx, priv->set);
}
static int nft_lookup_dump(struct sk_buff *skb, const struct nft_expr *expr)
@@ -151,6 +167,8 @@ static const struct nft_expr_ops nft_lookup_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)),
.eval = nft_lookup_eval,
.init = nft_lookup_init,
+ .activate = nft_lookup_activate,
+ .deactivate = nft_lookup_deactivate,
.destroy = nft_lookup_destroy,
.dump = nft_lookup_dump,
};
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index 7bcdc48f3d73..d289fa5e4eb9 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -154,12 +154,28 @@ static int nft_objref_map_dump(struct sk_buff *skb, const struct nft_expr *expr)
return -1;
}
+static void nft_objref_map_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_objref_map *priv = nft_expr_priv(expr);
+
+ nf_tables_rebind_set(ctx, priv->set, &priv->binding);
+}
+
+static void nft_objref_map_deactivate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_objref_map *priv = nft_expr_priv(expr);
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+}
+
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
const struct nft_expr *expr)
{
struct nft_objref_map *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ nf_tables_destroy_set(ctx, priv->set);
}
static struct nft_expr_type nft_objref_type;
@@ -168,6 +184,8 @@ static const struct nft_expr_ops nft_objref_map_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_objref_map)),
.eval = nft_objref_map_eval,
.init = nft_objref_map_init,
+ .activate = nft_objref_map_activate,
+ .deactivate = nft_objref_map_deactivate,
.destroy = nft_objref_map_destroy,
.dump = nft_objref_map_dump,
};
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH -stable,4.14 2/6] netfilter: nf_tables: unbind set in rule from commit path
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 1/6] netfilter: nf_tables: split set destruction in deactivate and destroy phase Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 3/6] netfilter: nft_hash: fix nft_hash_deactivate Pablo Neira Ayuso
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
[ backport for 4.14 of f6ac8585897684374a19863fff21186a05805286 ]
Anonymous sets that are bound to rules from the same transaction trigger
a kernel splat from the abort path due to double set list removal and
double free.
This patch updates the logic to search for the transaction that is
responsible for creating the set and disable the set list removal and
release, given the rule is now responsible for this. Lookup is reverse
since the transaction that adds the set is likely to be at the tail of
the list.
Moreover, this patch adds the unbind step to deliver the event from the
commit path. This should not be done from the worker thread, since we
have no guarantees of in-order delivery to the listener.
This patch removes the assumption that both activate and deactivate
callbacks need to be provided.
Fixes: cd5125d8f518 ("netfilter: nf_tables: split set destruction in deactivate and destroy phase")
Reported-by: Mikhail Morfikov <mmorfikov@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 17 +++++--
net/netfilter/nf_tables_api.c | 85 +++++++++++++++----------------
net/netfilter/nft_dynset.c | 18 +++----
net/netfilter/nft_immediate.c | 6 ++-
net/netfilter/nft_lookup.c | 18 +++----
net/netfilter/nft_objref.c | 18 +++----
6 files changed, 80 insertions(+), 82 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 59da90bb840d..cc6ba7e593e7 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -462,9 +462,7 @@ struct nft_set_binding {
int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding);
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding);
-void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding);
+ struct nft_set_binding *binding, bool commit);
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set);
/**
@@ -713,6 +711,13 @@ struct nft_expr_type {
#define NFT_EXPR_STATEFUL 0x1
+enum nft_trans_phase {
+ NFT_TRANS_PREPARE,
+ NFT_TRANS_ABORT,
+ NFT_TRANS_COMMIT,
+ NFT_TRANS_RELEASE
+};
+
/**
* struct nft_expr_ops - nf_tables expression operations
*
@@ -742,7 +747,8 @@ struct nft_expr_ops {
void (*activate)(const struct nft_ctx *ctx,
const struct nft_expr *expr);
void (*deactivate)(const struct nft_ctx *ctx,
- const struct nft_expr *expr);
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase);
void (*destroy)(const struct nft_ctx *ctx,
const struct nft_expr *expr);
int (*dump)(struct sk_buff *skb,
@@ -1290,12 +1296,15 @@ struct nft_trans_rule {
struct nft_trans_set {
struct nft_set *set;
u32 set_id;
+ bool bound;
};
#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)
struct nft_trans_chain {
bool update;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 39416c568d18..5541ba7cc4a0 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -140,6 +140,23 @@ 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)
+{
+ struct net *net = ctx->net;
+ struct nft_trans *trans;
+
+ if (!(set->flags & NFT_SET_ANONYMOUS))
+ return;
+
+ 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;
+ break;
+ }
+ }
+}
+
static int nf_tables_register_hooks(struct net *net,
const struct nft_table *table,
struct nft_chain *chain,
@@ -221,18 +238,6 @@ static int nft_delchain(struct nft_ctx *ctx)
return err;
}
-/* either expr ops provide both activate/deactivate, or neither */
-static bool nft_expr_check_ops(const struct nft_expr_ops *ops)
-{
- if (!ops)
- return true;
-
- if (WARN_ON_ONCE((!ops->activate ^ !ops->deactivate)))
- return false;
-
- return true;
-}
-
static void nft_rule_expr_activate(const struct nft_ctx *ctx,
struct nft_rule *rule)
{
@@ -248,14 +253,15 @@ static void nft_rule_expr_activate(const struct nft_ctx *ctx,
}
static void nft_rule_expr_deactivate(const struct nft_ctx *ctx,
- struct nft_rule *rule)
+ struct nft_rule *rule,
+ enum nft_trans_phase phase)
{
struct nft_expr *expr;
expr = nft_expr_first(rule);
while (expr != nft_expr_last(rule) && expr->ops) {
if (expr->ops->deactivate)
- expr->ops->deactivate(ctx, expr);
+ expr->ops->deactivate(ctx, expr, phase);
expr = nft_expr_next(expr);
}
@@ -306,7 +312,7 @@ static int nft_delrule(struct nft_ctx *ctx, struct nft_rule *rule)
nft_trans_destroy(trans);
return err;
}
- nft_rule_expr_deactivate(ctx, rule);
+ nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_PREPARE);
return 0;
}
@@ -1737,9 +1743,6 @@ static int nf_tables_delchain(struct net *net, struct sock *nlsk,
*/
int nft_register_expr(struct nft_expr_type *type)
{
- if (!nft_expr_check_ops(type->ops))
- return -EINVAL;
-
nfnl_lock(NFNL_SUBSYS_NFTABLES);
if (type->family == NFPROTO_UNSPEC)
list_add_tail_rcu(&type->list, &nf_tables_expressions);
@@ -1889,10 +1892,6 @@ static int nf_tables_expr_parse(const struct nft_ctx *ctx,
err = PTR_ERR(ops);
goto err1;
}
- if (!nft_expr_check_ops(ops)) {
- err = -EINVAL;
- goto err1;
- }
} else
ops = type->ops;
@@ -2297,7 +2296,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
static void nf_tables_rule_release(const struct nft_ctx *ctx,
struct nft_rule *rule)
{
- nft_rule_expr_deactivate(ctx, rule);
+ nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
nf_tables_rule_destroy(ctx, rule);
}
@@ -3389,39 +3388,30 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
bind:
binding->chain = ctx->chain;
list_add_tail_rcu(&binding->list, &set->bindings);
+ nft_set_trans_bind(ctx, set);
+
return 0;
}
EXPORT_SYMBOL_GPL(nf_tables_bind_set);
-void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding)
-{
- if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
- nft_is_active(ctx->net, set))
- list_add_tail_rcu(&set->list, &ctx->table->sets);
-
- list_add_tail_rcu(&binding->list, &set->bindings);
-}
-EXPORT_SYMBOL_GPL(nf_tables_rebind_set);
-
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding)
+ struct nft_set_binding *binding, bool event)
{
list_del_rcu(&binding->list);
- if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
- nft_is_active(ctx->net, set))
+ if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS) {
list_del_rcu(&set->list);
+ if (event)
+ nf_tables_set_notify(ctx, set, NFT_MSG_DELSET,
+ GFP_KERNEL);
+ }
}
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
{
- if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS &&
- nft_is_active(ctx->net, set)) {
- nf_tables_set_notify(ctx, set, NFT_MSG_DELSET, GFP_ATOMIC);
+ if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
nft_set_destroy(set);
- }
}
EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
@@ -5197,6 +5187,9 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
nf_tables_rule_notify(&trans->ctx,
nft_trans_rule(trans),
NFT_MSG_DELRULE);
+ nft_rule_expr_deactivate(&trans->ctx,
+ nft_trans_rule(trans),
+ NFT_TRANS_COMMIT);
break;
case NFT_MSG_NEWSET:
nft_clear(net, nft_trans_set(trans));
@@ -5274,7 +5267,8 @@ 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:
- nft_set_destroy(nft_trans_set(trans));
+ if (!nft_trans_set_bound(trans))
+ nft_set_destroy(nft_trans_set(trans));
break;
case NFT_MSG_NEWSETELEM:
nft_set_elem_destroy(nft_trans_elem_set(trans),
@@ -5334,7 +5328,9 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
case NFT_MSG_NEWRULE:
trans->ctx.chain->use--;
list_del_rcu(&nft_trans_rule(trans)->list);
- nft_rule_expr_deactivate(&trans->ctx, nft_trans_rule(trans));
+ nft_rule_expr_deactivate(&trans->ctx,
+ nft_trans_rule(trans),
+ NFT_TRANS_ABORT);
break;
case NFT_MSG_DELRULE:
trans->ctx.chain->use++;
@@ -5344,7 +5340,8 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
break;
case NFT_MSG_NEWSET:
trans->ctx.table->use--;
- list_del_rcu(&nft_trans_set(trans)->list);
+ if (!nft_trans_set_bound(trans))
+ list_del_rcu(&nft_trans_set(trans)->list);
break;
case NFT_MSG_DELSET:
trans->ctx.table->use++;
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 800ca627a457..7e0a20343740 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -223,20 +223,17 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
return err;
}
-static void nft_dynset_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_dynset *priv = nft_expr_priv(expr);
-
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
static void nft_dynset_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase)
{
struct nft_dynset *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ if (phase == NFT_TRANS_PREPARE)
+ return;
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding,
+ phase == NFT_TRANS_COMMIT);
}
static void nft_dynset_destroy(const struct nft_ctx *ctx,
@@ -284,7 +281,6 @@ static const struct nft_expr_ops nft_dynset_ops = {
.eval = nft_dynset_eval,
.init = nft_dynset_init,
.destroy = nft_dynset_destroy,
- .activate = nft_dynset_activate,
.deactivate = nft_dynset_deactivate,
.dump = nft_dynset_dump,
};
diff --git a/net/netfilter/nft_immediate.c b/net/netfilter/nft_immediate.c
index aa87ff8beae8..86fd35018b4a 100644
--- a/net/netfilter/nft_immediate.c
+++ b/net/netfilter/nft_immediate.c
@@ -78,10 +78,14 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
}
static void nft_immediate_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase)
{
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
+ if (phase == NFT_TRANS_COMMIT)
+ return;
+
return nft_data_release(&priv->data, nft_dreg_to_type(priv->dreg));
}
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index c34667f4f48a..7dd35245222c 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -118,20 +118,17 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
return 0;
}
-static void nft_lookup_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_lookup *priv = nft_expr_priv(expr);
-
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
static void nft_lookup_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase)
{
struct nft_lookup *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ if (phase == NFT_TRANS_PREPARE)
+ return;
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding,
+ phase == NFT_TRANS_COMMIT);
}
static void nft_lookup_destroy(const struct nft_ctx *ctx,
@@ -167,7 +164,6 @@ static const struct nft_expr_ops nft_lookup_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)),
.eval = nft_lookup_eval,
.init = nft_lookup_init,
- .activate = nft_lookup_activate,
.deactivate = nft_lookup_deactivate,
.destroy = nft_lookup_destroy,
.dump = nft_lookup_dump,
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index d289fa5e4eb9..f72aeff93efa 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -154,20 +154,17 @@ static int nft_objref_map_dump(struct sk_buff *skb, const struct nft_expr *expr)
return -1;
}
-static void nft_objref_map_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_objref_map *priv = nft_expr_priv(expr);
-
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
static void nft_objref_map_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+ const struct nft_expr *expr,
+ enum nft_trans_phase phase)
{
struct nft_objref_map *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ if (phase == NFT_TRANS_PREPARE)
+ return;
+
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding,
+ phase == NFT_TRANS_COMMIT);
}
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
@@ -184,7 +181,6 @@ static const struct nft_expr_ops nft_objref_map_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_objref_map)),
.eval = nft_objref_map_eval,
.init = nft_objref_map_init,
- .activate = nft_objref_map_activate,
.deactivate = nft_objref_map_deactivate,
.destroy = nft_objref_map_destroy,
.dump = nft_objref_map_dump,
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH -stable,4.14 3/6] netfilter: nft_hash: fix nft_hash_deactivate
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 1/6] netfilter: nf_tables: split set destruction in deactivate and destroy phase Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 2/6] netfilter: nf_tables: unbind set in rule from commit path Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 4/6] netfilter: nf_tables: use-after-free in failing rule with bound set Pablo Neira Ayuso
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
[ backport for 4.14 of 7f4dae2d7f03d2aaf3b7d8343d4509c8d9d7ca9b ]
Jindřich Makovička says:
The logical OR looks fishy to me. Shouldn't be && there instead?
Link: https://bugzilla.netfilter.org/show_bug.cgi?id=1199
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nft_set_hash.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/nft_set_hash.c b/net/netfilter/nft_set_hash.c
index a684234bd229..eb7db31dd173 100644
--- a/net/netfilter/nft_set_hash.c
+++ b/net/netfilter/nft_set_hash.c
@@ -520,7 +520,7 @@ static void *nft_hash_deactivate(const struct net *net,
hash = nft_jhash(set, priv, &this->ext);
hlist_for_each_entry(he, &priv->table[hash], node) {
if (!memcmp(nft_set_ext_key(&this->ext), &elem->key.val,
- set->klen) ||
+ set->klen) &&
nft_set_elem_active(&he->ext, genmask)) {
nft_set_elem_change_active(net, set, &he->ext);
return he;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH -stable,4.14 4/6] netfilter: nf_tables: use-after-free in failing rule with bound set
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
` (2 preceding siblings ...)
2023-05-11 15:41 ` [PATCH -stable,4.14 3/6] netfilter: nft_hash: fix nft_hash_deactivate Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 5/6] netfilter: nf_tables: bogus EBUSY when deleting set after flush Pablo Neira Ayuso
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
[ backport for 4.14 of 6a0a8d10a3661a036b55af695542a714c429ab7c ]
If a rule that has already a bound anonymous set fails to be added, the
preparation phase releases the rule and the bound set. However, the
transaction object from the abort path still has a reference to the set
object that is stale, leading to a use-after-free when checking for the
set->bound field. Add a new field to the transaction that specifies if
the set is bound, so the abort path can skip releasing it since the rule
command owns it and it takes care of releasing it. After this update,
the set->bound field is removed.
[ 24.649883] Unable to handle kernel paging request at virtual address 0000000000040434
[ 24.657858] Mem abort info:
[ 24.660686] ESR = 0x96000004
[ 24.663769] Exception class = DABT (current EL), IL = 32 bits
[ 24.669725] SET = 0, FnV = 0
[ 24.672804] EA = 0, S1PTW = 0
[ 24.675975] Data abort info:
[ 24.678880] ISV = 0, ISS = 0x00000004
[ 24.682743] CM = 0, WnR = 0
[ 24.685723] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000428952000
[ 24.692207] [0000000000040434] pgd=0000000000000000
[ 24.697119] Internal error: Oops: 96000004 [#1] SMP
[...]
[ 24.889414] Call trace:
[ 24.891870] __nf_tables_abort+0x3f0/0x7a0
[ 24.895984] nf_tables_abort+0x20/0x40
[ 24.899750] nfnetlink_rcv_batch+0x17c/0x588
[ 24.904037] nfnetlink_rcv+0x13c/0x190
[ 24.907803] netlink_unicast+0x18c/0x208
[ 24.911742] netlink_sendmsg+0x1b0/0x350
[ 24.915682] sock_sendmsg+0x4c/0x68
[ 24.919185] ___sys_sendmsg+0x288/0x2c8
[ 24.923037] __sys_sendmsg+0x7c/0xd0
[ 24.926628] __arm64_sys_sendmsg+0x2c/0x38
[ 24.930744] el0_svc_common.constprop.0+0x94/0x158
[ 24.935556] el0_svc_handler+0x34/0x90
[ 24.939322] el0_svc+0x8/0xc
[ 24.942216] Code: 37280300 f9404023 91014262 aa1703e0 (f9401863)
[ 24.948336] ---[ end trace cebbb9dcbed3b56f ]---
Fixes: f6ac85858976 ("netfilter: nf_tables: unbind set in rule from commit path")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 3 +++
net/netfilter/nf_tables_api.c | 22 +++++++++++++++++-----
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index cc6ba7e593e7..ca82f32d10cd 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1335,12 +1335,15 @@ struct nft_trans_table {
struct nft_trans_elem {
struct nft_set *set;
struct nft_set_elem elem;
+ bool bound;
};
#define nft_trans_elem_set(trans) \
(((struct nft_trans_elem *)trans->data)->set)
#define nft_trans_elem(trans) \
(((struct nft_trans_elem *)trans->data)->elem)
+#define nft_trans_elem_set_bound(trans) \
+ (((struct nft_trans_elem *)trans->data)->bound)
struct nft_trans_obj {
struct nft_object *obj;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 5541ba7cc4a0..e9e3e7680a14 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -149,9 +149,14 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
return;
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;
+ switch (trans->msg_type) {
+ case NFT_MSG_NEWSET:
+ if (nft_trans_set(trans) == set)
+ nft_trans_set_bound(trans) = true;
+ break;
+ case NFT_MSG_NEWSETELEM:
+ if (nft_trans_elem_set(trans) == set)
+ nft_trans_elem_set_bound(trans) = true;
break;
}
}
@@ -5340,8 +5345,11 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
break;
case NFT_MSG_NEWSET:
trans->ctx.table->use--;
- if (!nft_trans_set_bound(trans))
- list_del_rcu(&nft_trans_set(trans)->list);
+ if (nft_trans_set_bound(trans)) {
+ nft_trans_destroy(trans);
+ break;
+ }
+ list_del_rcu(&nft_trans_set(trans)->list);
break;
case NFT_MSG_DELSET:
trans->ctx.table->use++;
@@ -5349,6 +5357,10 @@ static int nf_tables_abort(struct net *net, struct sk_buff *skb)
nft_trans_destroy(trans);
break;
case NFT_MSG_NEWSETELEM:
+ if (nft_trans_elem_set_bound(trans)) {
+ nft_trans_destroy(trans);
+ break;
+ }
te = (struct nft_trans_elem *)trans->data;
te->set->ops->remove(net, te->set, &te->elem);
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH -stable,4.14 5/6] netfilter: nf_tables: bogus EBUSY when deleting set after flush
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
` (3 preceding siblings ...)
2023-05-11 15:41 ` [PATCH -stable,4.14 4/6] netfilter: nf_tables: use-after-free in failing rule with bound set Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 15:41 ` [PATCH -stable,4.14 6/6] netfilter: nf_tables: deactivate anonymous set from preparation phase Pablo Neira Ayuso
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
[ backport for 4.14 of 273fe3f1006ea5ebc63d6729e43e8e45e32b256a ]
Set deletion after flush coming in the same batch results in EBUSY. Add
set use counter to track the number of references to this set from
rules. We cannot rely on the list of bindings for this since such list
is still populated from the preparation phase.
Reported-by: Václav Zindulka <vaclav.zindulka@tlapnet.cz>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 6 ++++++
net/netfilter/nf_tables_api.c | 28 +++++++++++++++++++++++++++-
net/netfilter/nft_dynset.c | 13 +++++++++----
net/netfilter/nft_lookup.c | 13 +++++++++----
net/netfilter/nft_objref.c | 13 +++++++++----
5 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index ca82f32d10cd..fe56b2f825b4 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -383,6 +383,7 @@ void nft_unregister_set(struct nft_set_type *type);
* @dtype: data type (verdict or numeric type defined by userspace)
* @objtype: object type (see NFT_OBJECT_* definitions)
* @size: maximum set size
+ * @use: number of rules references to this set
* @nelems: number of elements
* @ndeact: number of deactivated elements queued for removal
* @timeout: default timeout value in jiffies
@@ -405,6 +406,7 @@ struct nft_set {
u32 dtype;
u32 objtype;
u32 size;
+ u32 use;
atomic_t nelems;
u32 ndeact;
u64 timeout;
@@ -459,6 +461,10 @@ struct nft_set_binding {
u32 flags;
};
+enum nft_trans_phase;
+void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
+ struct nft_set_binding *binding,
+ enum nft_trans_phase phase);
int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding);
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e9e3e7680a14..2f5b5d563e4d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3309,6 +3309,9 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
static void nft_set_destroy(struct nft_set *set)
{
+ if (WARN_ON(set->use > 0))
+ return;
+
set->ops->destroy(set);
module_put(set->ops->type->owner);
kfree(set->name);
@@ -3339,7 +3342,7 @@ static int nf_tables_delset(struct net *net, struct sock *nlsk,
if (IS_ERR(set))
return PTR_ERR(set);
- if (!list_empty(&set->bindings) ||
+ if (set->use ||
(nlh->nlmsg_flags & NLM_F_NONREC && atomic_read(&set->nelems) > 0))
return -EBUSY;
@@ -3367,6 +3370,9 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *i;
struct nft_set_iter iter;
+ if (set->use == UINT_MAX)
+ return -EOVERFLOW;
+
if (!list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
return -EBUSY;
@@ -3394,6 +3400,7 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
binding->chain = ctx->chain;
list_add_tail_rcu(&binding->list, &set->bindings);
nft_set_trans_bind(ctx, set);
+ set->use++;
return 0;
}
@@ -3413,6 +3420,25 @@ void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
}
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
+void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
+ struct nft_set_binding *binding,
+ enum nft_trans_phase phase)
+{
+ switch (phase) {
+ case NFT_TRANS_PREPARE:
+ set->use--;
+ return;
+ case NFT_TRANS_ABORT:
+ case NFT_TRANS_RELEASE:
+ set->use--;
+ /* fall through */
+ default:
+ nf_tables_unbind_set(ctx, set, binding,
+ phase == NFT_TRANS_COMMIT);
+ }
+}
+EXPORT_SYMBOL_GPL(nf_tables_deactivate_set);
+
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
{
if (list_empty(&set->bindings) && set->flags & NFT_SET_ANONYMOUS)
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 7e0a20343740..a20f1668328d 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -229,11 +229,15 @@ static void nft_dynset_deactivate(const struct nft_ctx *ctx,
{
struct nft_dynset *priv = nft_expr_priv(expr);
- if (phase == NFT_TRANS_PREPARE)
- return;
+ nf_tables_deactivate_set(ctx, priv->set, &priv->binding, phase);
+}
+
+static void nft_dynset_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_dynset *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding,
- phase == NFT_TRANS_COMMIT);
+ priv->set->use++;
}
static void nft_dynset_destroy(const struct nft_ctx *ctx,
@@ -281,6 +285,7 @@ static const struct nft_expr_ops nft_dynset_ops = {
.eval = nft_dynset_eval,
.init = nft_dynset_init,
.destroy = nft_dynset_destroy,
+ .activate = nft_dynset_activate,
.deactivate = nft_dynset_deactivate,
.dump = nft_dynset_dump,
};
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 7dd35245222c..453f84c57166 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -124,11 +124,15 @@ static void nft_lookup_deactivate(const struct nft_ctx *ctx,
{
struct nft_lookup *priv = nft_expr_priv(expr);
- if (phase == NFT_TRANS_PREPARE)
- return;
+ nf_tables_deactivate_set(ctx, priv->set, &priv->binding, phase);
+}
+
+static void nft_lookup_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_lookup *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding,
- phase == NFT_TRANS_COMMIT);
+ priv->set->use++;
}
static void nft_lookup_destroy(const struct nft_ctx *ctx,
@@ -164,6 +168,7 @@ static const struct nft_expr_ops nft_lookup_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)),
.eval = nft_lookup_eval,
.init = nft_lookup_init,
+ .activate = nft_lookup_activate,
.deactivate = nft_lookup_deactivate,
.destroy = nft_lookup_destroy,
.dump = nft_lookup_dump,
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index f72aeff93efa..7e628f4f02b9 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -160,11 +160,15 @@ static void nft_objref_map_deactivate(const struct nft_ctx *ctx,
{
struct nft_objref_map *priv = nft_expr_priv(expr);
- if (phase == NFT_TRANS_PREPARE)
- return;
+ nf_tables_deactivate_set(ctx, priv->set, &priv->binding, phase);
+}
+
+static void nft_objref_map_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_objref_map *priv = nft_expr_priv(expr);
- nf_tables_unbind_set(ctx, priv->set, &priv->binding,
- phase == NFT_TRANS_COMMIT);
+ priv->set->use++;
}
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
@@ -181,6 +185,7 @@ static const struct nft_expr_ops nft_objref_map_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_objref_map)),
.eval = nft_objref_map_eval,
.init = nft_objref_map_init,
+ .activate = nft_objref_map_activate,
.deactivate = nft_objref_map_deactivate,
.destroy = nft_objref_map_destroy,
.dump = nft_objref_map_dump,
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH -stable,4.14 6/6] netfilter: nf_tables: deactivate anonymous set from preparation phase
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
` (4 preceding siblings ...)
2023-05-11 15:41 ` [PATCH -stable,4.14 5/6] netfilter: nf_tables: bogus EBUSY when deleting set after flush Pablo Neira Ayuso
@ 2023-05-11 15:41 ` Pablo Neira Ayuso
2023-05-11 21:53 ` [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Sasha Levin
2023-05-13 3:25 ` luwei (O)
7 siblings, 0 replies; 10+ messages in thread
From: Pablo Neira Ayuso @ 2023-05-11 15:41 UTC (permalink / raw)
To: netfilter-devel; +Cc: fw, gregkh, sashal, stable
[ backport for 4.14 of c1592a89942e9678f7d9c8030efa777c0d57edab ]
Toggle deleted anonymous sets as inactive in the next generation, so
users cannot perform any update on it. Clear the generation bitmask
in case the transaction is aborted.
The following KASAN splat shows a set element deletion for a bound
anonymous set that has been already removed in the same transaction.
[ 64.921510] ==================================================================
[ 64.923123] BUG: KASAN: wild-memory-access in nf_tables_commit+0xa24/0x1490 [nf_tables]
[ 64.924745] Write of size 8 at addr dead000000000122 by task test/890
[ 64.927903] CPU: 3 PID: 890 Comm: test Not tainted 6.3.0+ #253
[ 64.931120] Call Trace:
[ 64.932699] <TASK>
[ 64.934292] dump_stack_lvl+0x33/0x50
[ 64.935908] ? nf_tables_commit+0xa24/0x1490 [nf_tables]
[ 64.937551] kasan_report+0xda/0x120
[ 64.939186] ? nf_tables_commit+0xa24/0x1490 [nf_tables]
[ 64.940814] nf_tables_commit+0xa24/0x1490 [nf_tables]
[ 64.942452] ? __kasan_slab_alloc+0x2d/0x60
[ 64.944070] ? nf_tables_setelem_notify+0x190/0x190 [nf_tables]
[ 64.945710] ? kasan_set_track+0x21/0x30
[ 64.947323] nfnetlink_rcv_batch+0x709/0xd90 [nfnetlink]
[ 64.948898] ? nfnetlink_rcv_msg+0x480/0x480 [nfnetlink]
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 1 +
net/netfilter/nf_tables_api.c | 12 ++++++++++++
net/netfilter/nft_dynset.c | 2 +-
net/netfilter/nft_lookup.c | 2 +-
net/netfilter/nft_objref.c | 2 +-
5 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index fe56b2f825b4..2db486e9724c 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -462,6 +462,7 @@ struct nft_set_binding {
};
enum nft_trans_phase;
+void nf_tables_activate_set(const struct nft_ctx *ctx, struct nft_set *set);
void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding,
enum nft_trans_phase phase);
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 2f5b5d563e4d..c683a45b8ae5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3420,12 +3420,24 @@ void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
}
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
+void nf_tables_activate_set(const struct nft_ctx *ctx, struct nft_set *set)
+{
+ if (set->flags & NFT_SET_ANONYMOUS)
+ nft_clear(ctx->net, set);
+
+ set->use++;
+}
+EXPORT_SYMBOL_GPL(nf_tables_activate_set);
+
void nf_tables_deactivate_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding,
enum nft_trans_phase phase)
{
switch (phase) {
case NFT_TRANS_PREPARE:
+ if (set->flags & NFT_SET_ANONYMOUS)
+ nft_deactivate_next(ctx->net, set);
+
set->use--;
return;
case NFT_TRANS_ABORT:
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index a20f1668328d..74e8fdaa3432 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -237,7 +237,7 @@ static void nft_dynset_activate(const struct nft_ctx *ctx,
{
struct nft_dynset *priv = nft_expr_priv(expr);
- priv->set->use++;
+ nf_tables_activate_set(ctx, priv->set);
}
static void nft_dynset_destroy(const struct nft_ctx *ctx,
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 453f84c57166..4fcbe51e88c7 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -132,7 +132,7 @@ static void nft_lookup_activate(const struct nft_ctx *ctx,
{
struct nft_lookup *priv = nft_expr_priv(expr);
- priv->set->use++;
+ nf_tables_activate_set(ctx, priv->set);
}
static void nft_lookup_destroy(const struct nft_ctx *ctx,
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index 7e628f4f02b9..49a067a67e72 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -168,7 +168,7 @@ static void nft_objref_map_activate(const struct nft_ctx *ctx,
{
struct nft_objref_map *priv = nft_expr_priv(expr);
- priv->set->use++;
+ nf_tables_activate_set(ctx, priv->set);
}
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
` (5 preceding siblings ...)
2023-05-11 15:41 ` [PATCH -stable,4.14 6/6] netfilter: nf_tables: deactivate anonymous set from preparation phase Pablo Neira Ayuso
@ 2023-05-11 21:53 ` Sasha Levin
2023-05-13 3:25 ` luwei (O)
7 siblings, 0 replies; 10+ messages in thread
From: Sasha Levin @ 2023-05-11 21:53 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, fw, gregkh, stable
On Thu, May 11, 2023 at 05:41:37PM +0200, Pablo Neira Ayuso wrote:
>Hi Greg, Sasha,
>
>This is a backport of c1592a89942e ("netfilter: nf_tables: deactivate anonymous
>set from preparation phase") which fixes CVE-2023-32233. This patch requires
>dependency fixes which are not currently in the 4.14 branch.
Queued up, thanks!
--
Thanks,
Sasha
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14
2023-05-11 15:41 [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Pablo Neira Ayuso
` (6 preceding siblings ...)
2023-05-11 21:53 ` [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14 Sasha Levin
@ 2023-05-13 3:25 ` luwei (O)
2023-05-13 6:20 ` Greg KH
7 siblings, 1 reply; 10+ messages in thread
From: luwei (O) @ 2023-05-13 3:25 UTC (permalink / raw)
To: Pablo Neira Ayuso, netfilter-devel; +Cc: fw, gregkh, sashal, stable
Hi, Pablo, I followed up this CVE for several days but I can't figure
out which commit caused this CVE, It seems the
kernel is affected from 4.0 version according:
https://www.suse.com/security/cve/CVE-2023-32233.html.
So is there any fix patches for the lower versions ?
在 2023/5/11 11:41 PM, Pablo Neira Ayuso 写道:
> Hi Greg, Sasha,
>
> This is a backport of c1592a89942e ("netfilter: nf_tables: deactivate anonymous
> set from preparation phase") which fixes CVE-2023-32233. This patch requires
> dependency fixes which are not currently in the 4.14 branch.
>
> The following list shows the backported patches, I am using original commit IDs
> for reference:
>
> 1) cd5125d8f518 ("netfilter: nf_tables: split set destruction in deactivate and destroy phase")
>
> 2) f6ac85858976 ("netfilter: nf_tables: unbind set in rule from commit path")
>
> 3) 7f4dae2d7f03 ("netfilter: nft_hash: fix nft_hash_deactivate")
>
> 4) 6a0a8d10a366 ("netfilter: nf_tables: use-after-free in failing rule with bound set")
>
> 5) 273fe3f1006e ("netfilter: nf_tables: bogus EBUSY when deleting set after flush")
>
> 6) c1592a89942e ("netfilter: nf_tables: deactivate anonymous set from preparation phase")
>
> Please apply to 4.14-stable.
>
> Thanks.
>
> Florian Westphal (1):
> netfilter: nf_tables: split set destruction in deactivate and destroy phase
>
> Pablo Neira Ayuso (5):
> netfilter: nf_tables: unbind set in rule from commit path
> netfilter: nft_hash: fix nft_hash_deactivate
> netfilter: nf_tables: use-after-free in failing rule with bound set
> netfilter: nf_tables: bogus EBUSY when deleting set after flush
> netfilter: nf_tables: deactivate anonymous set from preparation phase
>
> include/net/netfilter/nf_tables.h | 30 ++++++-
> net/netfilter/nf_tables_api.c | 139 +++++++++++++++++++++---------
> net/netfilter/nft_dynset.c | 22 ++++-
> net/netfilter/nft_immediate.c | 6 +-
> net/netfilter/nft_lookup.c | 21 ++++-
> net/netfilter/nft_objref.c | 21 ++++-
> net/netfilter/nft_set_hash.c | 2 +-
> 7 files changed, 194 insertions(+), 47 deletions(-)
>
--
Best Regards,
Lu Wei
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH netfilter -stable,4.14 0/6] stable fixes for 4.14
2023-05-13 3:25 ` luwei (O)
@ 2023-05-13 6:20 ` Greg KH
0 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2023-05-13 6:20 UTC (permalink / raw)
To: luwei (O); +Cc: Pablo Neira Ayuso, netfilter-devel, fw, sashal, stable
On Sat, May 13, 2023 at 11:25:33AM +0800, luwei (O) wrote:
> Hi, Pablo, I followed up this CVE for several days but I can't figure out
> which commit caused this CVE, It seems the
>
> kernel is affected from 4.0 version according:
> https://www.suse.com/security/cve/CVE-2023-32233.html.
>
> So is there any fix patches for the lower versions ?
There are no more supported "lower versions" of the kernel at the moment
by the community. So if you are stuck on a older one, please work with
the vendor that is providing you that support as they are the only ones
that can provide that _and_ you are already paying for that support.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread