diff mbox series

[nf] netfilter: nf_tables: reject requests exceeding NF_FLOW_RULE_ACTION_MAX actions

Message ID 20260325164130.29060-1-fw@strlen.de
State Not Applicable
Headers show
Series [nf] netfilter: nf_tables: reject requests exceeding NF_FLOW_RULE_ACTION_MAX actions | expand

Commit Message

Florian Westphal March 25, 2026, 4:41 p.m. UTC
nf_flow_offload_rule_alloc() allocates space for NF_FLOW_RULE_ACTION_MAX
entries.  Make sure userspace passes more entries to us.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Can also route via nf-next if thats deemed the better tree.

 include/net/netfilter/nf_flow_table.h | 2 ++
 net/netfilter/nf_flow_table_offload.c | 2 --
 net/netfilter/nf_tables_offload.c     | 5 +++--
 3 files changed, 5 insertions(+), 4 deletions(-)

Comments

Fernando Fernandez Mancera March 26, 2026, 12:03 p.m. UTC | #1
On 3/25/26 5:41 PM, Florian Westphal wrote:
> nf_flow_offload_rule_alloc() allocates space for NF_FLOW_RULE_ACTION_MAX
> entries.  Make sure userspace passes more entries to us.
> 

nit: shouldn't this be "Make sure userspace does not pass more entries 
to us"?

Other than that, LGTM.

Thanks.

> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>   Can also route via nf-next if thats deemed the better tree.
> 
>   include/net/netfilter/nf_flow_table.h | 2 ++
>   net/netfilter/nf_flow_table_offload.c | 2 --
>   net/netfilter/nf_tables_offload.c     | 5 +++--
>   3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index b09c11c048d5..0b2fb1467b3f 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -13,6 +13,8 @@
>   #include <linux/if_pppox.h>
>   #include <linux/ppp_defs.h>
>   
> +#define NF_FLOW_RULE_ACTION_MAX	16
> +
>   struct nf_flowtable;
>   struct nf_flow_rule;
>   struct flow_offload;
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 9b677e116487..11463682bbfa 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -727,8 +727,6 @@ int nf_flow_rule_route_ipv6(struct net *net, struct flow_offload *flow,
>   }
>   EXPORT_SYMBOL_GPL(nf_flow_rule_route_ipv6);
>   
> -#define NF_FLOW_RULE_ACTION_MAX	16
> -
>   static struct nf_flow_rule *
>   nf_flow_offload_rule_alloc(struct net *net,
>   			   const struct flow_offload_work *offload,
> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> index 9101b1703b52..a2f7966bc201 100644
> --- a/net/netfilter/nf_tables_offload.c
> +++ b/net/netfilter/nf_tables_offload.c
> @@ -88,10 +88,11 @@ static void nft_flow_rule_transfer_vlan(struct nft_offload_ctx *ctx,
>   struct nft_flow_rule *nft_flow_rule_create(struct net *net,
>   					   const struct nft_rule *rule)
>   {
> +	unsigned int num_actions = 0;
>   	struct nft_offload_ctx *ctx;
>   	struct nft_flow_rule *flow;
> -	int num_actions = 0, err;
>   	struct nft_expr *expr;
> +	int err;
>   
>   	expr = nft_expr_first(rule);
>   	while (nft_expr_more(rule, expr)) {
> @@ -102,7 +103,7 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net,
>   		expr = nft_expr_next(expr);
>   	}
>   
> -	if (num_actions == 0)
> +	if (num_actions == 0 || num_actions > NF_FLOW_RULE_ACTION_MAX)
>   		return ERR_PTR(-EOPNOTSUPP);
>   
>   	flow = nft_flow_rule_alloc(num_actions);
Pablo Neira Ayuso March 26, 2026, 9:49 p.m. UTC | #2
On Wed, Mar 25, 2026 at 05:41:27PM +0100, Florian Westphal wrote:
> nf_flow_offload_rule_alloc() allocates space for NF_FLOW_RULE_ACTION_MAX
> entries.  Make sure userspace passes more entries to us.

While the flowtable hardware offload uses a fixed maximum number of
actions NF_FLOW_RULE_ACTION_MAX for simplicity.

But nf_tables hardware offload allocates the number of actions
dynamically from nft_flow_rule_create(), such function iterates to
check if there is .offload_action is true, the increments the array of
actions by one for each.

Possible actions (note payload mangling is currently not supported
in nf_tables hardware offload).

This is fragile, because advancing the action array is opencoded:

        entry = &flow->rule->action.entries[ctx->num_actions++];

I can make a patch for nf-next to add a helper, but I don't see any
issue on nf_tables_offload at this stage. There are three actions only
and they add one single entry to the array.

As for the flowtable hardware offload (different infrastructure)
I proposed a different approach:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260326200935.729750-1-pablo@netfilter.org/
diff mbox series

Patch

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index b09c11c048d5..0b2fb1467b3f 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -13,6 +13,8 @@ 
 #include <linux/if_pppox.h>
 #include <linux/ppp_defs.h>
 
+#define NF_FLOW_RULE_ACTION_MAX	16
+
 struct nf_flowtable;
 struct nf_flow_rule;
 struct flow_offload;
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 9b677e116487..11463682bbfa 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -727,8 +727,6 @@  int nf_flow_rule_route_ipv6(struct net *net, struct flow_offload *flow,
 }
 EXPORT_SYMBOL_GPL(nf_flow_rule_route_ipv6);
 
-#define NF_FLOW_RULE_ACTION_MAX	16
-
 static struct nf_flow_rule *
 nf_flow_offload_rule_alloc(struct net *net,
 			   const struct flow_offload_work *offload,
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 9101b1703b52..a2f7966bc201 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -88,10 +88,11 @@  static void nft_flow_rule_transfer_vlan(struct nft_offload_ctx *ctx,
 struct nft_flow_rule *nft_flow_rule_create(struct net *net,
 					   const struct nft_rule *rule)
 {
+	unsigned int num_actions = 0;
 	struct nft_offload_ctx *ctx;
 	struct nft_flow_rule *flow;
-	int num_actions = 0, err;
 	struct nft_expr *expr;
+	int err;
 
 	expr = nft_expr_first(rule);
 	while (nft_expr_more(rule, expr)) {
@@ -102,7 +103,7 @@  struct nft_flow_rule *nft_flow_rule_create(struct net *net,
 		expr = nft_expr_next(expr);
 	}
 
-	if (num_actions == 0)
+	if (num_actions == 0 || num_actions > NF_FLOW_RULE_ACTION_MAX)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	flow = nft_flow_rule_alloc(num_actions);