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

[Joas Antonio dos Santos]

Florian Westphal <fw@xxxxxxxxx> 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