Re: [PATCH net] netfilter: nf_flow_table_offload: fix heap overflow in flow_action_entry_next()

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Mar 25, 2026 at 03:27:51PM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > > hmm. So, based on what you said, I assume the run-time check would look 
> > > something like this?
> > > 
> > > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> > > index 9b677e116487..69ffefbdd5e8 100644
> > > --- a/net/netfilter/nf_flow_table_offload.c
> > > +++ b/net/netfilter/nf_flow_table_offload.c
> > > @@ -218,6 +218,9 @@ flow_action_entry_next(struct nf_flow_rule *flow_rule)
> > >  {
> > >         int i = flow_rule->rule->action.num_entries++;
> > > 
> > > +       if (WARN_ON_ONCE(i >= NF_FLOW_RULE_ACTION_MAX))
> > > +               return NULL;
> > > +
> > >         return &flow_rule->rule->action.entries[i];
> > >  }
> > > 
> > > However, if we add a runtime check in this way, all callers of 
> > > flow_action_entry_next() would also need to handle a NULL return value, 
> > > since none of them currently perform a null check.
> > > 
> > > Because of the potential risk, this would require modifying quite a number 
> > > of call sites carefully. What do you think about this approach?
> > 
> > Can't we reject this at configuration time?
> > 
> > I mean, userspace has to ask for this action sequence, no?
> 
> Guess:
> 
> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> --- a/net/netfilter/nf_tables_offload.c
> +++ b/net/netfilter/nf_tables_offload.c
> @@ -105,6 +105,9 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net,
>         if (num_actions == 0)
>                 return ERR_PTR(-EOPNOTSUPP);
> 
> +       if (num_actions > NF_FLOW_RULE_ACTION_MAX)
> +               return ERR_PTR(-EOPNOTSUPP);
> +
>         flow = nft_flow_rule_alloc(num_actions);
>         if (!flow)
>                 return ERR_PTR(-ENOMEM);
> 

That is good enough, thanks.

Would you submit this?

This is nf-next material, it is not possible to reach such number of
actions (16) in the flowtable.