Re: [RFC] nftables 0.9.8 -stable backports

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]