netlink_delinearize: don't kill dependencies accross statements

nft currently translates
ip protocol tcp meta mark set 1 tcp dport 22
to
mark set 0x00000001 tcp dport 22

This is wrong, the latter form is same as

mark set 0x00000001 ip protocol tcp tcp dport 22

and thats not correct (original rule sets mark for tcp packets only).

We need to clear the dependency stack whenever we see a statement other
than stmt_expr, as these will have side effects (counter, payload
mangling, logging and the like).

Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

3 files changed, 34 insertions, 1 deletions

diff --git a/include/payload.h b/include/payload.h
index a3d23095..8e357aef 100644
--- a/include/payload.h
+++ b/include/payload.h
@@ -35,6 +35,8 @@ struct payload_dep_ctx {
 extern bool payload_is_known(const struct expr *expr);
 extern bool payload_is_stacked(const struct proto_desc *desc,
 			       const struct expr *expr);
+
+void payload_dependency_reset(struct payload_dep_ctx *ctx);
 extern void payload_dependency_store(struct payload_dep_ctx *ctx,
 				     struct stmt *stmt,
 				     enum proto_bases base);
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 4a0b8dcc..a65a97da 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2153,6 +2153,27 @@ static void stmt_payload_postprocess(struct rule_pp_ctx *ctx)
 	expr_postprocess(ctx, &stmt->payload.val);
 }
 
+/*
+ * We can only remove payload dependencies if they occur without
+ * a statment with side effects in between.
+ *
+ * For instance:
+ * 'ip protocol tcp tcp dport 22 counter' is same as
+ * 'tcp dport 22 counter'.
+ *
+ * 'ip protocol tcp counter tcp dport 22' cannot be written as
+ * 'counter tcp dport 22' (that would be counter ip protocol tcp, but
+ * that counts every packet, not just ip/tcp).
+ */
+static void
+rule_maybe_reset_payload_deps(struct payload_dep_ctx *pdctx, enum stmt_types t)
+{
+	if (t == STMT_EXPRESSION)
+		return;
+
+	payload_dependency_reset(pdctx);
+}
+
 static void rule_parse_postprocess(struct netlink_parse_ctx *ctx, struct rule *rule)
 {
 	struct rule_pp_ctx rctx;
@@ -2162,9 +2183,11 @@ static void rule_parse_postprocess(struct netlink_parse_ctx *ctx, struct rule *r
 	proto_ctx_init(&rctx.pctx, rule->handle.family);
 
 	list_for_each_entry_safe(stmt, next, &rule->stmts, list) {
+		enum stmt_types type = stmt->ops->type;
+
 		rctx.stmt = stmt;
 
-		switch (stmt->ops->type) {
+		switch (type) {
 		case STMT_EXPRESSION:
 			stmt_expr_postprocess(&rctx);
 			break;
@@ -2217,7 +2240,10 @@ static void rule_parse_postprocess(struct netlink_parse_ctx *ctx, struct rule *r
 		default:
 			break;
 		}
+
 		rctx.pdctx.prev = rctx.stmt;
+
+		rule_maybe_reset_payload_deps(&rctx.pdctx, type);
 	}
 }
 
diff --git a/src/payload.c b/src/payload.c
index 169954ba..55128fee 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -366,6 +366,11 @@ bool payload_is_stacked(const struct proto_desc *desc, const struct expr *expr)
 	return next && next->base == desc->base;
 }
 
+void payload_dependency_reset(struct payload_dep_ctx *ctx)
+{
+	memset(ctx, 0, sizeof(*ctx));
+}
+
 /**
  * payload_dependency_store - store a possibly redundant protocol match
  *