[PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[Joas Antonio dos Santos]

Since nft_set_commit_update() runs set commit callbacks before processing
NEWSETELEM transactions, pipapo_gc_scan() can observe elements added by
the transaction being committed: nft_pipapo_commit() calls
pipapo_gc_scan() on the clone, which already holds the freshly inserted
(not yet activated) elements.

pipapo_gc_scan() selects elements to collect using only
__nft_set_elem_expired(), without checking the element's transaction
state.  A new element that is already expired at commit time (for
example a short expiration whose delta elapses during a large batch
parse) is deactivated, dropped and queued on the synchronous gc list,
which frees it after an RCU grace period.  The later NEWSETELEM step of
the same transaction then calls nft_setelem_activate() ->
nft_pipapo_activate() -> nft_clear() on the already freed element,
causing a use-after-free.

This is the pipapo counterpart of the rbtree fix in commit 86b6471e690c
("netfilter: nft_set_rbtree: skip transaction elements during GC"); the
rbtree scan gained an nft_set_elem_active() guard but the pipapo scan
was left unchanged.

Only consider elements that are fully active in both generations, which
keeps transaction-state elements out of the GC scan.

The use-after-free has the same shape as the rbtree one:

  BUG: KASAN: slab-use-after-free in nft_setelem_activate
  nft_setelem_activate net/netfilter/nf_tables_api.c
  nf_tables_commit net/netfilter/nf_tables_api.c

  Allocated by nft_set_elem_init / nft_add_set_elem
  Freed by nft_trans_gc_trans_free / rcu_core

Fixes: 1e3b9e1c77fe ("netfilter: nf_tables: call set ops .commit when building new ruleset blob")
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
Compile-tested (arm64, W=1, CONFIG_NF_TABLES=y): nft_set_pipapo.o builds
with no new warnings.

I demonstrated the precondition on a KASAN VM (unpatched 7.3-rc6): adding
20000 fresh elements with a short timeout (so they are already expired at
the commit tstamp) to a timeout+interval pipapo set leaves ~0 elements in
the set afterwards, i.e. pipapo_gc_scan() collects the not-yet-activated
NEWSETELEM elements during the very commit that adds them - exactly what
the nft_set_elem_active() guard prevents. I did not capture the KASAN
splat itself: the free is call_rcu()-deferred (nft_trans_gc_queue_sync_done),
so the subsequent nft_setelem_activate() -> nft_clear() races the grace
period, same as the rbtree sibling.

This is the pipapo counterpart of 86b6471e690c ("netfilter:
nft_set_rbtree: skip transaction elements during GC"): same root cause
(1e3b9e1c77fe runs set .commit before NEWSETELEM activation), same
scan-without-active-check pattern, same nft_setelem_activate()
use-after-free shape. pipapo_gc_scan() was not given the
nft_set_elem_active() guard that the rbtree scan received.

 net/netfilter/nft_set_pipapo.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 978bb0c01..4680969eb 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -1745,6 +1745,18 @@ static void pipapo_gc_scan(struct nft_set *set, struct nft_pipapo_match *m)
 		i--;
 		e = f->mt[rulemap[i].to].e;
 
+		/* Skip entries not fully active in both generations: these are
+		 * elements inserted by the transaction currently being
+		 * committed, since nft_set_commit_update() runs the set .commit
+		 * callback before NEWSETELEM activation.  Collecting such an
+		 * element here lets synchronous gc free it before
+		 * nft_setelem_activate() runs on it, causing a use-after-free.
+		 */
+		if (!nft_set_elem_active(&e->ext, NFT_GENMASK_ANY)) {
+			first_rule += rules_f0;
+			continue;
+		}
+
 		/* synchronous gc never fails, there is no need to set on
 		 * NFT_SET_ELEM_DEAD_BIT.
 		 */

Re: [PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[Florian Westphal]

Joas Antonio dos Santos <joasantonio108@gmail.com> wrote:
> Since nft_set_commit_update() runs set commit callbacks before processing
> NEWSETELEM transactions, pipapo_gc_scan() can observe elements added by
> the transaction being committed: nft_pipapo_commit() calls
> pipapo_gc_scan() on the clone, which already holds the freshly inserted
> (not yet activated) elements.
> 
> pipapo_gc_scan() selects elements to collect using only
> __nft_set_elem_expired(), without checking the element's transaction
> state.  A new element that is already expired at commit time (for
> example a short expiration whose delta elapses during a large batch

Please ask your LLM to explain how a newly added element can be both new
and expired at the same time, then fix that.

Re: [PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[Joas Antonio dos Santos]

Florian Westphal <fw@strlen.de> wrote:
> Please ask your LLM to explain how a newly added element can be both
> new and expired at the same time, then fix that.

Hi Florian,

Fair question, it confused me too at first, so I went and traced it.

The catch is that the expiration isn't stamped at activation, it's
stamped when the element is allocated. nft_set_elem_init() does:

	nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;

and that happens back in the parse phase (nft_add_set_elem), while we're
still going through the batch. The element only goes active later, in
nf_tables_commit(), which is also where the gc tstamp is taken
(nft_net->tstamp = get_jiffies_64()).

So the expiration is really parse_time + timeout, and the commit-time gc
scan checks it against commit_time. If you add a big batch of elements
with a short timeout, they get parsed one by one and the commit only
happens at the very end, so the ones parsed early in the batch have
already gone past their timeout by the time we reach the commit tstamp.
They're expired and still new (not activated yet). That's the "both at
once" part.

I confirmed it on an unpatched 7.3-rc6 + KASAN: add 20000 elements with
"timeout 10ms" to an interval+timeout pipapo set (the parse alone takes
~6s) and the set ends up basically empty. pipapo_gc_scan() collected the
new, not-yet-activated elements during the same commit that added them.
With no nft_set_elem_active() check they get deactivated, dropped and
queued for the sync gc free, and then nft_setelem_activate() ->
nft_pipapo_activate() -> nft_clear() still runs on them.

It's the same thing you already fixed for rbtree in 86b6471e690c, the
pipapo scan just never got the active() guard. One difference: on rbtree
a non-expired new interval-end can also get pulled in through the
interval pairing; pipapo has no pairing, so here it's specifically the
expired-at-commit new element.

Full disclosure: I didn't manage to trip the actual UAF splat. The free
goes through call_rcu (nft_trans_gc_queue_sync_done), so the activate
races the grace period just like the rbtree case, but the precondition
(gc picking up the not-yet-active elements) reproduces every time.

If it'd help, I can respin v2 with the parse-time vs commit-time
expiration written out in the commit message, just let me know.

Thanks for taking a look,
Joas

Re: [PATCH nf] netfilter: nft_set_pipapo: skip transaction elements during GC

[Florian Westphal]

Joas Antonio dos Santos <joasantonio108@gmail.com> wrote:
> The catch is that the expiration isn't stamped at activation, it's
> stamped when the element is allocated. nft_set_elem_init() does:
> 
> 	nft_set_ext_timeout(ext)->expiration = get_jiffies_64() + expiration;
> 
> and that happens back in the parse phase (nft_add_set_elem), while we're
> still going through the batch. The element only goes active later, in
> nf_tables_commit(), which is also where the gc tstamp is taken
> (nft_net->tstamp = get_jiffies_64()).


Can you name the line in the source code?
I am not seeing it :-(

> So the expiration is really parse_time + timeout, and the commit-time gc
> scan checks it against commit_time. If you add a big batch of elements
> with a short timeout, they get parsed one by one and the commit only
> happens at the very end, so the ones parsed early in the batch have
> already gone past their timeout by the time we reach the commit tstamp.

Thats not supposed to cause a gc?

All the sync gc functions are supposed to check against the reference
timestamp taken at the beginning of the transaction start, in
nf_tables_valid_genid().

They are NOT supposed to check against wall clock time.
Even if transaction takes 5 minutes: the sync gc step should
NOT observe even a single timed out elemenent.

They become stale only AFTER the sync gc step.

tstamp is coming from nft_nat->tstamp, i.e. start of transaction.
e->ext timestamp therefore should never be in the past compared
to that reference time.

1751                 if (__nft_set_elem_expired(&e->ext, tstamp)) {
1752                         if (!nft_trans_gc_space(gc)) {
1753                                 gc = nft_trans_gc_alloc(set, 0, GFP_KERNEL);
1754                                 if (!gc)

> I confirmed it on an unpatched 7.3-rc6 + KASAN: add 20000 elements with
> "timeout 10ms" to an interval+timeout pipapo set (the parse alone takes
> ~6s) and the set ends up basically empty. pipapo_gc_scan() collected the
> new, not-yet-activated elements during the same commit that added them.

How? I'm not seeing it :-(

> It's the same thing you already fixed for rbtree in 86b6471e690c, the

I thought that was about false zap of rbtree end nodes which doesn't
apply here :-(

> races the grace period just like the rbtree case, but the precondition
> (gc picking up the not-yet-active elements) reproduces every time.

How?

> If it'd help, I can respin v2 with the parse-time vs commit-time
> expiration written out in the commit message, just let me know.

Yes, please explain in detail how time can advance during transaction,
because its supposedly impossible.