[PATCH 1/3] eval: refactor NAT evaluation functions

[PATCH 1/3] eval: refactor NAT evaluation functions

[Patrick McHardy]

The redir and masq evaluation functions include some useless context
updates and checks.

Refactor the NAT code to have a single instance of address and transport
evaluation functions for simplicity and unified error reporting.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/evaluate.c | 110 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 54 insertions(+), 56 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 43fb968..4cfe749 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1496,32 +1496,61 @@ static int stmt_evaluate_reject(struct eval_ctx *ctx, struct stmt *stmt)
 	return stmt_evaluate_reject_family(ctx, stmt, expr);
 }
 
+static int nat_evaluate_family(struct eval_ctx *ctx, struct stmt *stmt)
+{
+	switch (ctx->pctx.family) {
+	case AF_INET:
+	case AF_INET6:
+		return 0;
+	default:
+		return stmt_error(ctx, stmt,
+				  "NAT is only supported for IPv4/IPv6");
+	}
+}
+
+static int nat_evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt,
+			     struct expr **expr)
+{
+	struct proto_ctx *pctx = &ctx->pctx;
+
+	if (pctx->family == AF_INET)
+		expr_set_context(&ctx->ectx, &ipaddr_type, 4 * BITS_PER_BYTE);
+	else
+		expr_set_context(&ctx->ectx, &ip6addr_type, 16 * BITS_PER_BYTE);
+
+	return expr_evaluate(ctx, expr);
+}
+
+static int nat_evaluate_transport(struct eval_ctx *ctx, struct stmt *stmt,
+				  struct expr **expr)
+{
+	struct proto_ctx *pctx = &ctx->pctx;
+
+	if (pctx->protocol[PROTO_BASE_TRANSPORT_HDR].desc == NULL)
+		return stmt_binary_error(ctx, *expr, stmt,
+					 "transport protocol mapping is only "
+					 "valid after transport protocol match");
+
+	expr_set_context(&ctx->ectx, &inet_service_type, 2 * BITS_PER_BYTE);
+	return expr_evaluate(ctx, expr);
+}
+
 static int stmt_evaluate_nat(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	struct proto_ctx *pctx = &ctx->pctx;
 	int err;
 
+	err = nat_evaluate_family(ctx, stmt);
+	if (err < 0)
+		return err;
+
 	if (stmt->nat.addr != NULL) {
-		if (pctx && (pctx->family == AF_INET))
-			expr_set_context(&ctx->ectx, &ipaddr_type,
-					4 * BITS_PER_BYTE);
-		else
-			expr_set_context(&ctx->ectx, &ip6addr_type,
-					 16 * BITS_PER_BYTE);
-		err = expr_evaluate(ctx, &stmt->nat.addr);
+		err = nat_evaluate_addr(ctx, stmt, &stmt->nat.addr);
 		if (err < 0)
 			return err;
 	}
-
 	if (stmt->nat.proto != NULL) {
-		if (pctx->protocol[PROTO_BASE_TRANSPORT_HDR].desc == NULL)
-			return stmt_binary_error(ctx, stmt->nat.proto, stmt,
-						 "transport protocol mapping is only "
-						 "valid after transport protocol match");
-
-		expr_set_context(&ctx->ectx, &inet_service_type,
-				 2 * BITS_PER_BYTE);
-		err = expr_evaluate(ctx, &stmt->nat.proto);
+		err = nat_evaluate_transport(ctx, stmt, &stmt->nat.proto);
 		if (err < 0)
 			return err;
 	}
@@ -1533,62 +1562,31 @@ static int stmt_evaluate_nat(struct eval_ctx *ctx, struct stmt *stmt)
 static int stmt_evaluate_masq(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	struct proto_ctx *pctx = &ctx->pctx;
+	int err;
 
-	if (!pctx)
-		goto out;
-
-	switch (pctx->family) {
-	case AF_INET:
-		expr_set_context(&ctx->ectx, &ipaddr_type,
-				4 * BITS_PER_BYTE);
-		break;
-	case AF_INET6:
-		expr_set_context(&ctx->ectx, &ip6addr_type,
-				 16 * BITS_PER_BYTE);
-		break;
-	default:
-		return stmt_error(ctx, stmt, "ip and ip6 support only");
-	}
+	err = nat_evaluate_family(ctx, stmt);
+	if (err < 0)
+		return err;
 
-out:
 	stmt->flags |= STMT_F_TERMINAL;
 	return 0;
 }
 
 static int stmt_evaluate_redir(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	int err;
 	struct proto_ctx *pctx = &ctx->pctx;
+	int err;
 
-	if (!pctx)
-		goto out;
-
-	switch (pctx->family) {
-	case AF_INET:
-		expr_set_context(&ctx->ectx, &ipaddr_type,
-				4 * BITS_PER_BYTE);
-		break;
-	case AF_INET6:
-		expr_set_context(&ctx->ectx, &ip6addr_type,
-				 16 * BITS_PER_BYTE);
-		break;
-	default:
-		return stmt_error(ctx, stmt, "ip and ip6 support only");
-	}
+	err = nat_evaluate_family(ctx, stmt);
+	if (err < 0)
+		return err;
 
 	if (stmt->redir.proto != NULL) {
-		if (pctx->protocol[PROTO_BASE_TRANSPORT_HDR].desc == NULL)
-			return stmt_binary_error(ctx, stmt->redir.proto, stmt,
-						 "missing transport protocol match");
-
-		expr_set_context(&ctx->ectx, &inet_service_type,
-				 2 * BITS_PER_BYTE);
-		err = expr_evaluate(ctx, &stmt->redir.proto);
+		err = nat_evaluate_transport(ctx, stmt, &stmt->redir.proto);
 		if (err < 0)
 			return err;
 	}
 
-out:
 	stmt->flags |= STMT_F_TERMINAL;
 	return 0;
 }
-- 
2.1.0

[PATCH 2/3] evaluate: add missing datatype compat checks for statement arguments

[Patrick McHardy]

Add a helper function to evaluate expressions used as arguments for
statements and report datatype mismatches.

Fixes acceptance of mismatching expressions like:

$ nft filter output meta mark set ip daddr
<cmdline>:1:29-36: Error: datatype mismatch: expected packet mark. expression has type IPv4 address
filter output meta mark set ip daddr
              ~~~~~~~~~~~~~~^^^^^^^^

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/evaluate.c | 66 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 4cfe749..0a1422a 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1112,6 +1112,22 @@ static int stmt_evaluate_expr(struct eval_ctx *ctx, struct stmt *stmt)
 	return expr_evaluate(ctx, &stmt->expr);
 }
 
+static int stmt_evaluate_arg(struct eval_ctx *ctx, struct stmt *stmt,
+			     const struct datatype *dtype, unsigned int len,
+			     struct expr **expr)
+{
+	expr_set_context(&ctx->ectx, dtype, len);
+	if (expr_evaluate(ctx, expr) < 0)
+		return -1;
+
+	if (!datatype_equal((*expr)->dtype, dtype))
+		return stmt_binary_error(ctx, *expr, stmt,
+					 "datatype mismatch: expected %s. "
+					 "expression has type %s",
+					 dtype->desc, (*expr)->dtype->desc);
+	return 0;
+}
+
 static int stmt_evaluate_verdict(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	expr_set_context(&ctx->ectx, &verdict_type, 0);
@@ -1133,11 +1149,18 @@ static int stmt_evaluate_verdict(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_meta(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	expr_set_context(&ctx->ectx, stmt->meta.tmpl->dtype,
-			 stmt->meta.tmpl->len);
-	if (expr_evaluate(ctx, &stmt->meta.expr) < 0)
-		return -1;
-	return 0;
+	return stmt_evaluate_arg(ctx, stmt,
+				 stmt->meta.tmpl->dtype,
+				 stmt->meta.tmpl->len,
+				 &stmt->meta.expr);
+}
+
+static int stmt_evaluate_ct(struct eval_ctx *ctx, struct stmt *stmt)
+{
+	return stmt_evaluate_arg(ctx, stmt,
+				 stmt->ct.tmpl->dtype,
+				 stmt->ct.tmpl->len,
+				 &stmt->ct.expr);
 }
 
 static int reject_payload_gen_dependency_tcp(struct eval_ctx *ctx,
@@ -1512,13 +1535,18 @@ static int nat_evaluate_addr(struct eval_ctx *ctx, struct stmt *stmt,
 			     struct expr **expr)
 {
 	struct proto_ctx *pctx = &ctx->pctx;
+	const struct datatype *dtype;
+	unsigned int len;
 
-	if (pctx->family == AF_INET)
-		expr_set_context(&ctx->ectx, &ipaddr_type, 4 * BITS_PER_BYTE);
-	else
-		expr_set_context(&ctx->ectx, &ip6addr_type, 16 * BITS_PER_BYTE);
+	if (pctx->family == AF_INET) {
+		dtype = &ipaddr_type;
+		len   = 4 * BITS_PER_BYTE;
+	} else {
+		dtype = &ip6addr_type;
+		len   = 16 * BITS_PER_BYTE;
+	}
 
-	return expr_evaluate(ctx, expr);
+	return stmt_evaluate_arg(ctx, stmt, dtype, len, expr);
 }
 
 static int nat_evaluate_transport(struct eval_ctx *ctx, struct stmt *stmt,
@@ -1531,8 +1559,9 @@ static int nat_evaluate_transport(struct eval_ctx *ctx, struct stmt *stmt,
 					 "transport protocol mapping is only "
 					 "valid after transport protocol match");
 
-	expr_set_context(&ctx->ectx, &inet_service_type, 2 * BITS_PER_BYTE);
-	return expr_evaluate(ctx, expr);
+	return stmt_evaluate_arg(ctx, stmt,
+				 &inet_service_type, 2 * BITS_PER_BYTE,
+				 expr);
 }
 
 static int stmt_evaluate_nat(struct eval_ctx *ctx, struct stmt *stmt)
@@ -1591,15 +1620,6 @@ static int stmt_evaluate_redir(struct eval_ctx *ctx, struct stmt *stmt)
 	return 0;
 }
 
-static int stmt_evaluate_ct(struct eval_ctx *ctx, struct stmt *stmt)
-{
-	expr_set_context(&ctx->ectx, stmt->ct.tmpl->dtype,
-			 stmt->ct.tmpl->len);
-	if (expr_evaluate(ctx, &stmt->ct.expr) < 0)
-		return -1;
-	return 0;
-}
-
 static int stmt_evaluate_queue(struct eval_ctx *ctx, struct stmt *stmt)
 {
 	if (stmt->queue.queue != NULL) {
@@ -1645,6 +1665,8 @@ int stmt_evaluate(struct eval_ctx *ctx, struct stmt *stmt)
 		return stmt_evaluate_verdict(ctx, stmt);
 	case STMT_META:
 		return stmt_evaluate_meta(ctx, stmt);
+	case STMT_CT:
+		return stmt_evaluate_ct(ctx, stmt);
 	case STMT_LOG:
 		return stmt_evaluate_log(ctx, stmt);
 	case STMT_REJECT:
@@ -1657,8 +1679,6 @@ int stmt_evaluate(struct eval_ctx *ctx, struct stmt *stmt)
 		return stmt_evaluate_redir(ctx, stmt);
 	case STMT_QUEUE:
 		return stmt_evaluate_queue(ctx, stmt);
-	case STMT_CT:
-		return stmt_evaluate_ct(ctx, stmt);
 	default:
 		BUG("unknown statement type %s\n", stmt->ops->name);
 	}
-- 
2.1.0

[PATCH 3/3] netlink_delinearize: fix error handling for invalid registers

[Patrick McHardy]

netlink_delinearize is prepared to deal with malformed expressions from
the kernel that it doesn't understand. However since expressions are now
cloned unconditionally by netlink_get_register(), we crash before such
errors can be detected for invalid inputs.

Fix by only cloning non-NULL expressions.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 src/netlink_delinearize.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index e9a04dd..79d5af6 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -75,7 +75,10 @@ static struct expr *netlink_get_register(struct netlink_parse_ctx *ctx,
 	}
 
 	expr = ctx->registers[reg];
-	return expr_clone(expr);
+	if (expr != NULL)
+		expr = expr_clone(expr);
+
+	return expr;
 }
 
 static void netlink_release_registers(struct netlink_parse_ctx *ctx)
-- 
2.1.0

Re: [PATCH 1/3] eval: refactor NAT evaluation functions

[Pablo Neira Ayuso]

On Sat, Jan 10, 2015 at 02:59:13PM +0000, Patrick McHardy wrote:
> The redir and masq evaluation functions include some useless context
> updates and checks.
> 
> Refactor the NAT code to have a single instance of address and transport
> evaluation functions for simplicity and unified error reporting.

Thanks a lot for revisiting these changes Patrick!