Re: [PATCH nf] netfilter: nft_set_rbtree: do not pair an expired start with another interval's end

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

On Sat, Oct 31, 2026 at 02:55:55PM -0300, Joas Antonio wrote:
> Hi Pablo,
> 
> Sorry, the description was too terse. Answering both mails here.
> 
> There is no memory safety issue, and I should have said so explicitly.
> It is a correctness problem: after GC the set no longer holds what was
> committed, and the packet path matches keys that were never added.
> 
> Exact sequence, raw nfnetlink. The set has NFT_SET_INTERVAL |
> NFT_SET_TIMEOUT, an ipv4_addr key, no default timeout, and
> NFTA_SET_GC_INTERVAL = 3600000, so the commit-time scan does not run and
> only the insertion path is involved.
> 
> Batch 1 (NEWSETELEM): start 10 with a 50ms timeout, end 20 with no
> timeout, start 30 and end 40 with no timeout. Only start 10 expires;
> [30,40) is permanent.
> 
> Wait 200ms.
> 
> Batch 2: DELSETELEM of end 20 only, then NEWSETELEM of [12,13). This is
> not something nft(8) generates, but the kernel accepts it. Inserting 12
> runs nft_rbtree_gc_elem() on the expired start 10. The walk skips end
> 20 because it is being deleted in this transaction, passes start 30 and
> takes end 40 as the end of 10. 10 and 40 are removed, and start 30 is
> left without an end.
> 
> At commit, nft_rbtree_commit() turns 30 into an interval with ->from =
> 30 and ->to = NULL, and nft_array_lookup_cmp() treats ->to == NULL as
> open-ended. So nft_rbtree_lookup() matches every key >= 30, while
> userspace believes the set holds only [30,40). Reading back with
> GETSETELEM, which uses the same array: without the patch 45 is found,
> end 40 is gone and deleting [30,40) fails with ENOENT; with the patch
> 45 is not found, 40 is present and the delete succeeds.
> 
> To be fair about the limits: the only trigger I have is crafted
> nfnetlink from whoever already owns the ruleset (CAP_NET_ADMIN in that
> netns), so it does not cross a privilege boundary, and I have not found
> an nft(8) command sequence that reaches the insertion-path case.

Ok, then you confirm such sequence makes no sense for nft(8) command.

> The commit-path case (nft_rbtree_gc_scan() not clearing rbe_end)
> uses a different sequence without the lone end delete; I can send it
> in the same detail if useful.
> 
> If you consider sets built from such batches unsupported, I think the
> better fix is to reject deleting a lone interval end element, and I can
> send that instead; or drop this if it is not worth the churn.

While it would be good if this interface could be tighten in the
future to reject nonsense, I think it is not worth the churn at this
stage.