Skip to contentSkip to conversation

evaluate: refactor function to check the reject family in inet and bridge

PATCH Superseded Alvaro Neira Ayuso 2 participants 9 messages Oct 20, 2014 – Oct 21, 2014 netfilter

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/4 evaluate: refactor function to check the reject family in inet and bridge

This patch make a refactorization of the code to check the reject family in inet and bridge. These changes will be used in follow up patches.

---
 src/evaluate.c |  110 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 63 insertions(+), 47 deletions(-)
src/evaluate.c +63 -47
index 1fec120..977df86 100644
@@ -1202,12 +1202,72 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
 	return 0;
 }
 
-static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
+static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt,
+				       struct expr *expr)
+{
+	const struct proto_desc *desc, *base;
+	int protocol;
+
+	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+	if (desc != NULL) {
+		protocol = proto_find_num(base, desc);
+		switch (protocol) {
+		case NFPROTO_IPV4:
+			if (stmt->reject.family == NFPROTO_IPV4)
+				return 0;
+			return stmt_error(ctx, stmt,
+			  "conflicting protocols specified: ip vs ip6");
+		case NFPROTO_IPV6:
+			if (stmt->reject.family == NFPROTO_IPV6)
+				return 0;
+			return stmt_error(ctx, stmt,
+			  "conflicting protocols specified: ip vs ip6");
+		default:
+			BUG("unsupported family");
+		}
+	}
+	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+		return 0;
+	if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+		return -1;
+	return 0;
+}
+
+static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt,
 				       struct expr *expr)
 {
 	const struct proto_desc *desc, *base;
 	int protocol;
 
+	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+	if (desc != NULL) {
+		protocol = proto_find_num(base, desc);
+		switch (protocol) {
+		case __constant_htons(ETH_P_IP):
+			if (NFPROTO_IPV4 == stmt->reject.family)
+				return 0;
+		case __constant_htons(ETH_P_IPV6):
+			if (NFPROTO_IPV6 == stmt->reject.family)
+				return 0;
+			return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+		default:
+			return stmt_error(ctx, stmt,
+					  "cannot reject this ether type");
+		}
+	}
+	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+		return 0;
+	if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+		return -1;
+	return 0;
+}
+
+static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
+				       struct expr *expr)
+{
 	switch (ctx->pctx.family) {
 	case NFPROTO_ARP:
 		return stmt_error(ctx, stmt, "cannot use reject with arp");
@@ -1229,55 +1289,11 @@ static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
 		}
 		break;
 	case NFPROTO_BRIDGE:
-		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-		if (desc != NULL) {
-			protocol = proto_find_num(base, desc);
-			switch (protocol) {
-			case __constant_htons(ETH_P_IP):
-				if (NFPROTO_IPV4 == stmt->reject.family)
-					break;
-			case __constant_htons(ETH_P_IPV6):
-				if (NFPROTO_IPV6 == stmt->reject.family)
-					break;
-				return stmt_error(ctx, stmt,
-				  "conflicting protocols specified: ip vs ip6");
-			default:
-				return stmt_error(ctx, stmt,
-						"cannot reject this ether type");
-			}
-			break;
-		}
-		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
-			break;
-		if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+		if (stmt_evaluate_reject_bridge(ctx, stmt, expr) < 0)
 			return -1;
 		break;
 	case NFPROTO_INET:
-		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-		if (desc != NULL) {
-			protocol = proto_find_num(base, desc);
-			switch (protocol) {
-			case NFPROTO_IPV4:
-				if (stmt->reject.family == NFPROTO_IPV4)
-					break;
-				return stmt_error(ctx, stmt,
-				  "conflicting protocols specified: ip vs ip6");
-				break;
-			case NFPROTO_IPV6:
-				if (stmt->reject.family == NFPROTO_IPV6)
-					break;
-				return stmt_error(ctx, stmt,
-				  "conflicting protocols specified: ip vs ip6");
-			default:
-				BUG("unsupported family");
-			}
-			break;
-		}
-		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
-			break;
-		if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+		if (stmt_evaluate_reject_inet(ctx, stmt, expr) < 0)
 			return -1;
 		break;
 	}
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 2/4 evaluate: accept a reject reason with incorrect network context

nft add rule bridge test-bridge input ether type ip \
reject with icmpv6 type no-route

This rule pass the evaluation step but the network context is incompatible with the reject reason. In that cases, we have to throw an error like "conflicting protocols specified: ip vs ip6"

---
 src/evaluate.c |    2 ++
 1 file changed, 2 insertions(+)
src/evaluate.c +2 -0
index 977df86..20235a8 100644
@@ -1248,6 +1248,8 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt,
 		case __constant_htons(ETH_P_IP):
 			if (NFPROTO_IPV4 == stmt->reject.family)
 				return 0;
+			return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
 		case __constant_htons(ETH_P_IPV6):
 			if (NFPROTO_IPV6 == stmt->reject.family)
 				return 0;
Signature · 1 line
1.7.10.4
#2 lore raw

PATCH 3/4 evaluate: no check the network context in reject with tcp reset reason

In reply to Alvaro Neira Ayuso

nft add rule -nnn bridge test-bridge input \
ip protocol tcp reject with tcp reset

If we use in reject the type tcp reset. We don't need to check if the network context is compatible with the reason. This patch fix that.

---
 src/evaluate.c |   80 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 33 deletions(-)
src/evaluate.c +47 -33
index 20235a8..8b19baf 100644
@@ -1208,24 +1208,31 @@ static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt,
 	const struct proto_desc *desc, *base;
 	int protocol;
 
-	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-	if (desc != NULL) {
-		protocol = proto_find_num(base, desc);
-		switch (protocol) {
-		case NFPROTO_IPV4:
-			if (stmt->reject.family == NFPROTO_IPV4)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
-		case NFPROTO_IPV6:
-			if (stmt->reject.family == NFPROTO_IPV6)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
-		default:
-			BUG("unsupported family");
+	switch (stmt->reject.type) {
+	case NFT_REJECT_TCP_RST:
+		break;
+	case NFT_REJECT_ICMPX_UNREACH:
+	case NFT_REJECT_ICMP_UNREACH:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case NFPROTO_IPV4:
+				if (stmt->reject.family == NFPROTO_IPV4)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			case NFPROTO_IPV6:
+				if (stmt->reject.family == NFPROTO_IPV6)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			default:
+				BUG("unsupported family");
+			}
 		}
+		break;
 	}
 	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
 		return 0;
@@ -1240,25 +1247,32 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt,
 	const struct proto_desc *desc, *base;
 	int protocol;
 
-	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-	if (desc != NULL) {
-		protocol = proto_find_num(base, desc);
-		switch (protocol) {
-		case __constant_htons(ETH_P_IP):
-			if (NFPROTO_IPV4 == stmt->reject.family)
-				return 0;
-			return stmt_error(ctx, stmt,
+	switch (stmt->reject.type) {
+	case NFT_REJECT_TCP_RST:
+		break;
+	case NFT_REJECT_ICMPX_UNREACH:
+	case NFT_REJECT_ICMP_UNREACH:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case __constant_htons(ETH_P_IP):
+				if (NFPROTO_IPV4 == stmt->reject.family)
+					return 0;
+				return stmt_error(ctx, stmt,
 				  "conflicting protocols specified: ip vs ip6");
-		case __constant_htons(ETH_P_IPV6):
-			if (NFPROTO_IPV6 == stmt->reject.family)
-				return 0;
-			return stmt_error(ctx, stmt,
+			case __constant_htons(ETH_P_IPV6):
+				if (NFPROTO_IPV6 == stmt->reject.family)
+					return 0;
+				return stmt_error(ctx, stmt,
 				  "conflicting protocols specified: ip vs ip6");
-		default:
-			return stmt_error(ctx, stmt,
-					  "cannot reject this ether type");
+			default:
+				return stmt_error(ctx, stmt,
+					       "cannot reject this ether type");
+			}
 		}
+		break;
 	}
 	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
 		return 0;
Signature · 1 line
1.7.10.4
#3 lore raw

Re: [nft PATCH 3/4] evaluate: no check the network context in reject with tcp reset reason

Quoted 30 lines · Alvaro Neira Ayuso

nft add rule -nnn bridge test-bridge input \
ip protocol tcp reject with tcp reset

If we use in reject the type tcp reset. We don't need to check if the network context is compatible with the reason. This patch fix that.

---
 src/evaluate.c |   80 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 33 deletions(-)
src/evaluate.c +0 -11
index 20235a8..8b19baf 100644
@@ -1208,24 +1208,31 @@ static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt,
 	const struct proto_desc *desc, *base;
 	int protocol;
 
-	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-	if (desc != NULL) {
-		protocol = proto_find_num(base, desc);
-		switch (protocol) {
-		case NFPROTO_IPV4:
-			if (stmt->reject.family == NFPROTO_IPV4)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
-		case NFPROTO_IPV6:

src/evaluate.c static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt, · line 1214

Quoted 10 lines
src/evaluate.c +4 -6
-			if (stmt->reject.family == NFPROTO_IPV6)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
-		default:
-			BUG("unsupported family");
+	switch (stmt->reject.type) {
+	case NFT_REJECT_TCP_RST:
+		break;
+	case NFT_REJECT_ICMPX_UNREACH:

Do you really need to check layer 3 conflicts with icmpx?

Quoted 24 lines
src/evaluate.c +20 -0
+	case NFT_REJECT_ICMP_UNREACH:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case NFPROTO_IPV4:
+				if (stmt->reject.family == NFPROTO_IPV4)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			case NFPROTO_IPV6:
+				if (stmt->reject.family == NFPROTO_IPV6)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			default:
+				BUG("unsupported family");
+			}
 		}
+		break;
 	}
 	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
 		return 0;
#4 lore raw

Re: [nft PATCH 3/4] evaluate: no check the network context in reject with tcp reset reason

Quoted 35 lines · Pablo Neira Ayuso
Quoted 34 lines · Alvaro Neira Ayuso

nft add rule -nnn bridge test-bridge input \
ip protocol tcp reject with tcp reset

If we use in reject the type tcp reset. We don't need to check if the network context is compatible with the reason. This patch fix that.

---
  src/evaluate.c |   80 +++++++++++++++++++++++++++++++++-----------------------
  1 file changed, 47 insertions(+), 33 deletions(-)
src/evaluate.c +0 -15
index 20235a8..8b19baf 100644
@@ -1208,24 +1208,31 @@ static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt,
  	const struct proto_desc *desc, *base;
  	int protocol;
 
-	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
-	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
-	if (desc != NULL) {
-		protocol = proto_find_num(base, desc);
-		switch (protocol) {
-		case NFPROTO_IPV4:
-			if (stmt->reject.family == NFPROTO_IPV4)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
-		case NFPROTO_IPV6:
-			if (stmt->reject.family == NFPROTO_IPV6)
-				return 0;
-			return stmt_error(ctx, stmt,
-			  "conflicting protocols specified: ip vs ip6");
Quoted 8 lines

src/evaluate.c static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt, · line 1214

Quoted 6 lines
src/evaluate.c +4 -2
-		default:
-			BUG("unsupported family");
+	switch (stmt->reject.type) {
+	case NFT_REJECT_TCP_RST:
+		break;
+	case NFT_REJECT_ICMPX_UNREACH:

Do you really need to check layer 3 conflicts with icmpx?

Ups you're right. In Inet tables we don't need to check it. We only have Ipv4 and Ipv6 traffic. I have been focus in bridge and I have followed the same steps (check the network context) like bridge tables and in Inet is not necessary.

I'm going to fix it. Thanks Pablo

Quoted 17 lines
Quoted 17 lines
src/evaluate.c +16 -0
+	case NFT_REJECT_ICMP_UNREACH:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case NFPROTO_IPV4:
+				if (stmt->reject.family == NFPROTO_IPV4)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			case NFPROTO_IPV6:
+				if (stmt->reject.family == NFPROTO_IPV6)
+					return 0;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
Quoted 8 lines

src/evaluate.c static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt, · line 1238

Quoted 8 lines
src/evaluate.c +4 -0
+			default:
+				BUG("unsupported family");
+			}
  		}
+		break;
  	}
  	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
  		return 0;

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

PATCH v3 4/4 evaluate: fix a crash if we check the transport protocol is tcp

In reply to Alvaro Neira Ayuso

Example:

nft add rule inet filter input meta l4proto udp reject with tcp reset

If we try to check if the transport protocol is tcp, we use the network context. If we don't have this network context, we have a crash.

---
[changes in v3]
* Use the proto_inet_service proto in cases that we don't have network context

 src/evaluate.c |    6 ++++++
 1 file changed, 6 insertions(+)
src/evaluate.c +6 -0
index 8b19baf..fc3b1a2 100644
@@ -1365,6 +1365,12 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
 	if (desc == NULL)
 		return 0;
 
+	/* If we don't have network context in inet or bridge */
+	if (base == NULL &&
+	    (ctx->pctx.family == NFPROTO_INET ||
+	     ctx->pctx.family == NFPROTO_BRIDGE))
+		base = &proto_inet_service;
+
 	protonum = proto_find_num(base, desc);
 	switch (protonum) {
 	case IPPROTO_TCP:
Signature · 1 line
1.7.10.4
#6 lore raw

Re: [nft PATCH 4/4 v3] evaluate: fix a crash if we check the transport protocol is tcp

Quoted 6 lines · Alvaro Neira Ayuso

Example:

nft add rule inet filter input meta l4proto udp reject with tcp reset

If we try to check if the transport protocol is tcp, we use the network context. If we don't have this network context, we have a crash.

I have applied this with minor changes.

Please, select the patch ordering carefully. This is a v3 coming as the 4th patch in a batch.

Remember: Fixes always come in first place. Thanks.

#7 lore raw

Re: [nft PATCH 4/4 v3] evaluate: fix a crash if we check the transport protocol is tcp

Quoted 8 lines · Pablo Neira Ayuso
Quoted 6 lines · Alvaro Neira Ayuso

Example:

nft add rule inet filter input meta l4proto udp reject with tcp reset

If we try to check if the transport protocol is tcp, we use the network context. If we don't have this network context, we have a crash.

Quoted 7 lines

I have applied this with minor changes.

Please, select the patch ordering carefully. This is a v3 coming as the 4th patch in a batch.

Remember: Fixes always come in first place. Thanks.

Catched. Sorry.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw
In reply to Alvaro Neira Ayuso
Quoted 14 lines · Alvaro Neira Ayuso

This patch make a refactorization of the code to check the reject family in inet and bridge. These changes will be used in follow up patches.

---
 src/evaluate.c |  110 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 63 insertions(+), 47 deletions(-)
src/evaluate.c +0 -0
index 1fec120..977df86 100644
@@ -1202,12 +1202,72 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
 	return 0;

src/evaluate.c static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt, · line 1211

Quoted 10 lines
src/evaluate.c +7 -1
 }
 
-static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
+static int stmt_evaluate_reject_inet(struct eval_ctx *ctx, struct stmt *stmt,
+				       struct expr *expr)
+{
+	const struct proto_desc *desc, *base;
+	int protocol;
+
+	base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;

This base pointer is fetched, but you only need this if desc != NULL. Please, while you're refactoring this, it's good if you avoid this.

Quoted 11 lines
src/evaluate.c +2 -0
+	desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+	if (desc != NULL) {
 

From here:

src/evaluate.c +6 -0
+		protocol = proto_find_num(base, desc);
+		switch (protocol) {
+		case NFPROTO_IPV4:
+			if (stmt->reject.family == NFPROTO_IPV4)
+				return 0;
+			return stmt_error(ctx, stmt,

src/evaluate.c static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt, · line 1229

Quoted 10 lines
src/evaluate.c +10 -0
+			  "conflicting protocols specified: ip vs ip6");
+		case NFPROTO_IPV6:
+			if (stmt->reject.family == NFPROTO_IPV6)
+				return 0;
+			return stmt_error(ctx, stmt,
+			  "conflicting protocols specified: ip vs ip6");
+		default:
+			BUG("unsupported family");
+		}
+	}

to there. You can put this code in another function, given that you'll need it again for your patch 3/4.

static int stmt_evaluate_reject_inet_family(...)

{
	int protocol;

	protocol = proto_find_num(base, desc);
	switch (protocol) {
	case NFPROTO_IPV4:
		if (stmt->reject.family != NFPROTO_IPV4)
			return -1;

                break;
	case NFPROTO_IPV6:
		if (stmt->reject.family == NFPROTO_IPV6)
			return -1;

                break;
	default:
		BUG("unsupported family");
	}

        return 0;
}

Then, from stmt_evaluate_reject_inet():

        if (desc != NULL &&
            stmt_evaluate_reject_inet_family(...) < 0)
                return stmt_error(..., "conflicting protocols...");

src/evaluate.c static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt, · line 1238

Quoted 9 lines
src/evaluate.c +8 -0
+	if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+		return 0;
+	if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+		return -1;
+	return 0;
+}
+
+static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt,
 				       struct expr *expr)

Same thing for the bridge code.