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

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

[Hyunwoo Kim]

flow_action_entry_next() increments num_entries and returns a pointer
into the flow_action_entry array without any bounds checking.  The array
is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
require 17 or more entries, causing a slab-out-of-bounds write in the
kmalloc-4k slab.

The maximum possible entry count is:
  tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21

Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
worst case.

Fixes: efce49dfe6a8 ("netfilter: flowtable: add vlan pop action offload support")
Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 net/netfilter/nf_flow_table_offload.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index 9b677e116487..e843f6d0355e 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -727,7 +727,7 @@ 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
+#define NF_FLOW_RULE_ACTION_MAX	24
 
 static struct nf_flow_rule *
 nf_flow_offload_rule_alloc(struct net *net,
-- 
2.43.0

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

[Florian Westphal]

Hyunwoo Kim <imv4bel@gmail.com> wrote:
> flow_action_entry_next() increments num_entries and returns a pointer
> into the flow_action_entry array without any bounds checking.  The array
> is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> require 17 or more entries, causing a slab-out-of-bounds write in the
> kmalloc-4k slab.
> 
> The maximum possible entry count is:
>   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21
> 
> Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
>  
> -#define NF_FLOW_RULE_ACTION_MAX	16
> +#define NF_FLOW_RULE_ACTION_MAX	24

This fix looks rather fragile.

What guarantees that this stays right-sized?

Can you add a BUILD_BUG_ON or if needed, run-time check?

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

[Pablo Neira Ayuso]

On Sun, Mar 08, 2026 at 02:23:06AM +0900, Hyunwoo Kim wrote:
> flow_action_entry_next() increments num_entries and returns a pointer
> into the flow_action_entry array without any bounds checking.  The array
> is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> require 17 or more entries, causing a slab-out-of-bounds write in the
> kmalloc-4k slab.
> 
> The maximum possible entry count is:
>   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21

There is no hardware offload for the:

- tunnel support
- IPv6 NAT

This is all very hypothetical.

> Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
> worst case.
> 
> Fixes: efce49dfe6a8 ("netfilter: flowtable: add vlan pop action offload support")
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  net/netfilter/nf_flow_table_offload.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
> index 9b677e116487..e843f6d0355e 100644
> --- a/net/netfilter/nf_flow_table_offload.c
> +++ b/net/netfilter/nf_flow_table_offload.c
> @@ -727,7 +727,7 @@ 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
> +#define NF_FLOW_RULE_ACTION_MAX	24
>  
>  static struct nf_flow_rule *
>  nf_flow_offload_rule_alloc(struct net *net,
> -- 
> 2.43.0
> 

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

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > flow_action_entry_next() increments num_entries and returns a pointer
> > into the flow_action_entry array without any bounds checking.  The array
> > is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> > but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> > require 17 or more entries, causing a slab-out-of-bounds write in the
> > kmalloc-4k slab.
> > 
> > The maximum possible entry count is:
> >   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21
> > 
> > Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
> >  
> > -#define NF_FLOW_RULE_ACTION_MAX	16
> > +#define NF_FLOW_RULE_ACTION_MAX	24
> 
> This fix looks rather fragile.
> 
> What guarantees that this stays right-sized?
> 
> Can you add a BUILD_BUG_ON or if needed, run-time check?

Ping.  I'm not even sure if there is a bug to begin with, see Pablos
response.  How did you conclude there is a missing bounds check and that
this increase is the best fix?

Normally there should be a check that prevents such a configuration.
If thats missing, please add one instead of increasing this define.

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

[Hyunwoo Kim]

On Mon, Mar 16, 2026 at 11:53:56AM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > > flow_action_entry_next() increments num_entries and returns a pointer
> > > into the flow_action_entry array without any bounds checking.  The array
> > > is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> > > but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> > > require 17 or more entries, causing a slab-out-of-bounds write in the
> > > kmalloc-4k slab.
> > > 
> > > The maximum possible entry count is:
> > >   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21
> > > 
> > > Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
> > >  
> > > -#define NF_FLOW_RULE_ACTION_MAX	16
> > > +#define NF_FLOW_RULE_ACTION_MAX	24
> > 
> > This fix looks rather fragile.
> > 
> > What guarantees that this stays right-sized?
> > 
> > Can you add a BUILD_BUG_ON or if needed, run-time check?
> 
> Ping.  I'm not even sure if there is a bug to begin with, see Pablos

Sorry for the late reply.

To clarify, I triggered the overflow using a dummy device that accepts
TC_SETUP_FT, as I don't have real offload-capable hardware. The 17 entry
scenario requires double VLAN (QinQ) + IPv6 + SNAT + DNAT simultaneously,
which is unlikely in real-world deployments, so it is hypothetical.

> response.  How did you conclude there is a missing bounds check and that
> this increase is the best fix?
> 
> Normally there should be a check that prevents such a configuration.
> If thats missing, please add one instead of increasing this define.

So, should I send a v2 with a bounds check, or drop this patch?

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

[Hyunwoo Kim]

On Mon, Mar 16, 2026 at 08:23:56PM +0900, Hyunwoo Kim wrote:
> On Mon, Mar 16, 2026 at 11:53:56AM +0100, Florian Westphal wrote:
> > Florian Westphal <fw@strlen.de> wrote:
> > > Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > > > flow_action_entry_next() increments num_entries and returns a pointer
> > > > into the flow_action_entry array without any bounds checking.  The array
> > > > is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> > > > but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> > > > require 17 or more entries, causing a slab-out-of-bounds write in the
> > > > kmalloc-4k slab.
> > > > 
> > > > The maximum possible entry count is:
> > > >   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21
> > > > 
> > > > Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
> > > >  
> > > > -#define NF_FLOW_RULE_ACTION_MAX	16
> > > > +#define NF_FLOW_RULE_ACTION_MAX	24
> > > 
> > > This fix looks rather fragile.
> > > 
> > > What guarantees that this stays right-sized?
> > > 
> > > Can you add a BUILD_BUG_ON or if needed, run-time check?
> > 
> > Ping.  I'm not even sure if there is a bug to begin with, see Pablos
> 
> Sorry for the late reply.
> 
> To clarify, I triggered the overflow using a dummy device that accepts
> TC_SETUP_FT, as I don't have real offload-capable hardware. The 17 entry
> scenario requires double VLAN (QinQ) + IPv6 + SNAT + DNAT simultaneously,
> which is unlikely in real-world deployments, so it is hypothetical.
> 
> > response.  How did you conclude there is a missing bounds check and that
> > this increase is the best fix?
> > 
> > Normally there should be a check that prevents such a configuration.
> > If thats missing, please add one instead of increasing this define.
> 
> So, should I send a v2 with a bounds check, or drop this patch?

+ Since this is not a real-world environment, it seems better not to apply 
the patch.

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

[Florian Westphal]

Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > Ping.  I'm not even sure if there is a bug to begin with, see Pablos
> 
> Sorry for the late reply.
> 
> To clarify, I triggered the overflow using a dummy device that accepts
> TC_SETUP_FT, as I don't have real offload-capable hardware. The 17 entry
> scenario requires double VLAN (QinQ) + IPv6 + SNAT + DNAT simultaneously,
> which is unlikely in real-world deployments, so it is hypothetical.

If you triggered it, its not hyptothetical and needs to be fixed.

> > Normally there should be a check that prevents such a configuration.
> > If thats missing, please add one instead of increasing this define.
> 
> So, should I send a v2 with a bounds check, or drop this patch?

Yes, please send a v2 that prevents the overflow at configuration time.

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

[Pablo Neira Ayuso]

On Mon, Mar 16, 2026 at 12:48:33PM +0100, Florian Westphal wrote:
> Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > > Ping.  I'm not even sure if there is a bug to begin with, see Pablos
> > 
> > Sorry for the late reply.
> > 
> > To clarify, I triggered the overflow using a dummy device that accepts
> > TC_SETUP_FT, as I don't have real offload-capable hardware. The 17 entry
> > scenario requires double VLAN (QinQ) + IPv6 + SNAT + DNAT simultaneously,
> > which is unlikely in real-world deployments, so it is hypothetical.
> 
> If you triggered it, its not hyptothetical and needs to be fixed.

He triggered it with... a device which is not in the tree? How is
tunnel really supported with TC_SETUP_FT? What driver did gain support
for this? And SNAT and DNAT !?!?

> > > Normally there should be a check that prevents such a configuration.
> > > If thats missing, please add one instead of increasing this define.
> > 
> > So, should I send a v2 with a bounds check, or drop this patch?
> 
> Yes, please send a v2 that prevents the overflow at configuration time.

Just rising the maximum amount is a workaround, I did not check yet
the implications of this.

Yes, better checks would be good to have here, I agree, because this
is fragile, for future proofing.

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

[Hyunwoo Kim]

On Mon, Mar 16, 2026 at 12:48:33PM +0100, Florian Westphal wrote:
> Hyunwoo Kim <imv4bel@gmail.com> wrote:
> > > Ping.  I'm not even sure if there is a bug to begin with, see Pablos
> > 
> > Sorry for the late reply.
> > 
> > To clarify, I triggered the overflow using a dummy device that accepts
> > TC_SETUP_FT, as I don't have real offload-capable hardware. The 17 entry
> > scenario requires double VLAN (QinQ) + IPv6 + SNAT + DNAT simultaneously,
> > which is unlikely in real-world deployments, so it is hypothetical.
> 
> If you triggered it, its not hyptothetical and needs to be fixed.
> 
> > > Normally there should be a check that prevents such a configuration.
> > > If thats missing, please add one instead of increasing this define.
> > 
> > So, should I send a v2 with a bounds check, or drop this patch?
> 
> Yes, please send a v2 that prevents the overflow at configuration time.

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?

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

[Florian Westphal]

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?

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

[Florian Westphal]

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

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

[Pablo Neira Ayuso]

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.

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

[Pablo Neira Ayuso]

On Sun, Mar 08, 2026 at 02:23:06AM +0900, Hyunwoo Kim wrote:
> flow_action_entry_next() increments num_entries and returns a pointer
> into the flow_action_entry array without any bounds checking.  The array
> is allocated with a fixed size of NF_FLOW_RULE_ACTION_MAX (16) entries,
> but certain combinations of IPv6 + SNAT + DNAT + double VLAN (QinQ)
> require 17 or more entries, causing a slab-out-of-bounds write in the
> kmalloc-4k slab.
> 
> The maximum possible entry count is:
>   tunnel(2) + eth(4) + VLAN(4) + IPv6_NAT(10) + redirect(1) = 21
> 
> Increase NF_FLOW_RULE_ACTION_MAX to 24 (with headroom) to cover the
> worst case.

For the record, proposed patch:

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