[PATCH nft 0/3] src: make set-merging less zealous

[PATCH nft 0/3] src: make set-merging less zealous

[Florian Westphal]

I got a large corpus of various crashes in the set internals code
tripping over expressions that should not exist, e.g. a range expression
with a symbolic expression.

From initial investigation it looks like to root cause is the same,
we have back-to-back declarations of the same set name, evaluation
is returning errors, but we instist to continue evaluation.

Then, we try to merge set elements and end up merging
such a 'redefined set' with an erroneous one.

This series adds an initial assertion which helped to make
crashes easier to backtrace.

Second patch adds a 'errors' flag to struct set and raises
it once we saw soemthing funky.

Patch 3 also sets/uses this when evaluating the set itself.

Alternative would be to make the lowlevel code more robust
of these kinds of issues, but that might take a while
to fix, also because this oce is partially not able to
indicate errors.

Florian Westphal (3):
  intervals: BUG on prefix expressions without value
  src: do not merge a set with a erroneous one
  evaluate: don't assert if set->data is NULL

 include/rule.h                                  |  2 ++
 src/evaluate.c                                  | 17 +++++++++++++++--
 src/intervals.c                                 |  5 ++++-
 .../nft-f/expr_evaluate_mapping_no_data_assert  |  4 ++++
 .../bogons/nft-f/invalid_range_expr_type_binop  | 12 ++++++++++++
 5 files changed, 37 insertions(+), 3 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
 create mode 100644 tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop

-- 
2.41.0

[PATCH nft 1/3] intervals: BUG on prefix expressions without value

[Florian Westphal]

Its possible to end up with prefix expressions that have
a symbolic expression, e.g.:

table t {
        set s {
                type inet_service
                flags interval
                elements = { 0-1024, 8080-8082, 10000-40000 }
                elements = { 172.16.0.0/16 }
        }

        set s {
                type inet_service
                flags interval
                elements = { 0-1024, 8080-8082, 10000-40000 }
        }
}

Without this change, nft will crash.  We end up in setelem_expr_to_range()
with prefix "/16" for the symbolic expression "172.16.0.0".

We than pass invalid mpz_t pointer into libgmp.

This isn't the right fix (see next patch), but instead of blindly assuming
that the attached expression has a gmp value die with at least some info.

Its possible there are more ways than one to feed such
"symbol-with-prefix" down into the interval code, so also add this
assertion.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/intervals.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/intervals.c b/src/intervals.c
index 85de0199c373..6849b221df2c 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -26,6 +26,9 @@ static void setelem_expr_to_range(struct expr *expr)
 	case EXPR_RANGE:
 		break;
 	case EXPR_PREFIX:
+		if (expr->key->prefix->etype != EXPR_VALUE)
+			BUG("Prefix for unexpected type %d", expr->key->prefix->etype);
+
 		mpz_init(rop);
 		mpz_bitmask(rop, expr->key->len - expr->key->prefix_len);
 		if (expr_basetype(expr)->type == TYPE_STRING)
-- 
2.41.0

[PATCH nft 2/3] src: do not merge a set with a erroneous one

[Florian Westphal]

The included sample causes a crash because we attempt to
range-merge a prefix expression with a symbolic expression.

This happens because the first set is evaluated, the symbol
expr evaluation fails, because the symbolic name cannot be resolved.

nft queues error message
("Could not resolve service: Servname not supported for ai_socktype")
*BUT CONTINUES PROCESSING* of the remaining ruleset.

It then encounters the same set definition again and merges the
new content with the preceeding one.

But the first set structure is dodgy, it still contains the
unresolved symbolic expression.

That then makes nft crash (assert) in the set internals.

There are various different incarnations of this issue, but the low
level set processing code does not allow for any partially transformed
expressions to still remain.

After this nft will exit with an error and will print
all errors that it found during processing of the 'first' set.

After this change, the reproducer from
'intervals: BUG on prefix expressions without value' errors out
as expected without triggering asssertions.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/rule.h                                       |  2 ++
 src/evaluate.c                                       |  4 +++-
 src/intervals.c                                      |  2 +-
 .../bogons/nft-f/invalid_range_expr_type_binop       | 12 ++++++++++++
 4 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop

diff --git a/include/rule.h b/include/rule.h
index 6236d2927c0a..40d7d50a3e4d 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -329,6 +329,7 @@ void rule_stmt_insert_at(struct rule *rule, struct stmt *nstmt,
  * @policy:	set mechanism policy
  * @automerge:	merge adjacents and overlapping elements, if possible
  * @comment:	comment
+ * @errors:	expr evaluation errors seen
  * @desc.size:		count of set elements
  * @desc.field_len:	length of single concatenated fields, bytes
  * @desc.field_count:	count of concatenated fields
@@ -353,6 +354,7 @@ struct set {
 	bool			root;
 	bool			automerge;
 	bool			key_typeof_valid;
+	bool			errors;
 	const char		*comment;
 	struct {
 		uint32_t	size;
diff --git a/src/evaluate.c b/src/evaluate.c
index 61672b0462c4..39296f8226db 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4735,8 +4735,10 @@ static int elems_evaluate(struct eval_ctx *ctx, struct set *set)
 
 		__expr_set_context(&ctx->ectx, set->key->dtype,
 				   set->key->byteorder, set->key->len, 0);
-		if (expr_evaluate(ctx, &set->init) < 0)
+		if (expr_evaluate(ctx, &set->init) < 0) {
+			set->errors = true;
 			return -1;
+		}
 		if (set->init->etype != EXPR_SET)
 			return expr_error(ctx->msgs, set->init, "Set %s: Unexpected initial type %s, missing { }?",
 					  set->handle.set.name, expr_name(set->init));
diff --git a/src/intervals.c b/src/intervals.c
index 6849b221df2c..d0a05cde5018 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -130,7 +130,7 @@ static void set_sort_splice(struct expr *init, struct set *set)
 	set_to_range(init);
 	list_expr_sort(&init->expressions);
 
-	if (!existing_set)
+	if (!existing_set || existing_set->errors)
 		return;
 
 	if (existing_set->init) {
diff --git a/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop b/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
new file mode 100644
index 000000000000..514d6ffe1319
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/invalid_range_expr_type_binop
@@ -0,0 +1,12 @@
+table ip x {
+	map z {
+		type ipv4_addr : ipv4_addr
+		elements = { 1&.141.0.1 - 192.168.0.2}
+	}
+
+	map z {
+		type ipv4_addr : ipv4_addr
+		flags interval
+		elements = { 10.141.0.0, * : 192.168.0.4 }
+	}
+}
-- 
2.41.0

[PATCH nft 3/3] evaluate: don't assert if set->data is NULL

[Florian Westphal]

For the objref map case, set->data is only non-null if set evaluation
completed successfully.

Before:
nft: src/evaluate.c:2115: expr_evaluate_mapping: Assertion `set->data != NULL' failed.

After:
expr_evaluate_mapping_no_data_assert:1:5-5: Error: No such file or directory
map m p {
    ^

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c                                      | 13 ++++++++++++-
 .../nft-f/expr_evaluate_mapping_no_data_assert      |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert

diff --git a/src/evaluate.c b/src/evaluate.c
index 39296f8226db..89b84cd03864 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2104,6 +2104,10 @@ static int expr_evaluate_mapping(struct eval_ctx *ctx, struct expr **expr)
 	if (!set_is_map(set->flags))
 		return set_error(ctx, set, "set is not a map");
 
+	/* set already has more known issues, do not evaluate further */
+	if (set->errors)
+		return -1;
+
 	expr_set_context(&ctx->ectx, set->key->dtype, set->key->len);
 	if (expr_evaluate(ctx, &mapping->left) < 0)
 		return -1;
@@ -5325,12 +5329,17 @@ static int table_evaluate(struct eval_ctx *ctx, struct table *table)
 
 static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 {
+	int err = -1;
+
 	switch (cmd->obj) {
 	case CMD_OBJ_ELEMENTS:
 		return setelem_evaluate(ctx, cmd);
 	case CMD_OBJ_SET:
 		handle_merge(&cmd->set->handle, &cmd->handle);
-		return set_evaluate(ctx, cmd->set);
+		err = set_evaluate(ctx, cmd->set);
+		if (err)
+			cmd->set->errors = true;
+		break;
 	case CMD_OBJ_SETELEMS:
 		return elems_evaluate(ctx, cmd->set);
 	case CMD_OBJ_RULE:
@@ -5356,6 +5365,8 @@ static int cmd_evaluate_add(struct eval_ctx *ctx, struct cmd *cmd)
 	default:
 		BUG("invalid command object type %u\n", cmd->obj);
 	}
+
+	return err;
 }
 
 static void table_del_cache(struct eval_ctx *ctx, struct cmd *cmd)
diff --git a/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert b/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
new file mode 100644
index 000000000000..34d3df61f334
--- /dev/null
+++ b/tests/shell/testcases/bogons/nft-f/expr_evaluate_mapping_no_data_assert
@@ -0,0 +1,4 @@
+map m p {
+	type ipv4_addr : counter
+	elements = { 1.2.3.4 : 1, }
+}
-- 
2.41.0

Re: [PATCH nft 0/3] src: make set-merging less zealous

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> I got a large corpus of various crashes in the set internals code
> tripping over expressions that should not exist, e.g. a range expression
> with a symbolic expression.
> 
> From initial investigation it looks like to root cause is the same,
> we have back-to-back declarations of the same set name, evaluation
> is returning errors, but we instist to continue evaluation.
> 
> Then, we try to merge set elements and end up merging
> such a 'redefined set' with an erroneous one.
> 
> This series adds an initial assertion which helped to make
> crashes easier to backtrace.
> 
> Second patch adds a 'errors' flag to struct set and raises
> it once we saw soemthing funky.
> 
> Patch 3 also sets/uses this when evaluating the set itself.
> 
> Alternative would be to make the lowlevel code more robust
> of these kinds of issues, but that might take a while
> to fix, also because this oce is partially not able to
> indicate errors.

We need to rewrite it, its too picky:

nft add rule t c  ip protocol . th dport { tcp . 22, udp . 1  }
nft add rule t c  ip protocol . th dport { tcp / 22, udp . 1  }
nft add rule t c  ip protocol . th dport { tcp / 22 }

In particular, there is a lot of strange code that causes
this to be evaluated in very different ways.