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

[Florian Westphal <fw@strlen.de>]

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);