evaluate: refactor function to check the reject family in inet and bridge
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
@@ -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
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
@@ -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
PATCH 3/4 evaluate: no check the network context in reject with tcp reset reason
In reply to Alvaro Neira Ayusonft 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
@@ -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
Pablo Neira Ayuso
maintainerRe: [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 resetIf 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.
Signed-off-by Alvaro Neira Ayuso <[email protected]>--- src/evaluate.c | 80 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 33 deletions(-)src/evaluate.c +0
-11@@ -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 ;
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 resetIf 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.
Signed-off-by Alvaro Neira Ayuso <[email protected]>--- src/evaluate.c | 80 +++++++++++++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 33 deletions(-)src/evaluate.c +0
-15@@ -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
PATCH v3 4/4 evaluate: fix a crash if we check the transport protocol is tcp
In reply to Alvaro Neira AyusoExample:
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
@@ -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
Pablo Neira Ayuso
maintainerRe: [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.
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
Pablo Neira Ayuso
maintainerQuoted 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.
Signed-off-by Alvaro Neira Ayuso <[email protected]>--- src/evaluate.c | 110 ++++++++++++++++++++++++++++++++------------------------ 1 file changed, 63 insertions(+), 47 deletions(-)src/evaluate.c +0
-0@@ -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.