From 9230899c6d2be8913646ff1a3b560865c330de7b Mon Sep 17 00:00:00 2001
From: Florian Westphal <fw@strlen.de>
Date: Mon, 1 Feb 2021 22:08:54 +0100
Subject: [PATCH] payload: check icmp dependency before removing previous icmp
 expression

nft is too greedy when removing icmp dependencies.
'icmp code 1 type 2' did remove the type when printing.

Be more careful and check that the icmp type dependency of the
candidate expression (earlier icmp payload expression) has the same
type dependency as the new expression.

Reported-by: Eric Garver <eric@garver.life>
Reported-by: Michael Biebl <biebl@debian.org>
Tested-by: Eric Garver <eric@garver.life>
Fixes: d0f3b9eaab8d77e ("payload: auto-remove simple icmp/icmpv6 dependency expressions")
Signed-off-by: Florian Westphal <fw@strlen.de>
(cherry picked from commit 533565244d88a818d8828ebabd7625e5a8a4c374)
Signed-off-by: Phil Sutter <psutter@redhat.com>
---
 src/payload.c | 63 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/src/payload.c b/src/payload.c
index 48529bcf5c514..a77ca55005509 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -627,6 +627,40 @@ void payload_dependency_release(struct payload_dep_ctx *ctx)
 	ctx->pdep  = NULL;
 }
 
+static uint8_t icmp_dep_to_type(enum icmp_hdr_field_type t)
+{
+	switch (t) {
+	case PROTO_ICMP_ANY:
+		BUG("Invalid map for simple dependency");
+	case PROTO_ICMP_ECHO: return ICMP_ECHO;
+	case PROTO_ICMP6_ECHO: return ICMP6_ECHO_REQUEST;
+	case PROTO_ICMP_MTU: return ICMP_DEST_UNREACH;
+	case PROTO_ICMP_ADDRESS: return ICMP_REDIRECT;
+	case PROTO_ICMP6_MTU: return ICMP6_PACKET_TOO_BIG;
+	case PROTO_ICMP6_MGMQ: return MLD_LISTENER_QUERY;
+	case PROTO_ICMP6_PPTR: return ICMP6_PARAM_PROB;
+	}
+
+	BUG("Missing icmp type mapping");
+}
+
+static bool payload_may_dependency_kill_icmp(struct payload_dep_ctx *ctx, struct expr *expr)
+{
+	const struct expr *dep = ctx->pdep->expr;
+	uint8_t icmp_type;
+
+	icmp_type = expr->payload.tmpl->icmp_dep;
+	if (icmp_type == PROTO_ICMP_ANY)
+		return false;
+
+	if (dep->left->payload.desc != expr->payload.desc)
+		return false;
+
+	icmp_type = icmp_dep_to_type(expr->payload.tmpl->icmp_dep);
+
+	return ctx->icmp_type == icmp_type;
+}
+
 static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 					unsigned int family, struct expr *expr)
 {
@@ -661,6 +695,14 @@ static bool payload_may_dependency_kill(struct payload_dep_ctx *ctx,
 		break;
 	}
 
+	if (expr->payload.base == PROTO_BASE_TRANSPORT_HDR &&
+	    dep->left->payload.base == PROTO_BASE_TRANSPORT_HDR) {
+		if (dep->left->payload.desc == &proto_icmp)
+			return payload_may_dependency_kill_icmp(ctx, expr);
+		if (dep->left->payload.desc == &proto_icmp6)
+			return payload_may_dependency_kill_icmp(ctx, expr);
+	}
+
 	return true;
 }
 
@@ -680,10 +722,6 @@ void payload_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 	if (payload_dependency_exists(ctx, expr->payload.base) &&
 	    payload_may_dependency_kill(ctx, family, expr))
 		payload_dependency_release(ctx);
-	else if (ctx->icmp_type && ctx->pdep) {
-		fprintf(stderr, "Did not kill \n");
-		payload_dependency_release(ctx);
-	}
 }
 
 void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
@@ -707,23 +745,6 @@ void exthdr_dependency_kill(struct payload_dep_ctx *ctx, struct expr *expr,
 	}
 }
 
-static uint8_t icmp_dep_to_type(enum icmp_hdr_field_type t)
-{
-	switch (t) {
-	case PROTO_ICMP_ANY:
-		BUG("Invalid map for simple dependency");
-	case PROTO_ICMP_ECHO: return ICMP_ECHO;
-	case PROTO_ICMP6_ECHO: return ICMP6_ECHO_REQUEST;
-	case PROTO_ICMP_MTU: return ICMP_DEST_UNREACH;
-	case PROTO_ICMP_ADDRESS: return ICMP_REDIRECT;
-	case PROTO_ICMP6_MTU: return ICMP6_PACKET_TOO_BIG;
-	case PROTO_ICMP6_MGMQ: return MLD_LISTENER_QUERY;
-	case PROTO_ICMP6_PPTR: return ICMP6_PARAM_PROB;
-	}
-
-	BUG("Missing icmp type mapping");
-}
-
 /**
  * payload_expr_complete - fill in type information of a raw payload expr
  *
-- 
2.31.1