[PATCH nft] src: release scope symbols by reference count

[PATCH nft] src: release scope symbols by reference count

[Avinash Duduskar]

nft -f aborts when a define in table or chain scope takes its value from
another variable:

  define outer = 111

  table inet t {
          define inner = $outer
  }

  nft: src/rule.c:579: scope_release: Assertion `sym->refcnt == 1' failed.

$outer becomes a variable expression that takes a reference on the
symbol, and it is still holding that reference when the top level scope
is released: the table has gone into the cache, so cmd_free() drops one
of its references rather than freeing it.  Using $inner in a rule does
not release it either, expr_evaluate_variable() shares or clones
sym->expr rather than destroying it.

It happens without the cache too.  When the table already exists,
cmd_free() frees it together with its scope, while the rule commands
that nft_cmd_expand() split out of the same block are still holding
their variable expressions.

scope_release() asserted the count was 1 and then freed the symbol
whatever it was, so on a build with NDEBUG this is a use-after-free
rather than an abort.  The same gap leaks a symbol that is undefined
while a define still refers to it.

Ordering fixes, as in b73298405cda ("rule: fix sym refcount assertion"),
settle one instance at a time.  Make the count own the symbol instead:
scope_release() and variable_expr_destroy() both release through
symbol_put(), so the symbol goes away with its last reference and the
order these run in stops mattering.

Fixes: 12a223ced7f6 ("libnftables: release top level scope")
Reported-by: Kamil Kwiek <kamil.kwiek@gmail.com>
Closes: https://lore.kernel.org/netfilter/20260910190351.38f2aafd@devtravel.lan/
Assisted-by: LLM
Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
---
Both new tests fail without the patch, nft aborts rather than returning, and
pass with it.  The rest of tests/shell is unchanged here: 498 ok, 10 skipped,
and 6 failures that are there with or without the patch.

Built with -DNDEBUG and ASan, both shapes above report a heap-use-after-free
before the patch and are clean after it, as is the whole of tests/shell (no
reports with ASAN_OPTIONS=log_path set).  With LSan, the undefine case leaks
the symbol before and nothing after.

The patch drops the assert from scope_release().  The same check still holds
in scope_free(), which runs once the cache has been released, so I can add it
there in a second patch if you would rather keep it somewhere.

 include/rule.h                                |  1 +
 src/expression.c                              |  3 +-
 src/rule.c                                    |  7 +--
 .../shell/testcases/nft-f/0033define_scope_0  | 22 ++++++++
 .../nft-f/0034define_scope_existing_1         | 25 +++++++++
 .../nft-f/dumps/0033define_scope_0.json-nft   | 53 +++++++++++++++++++
 .../nft-f/dumps/0033define_scope_0.nft        |  5 ++
 .../0034define_scope_existing_1.json-nft      | 18 +++++++
 .../dumps/0034define_scope_existing_1.nft     |  2 +
 9 files changed, 129 insertions(+), 7 deletions(-)
 create mode 100755 tests/shell/testcases/nft-f/0033define_scope_0
 create mode 100755 tests/shell/testcases/nft-f/0034define_scope_existing_1
 create mode 100644 tests/shell/testcases/nft-f/dumps/0033define_scope_0.json-nft
 create mode 100644 tests/shell/testcases/nft-f/dumps/0033define_scope_0.nft
 create mode 100644 tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.json-nft
 create mode 100644 tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.nft

diff --git a/include/rule.h b/include/rule.h
index 77023da2..7b7b7eb1 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -126,6 +126,7 @@ extern struct symbol *symbol_lookup(const struct scope *scope,
 struct symbol *symbol_lookup_fuzzy(const struct scope *scope,
 				   const char *identifier);
 struct symbol *symbol_get(const struct scope *scope, const char *identifier);
+void symbol_put(struct symbol *sym);
 
 enum table_flags {
 	TABLE_F_DORMANT		= (1 << 0),
diff --git a/src/expression.c b/src/expression.c
index 39221ba8..c450746a 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -352,8 +352,7 @@ static void variable_expr_clone(struct expr *new, const struct expr *expr)
 
 static void variable_expr_destroy(struct expr *expr)
 {
-	assert_refcount_safe(expr->sym->refcnt);
-	expr->sym->refcnt--;
+	symbol_put(expr->sym);
 }
 
 static const struct expr_ops variable_expr_ops = {
diff --git a/src/rule.c b/src/rule.c
index 85376e63..0ffe2ca1 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -576,11 +576,8 @@ void scope_release(const struct scope *scope)
 	struct symbol *sym, *next;
 
 	list_for_each_entry_safe(sym, next, &scope->symbols, list) {
-		assert(sym->refcnt == 1);
 		list_del(&sym->list);
-		free_const(sym->identifier);
-		expr_free(sym->expr);
-		free(sym);
+		symbol_put(sym);
 	}
 }
 
@@ -616,7 +613,7 @@ struct symbol *symbol_get(const struct scope *scope, const char *identifier)
 	return sym;
 }
 
-static void symbol_put(struct symbol *sym)
+void symbol_put(struct symbol *sym)
 {
 	assert_refcount_safe(sym->refcnt);
 	if (--sym->refcnt == 0) {
diff --git a/tests/shell/testcases/nft-f/0033define_scope_0 b/tests/shell/testcases/nft-f/0033define_scope_0
new file mode 100755
index 00000000..dcb258a7
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0033define_scope_0
@@ -0,0 +1,22 @@
+#!/bin/bash
+
+# a define in table or chain scope holds a reference to the outer variable it
+# expands to.  The table is kept in the cache once the ruleset is committed, so
+# that reference outlives the release of the top-level scope.
+
+set -e
+
+RULESET="define outer = 111
+
+table inet t {
+	define t_unused = \$outer
+	define t_used = \$outer
+
+	chain c {
+		define c_unused = \$outer
+
+		meta iifgroup { \$t_used } counter
+	}
+}"
+
+$NFT -f - <<< "$RULESET"
diff --git a/tests/shell/testcases/nft-f/0034define_scope_existing_1 b/tests/shell/testcases/nft-f/0034define_scope_existing_1
new file mode 100755
index 00000000..a14a58dd
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0034define_scope_existing_1
@@ -0,0 +1,25 @@
+#!/bin/bash
+
+# Same defines, but the table already exists in the kernel, so table_evaluate()
+# takes no reference on the parsed one.  nft_cmd_expand() has moved the rules
+# into commands of their own, and cmd_free() frees the table and its scope while
+# a rule command still holds the variable expression.  The rule is rejected on
+# purpose, which is what leaves that expression unevaluated.
+#
+# The ruleset must be rejected with 1, not aborted.
+
+set -e
+
+$NFT add table inet t
+
+RULESET="table inet t {
+	define B = 111
+
+	chain c {
+		tcp dport 70000 meta iifgroup \$B counter
+	}
+}"
+
+rc=0
+$NFT -f - <<< "$RULESET" || rc=$?
+[ "$rc" -eq 1 ]
diff --git a/tests/shell/testcases/nft-f/dumps/0033define_scope_0.json-nft b/tests/shell/testcases/nft-f/dumps/0033define_scope_0.json-nft
new file mode 100644
index 00000000..0aa83c42
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0033define_scope_0.json-nft
@@ -0,0 +1,53 @@
+{
+  "nftables": [
+    {
+      "metainfo": {
+        "version": "VERSION",
+        "release_name": "RELEASE_NAME",
+        "json_schema_version": 1
+      }
+    },
+    {
+      "table": {
+        "family": "inet",
+        "name": "t",
+        "handle": 0
+      }
+    },
+    {
+      "chain": {
+        "family": "inet",
+        "table": "t",
+        "name": "c",
+        "handle": 0
+      }
+    },
+    {
+      "rule": {
+        "family": "inet",
+        "table": "t",
+        "chain": "c",
+        "handle": 0,
+        "expr": [
+          {
+            "match": {
+              "op": "==",
+              "left": {
+                "meta": {
+                  "key": "iifgroup"
+                }
+              },
+              "right": 111
+            }
+          },
+          {
+            "counter": {
+              "packets": 0,
+              "bytes": 0
+            }
+          }
+        ]
+      }
+    }
+  ]
+}
diff --git a/tests/shell/testcases/nft-f/dumps/0033define_scope_0.nft b/tests/shell/testcases/nft-f/dumps/0033define_scope_0.nft
new file mode 100644
index 00000000..d206007a
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0033define_scope_0.nft
@@ -0,0 +1,5 @@
+table inet t {
+	chain c {
+		iifgroup 111 counter packets 0 bytes 0
+	}
+}
diff --git a/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.json-nft b/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.json-nft
new file mode 100644
index 00000000..10372b0e
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.json-nft
@@ -0,0 +1,18 @@
+{
+  "nftables": [
+    {
+      "metainfo": {
+        "version": "VERSION",
+        "release_name": "RELEASE_NAME",
+        "json_schema_version": 1
+      }
+    },
+    {
+      "table": {
+        "family": "inet",
+        "name": "t",
+        "handle": 0
+      }
+    }
+  ]
+}
diff --git a/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.nft b/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.nft
new file mode 100644
index 00000000..17838bdf
--- /dev/null
+++ b/tests/shell/testcases/nft-f/dumps/0034define_scope_existing_1.nft
@@ -0,0 +1,2 @@
+table inet t {
+}

base-commit: fcef13a358a0755a0bd980910d47989b064a756d
-- 
2.55.0

Re: [PATCH nft] src: release scope symbols by reference count

[Florian Westphal]

Avinash Duduskar <avinash.duduskar@gmail.com> wrote:
> nft -f aborts when a define in table or chain scope takes its value from
> another variable:
> 
>   define outer = 111

Applied, thanks.