Hi, Pablo.
On 2023-10-09, at 13:36:23 +0200, Pablo Neira Ayuso wrote:
> This is a small batch offering fixes for nftables 0.9.8. It only
> includes the fixes for the implicit chain regression in recent
> kernels.
>
> This is a few dependency patches that are missing in 0.9.8 are
> required:
>
> 3542e49cf539 ("evaluate: init cmd pointer for new on-stack context")
> a3ac2527724d ("src: split chain list in table")
> 4e718641397c ("cache: rename chain_htable to cache_chain_ht")
>
> a3ac2527724d is fixing an issue with the cache that is required by the
> fixes. Then, the backport fixes for the implicit chain regression with
> Linux -stable:
>
> 3975430b12d9 ("src: expand table command before evaluation")
> 27c753e4a8d4 ("rule: expand standalone chain that contains rules")
> 784597a4ed63 ("rule: add helper function to expand chain rules into commands")
>
> I tested with tests/shell at the time of the nftables 0.9.8 release
> (*I did not use git HEAD tests/shell as I did for 1.0.6*).
>
> I have kept back the backport of this patch intentionally:
>
> 56c90a2dd2eb ("evaluate: expand sets and maps before evaluation")
>
> this depends on the new src/interval.c code, in 0.9.8 overlap and
> automerge come a later stage and cache is not updated incrementally,
> I tried the tests coming in this patch and it works fine.
>
> I did run a few more tests with rulesets that I have been collecting
> from people that occasionally send them to me for my personal ruleset
> repo.
>
> I: results: [OK] 266 [FAILED] 0 [TOTAL] 266
>
> This has been tested with latest Linux kernel 5.10 -stable.
>
> I can still run a few more tests, I will get back to you if I find any
> issue.
>
> Let me know, thanks.
A new version of nftables containing these fixes was released as part of
the Debian 11.9 point release, which happened a week ago. Since then,
we've had a couple of bug-reports:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1063690
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1063769
The gist of them is that if nft processes a file containing multiple
table-blocks for the same table, and there is a set definition in one of
the non-initial ones, e.g.:
table inet t {
}
table inet t {
set s {
type inet_service
elements = { 42 }
}
}
it crashes with a seg-fault.
The bison parser creates two `CMD_ADD` commands and allocates two
`struct table` objects (which I shall refer to as `t0` and `t1`). When
it creates the second command, it also allocates a `struct set` object,
`s`, which it adds to `t1->sets`. After the `CMD_ADD` commands for `t0`
and `t1` have been expanded, when the new `CMD_ADD` command for `s` is
evaluated, `set_evaluate` does this (evaluate.c, ll. 3686ff.):
table = table_lookup_global(ctx);
if (table == NULL)
return table_not_found(ctx);
and later this (evaluate.c, ll. 3762f.):
if (set_lookup(table, set->handle.set.name) == NULL)
set_add_hash(set_get(set), table);
The `struct table` object returned by `table_lookup_global` is `t0`,
since this was evaluated first and cached by `table_evaluate`, not `t1`.
Therefore, `set_lookup` returns `NULL`, `set_add_hash` is called, `s` is
added to `t0->sets`, and `t1->sets` is effectively corrupted. It now
contains two elements which point to each other, and one of them is not
a set at all, but `t0->sets`. This results in a seg-fault when nft
tries to free `t1`.
I _think_ that the following is all that is needed to fix it:
@@ -3759,7 +3759,8 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
}
ctx->set = NULL;
- if (set_lookup(table, set->handle.set.name) == NULL)
+ if (set_lookup(table, set->handle.set.name) == NULL &&
+ list_empty(&set->list))
set_add_hash(set_get(set), table);
return 0;
Does this look good to you?
J.
Attachment:
signature.asc
Description: PGP signature
