Re: [RFC] nftables 0.9.8 -stable backports

[Jeremy Sowden <jeremy@azazel.net>]

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

On 2024-02-17, at 20:11:42 +0000, Jeremy Sowden wrote:
> 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?

Forgot to run the test-suite.  Doing so revealed that this doesn't quite
work because `set_alloc` doesn't initialize `s->list`.  This, however,
does:

  diff --git a/src/evaluate.c b/src/evaluate.c
  index 232ae39020cc..c58e37e14064 100644
  --- a/src/evaluate.c
  +++ b/src/evaluate.c
  @@ -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;
  diff --git a/src/rule.c b/src/rule.c
  index c23f87f47ae2..365feec08c32 100644
  --- a/src/rule.c
  +++ b/src/rule.c
  @@ -339,6 +339,7 @@ struct set *set_alloc(const struct location *loc)
          if (loc != NULL)
                  set->location = *loc;
   
  +       init_list_head(&set->list);
          init_list_head(&set->stmt_list);
   
          return set;
  @@ -360,6 +361,7 @@ struct set *set_clone(const struct set *set)
          new_set->policy         = set->policy;
          new_set->automerge      = set->automerge;
          new_set->desc           = set->desc;
  +       init_list_head(&new_set->list);
          init_list_head(&new_set->stmt_list);
   
          return new_set;

Alternatively, we could continue adding the set to the cached table, but
without the seg-fault:

  diff --git a/src/evaluate.c b/src/evaluate.c
  index 232ae39020cc..23ff982b73f0 100644
  --- a/src/evaluate.c
  +++ b/src/evaluate.c
  @@ -3760,7 +3760,7 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
          ctx->set = NULL;
   
          if (set_lookup(table, set->handle.set.name) == NULL)
  -               set_add_hash(set_get(set), table);
  +               set_add_hash(set, table);
   
          return 0;
   }
  diff --git a/src/rule.c b/src/rule.c
  index c23f87f47ae2..0aaefc54c30d 100644
  --- a/src/rule.c
  +++ b/src/rule.c
  @@ -339,6 +339,7 @@ struct set *set_alloc(const struct location *loc)
          if (loc != NULL)
                  set->location = *loc;
   
  +       init_list_head(&set->list);
          init_list_head(&set->stmt_list);
   
          return set;
  @@ -360,6 +361,7 @@ struct set *set_clone(const struct set *set)
          new_set->policy         = set->policy;
          new_set->automerge      = set->automerge;
          new_set->desc           = set->desc;
  +       init_list_head(&new_set->list);
          init_list_head(&new_set->stmt_list);
   
          return new_set;
  @@ -391,7 +393,10 @@ void set_free(struct set *set)
   
   void set_add_hash(struct set *set, struct table *table)
   {
  -       list_add_tail(&set->list, &table->sets);
  +       if (list_empty(&set->list))
  +               list_add_tail(&set_get(set)->list, &table->sets);
  +       else
  +               list_move_tail(&set->list, &table->sets);
   }
   
   struct set *set_lookup(const struct table *table, const char *name)
  
J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]