| Message ID | 1519712714-12756-1-git-send-email-amsalam20@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] nftables: Fixing Bug 1219 - handle rt0 and rt2 properly | expand |
Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > Type 0 and 2 of the IPv6 Routing extension header are not handled > properly by exthdr_init_raw() in src/exthdr.c > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > differentiate between rt, rt0, and rt2. > > This patch should fix the bug. We tested the patch against the > same configuration reported in the bug and the output is as > shown below. > > table ip6 filter { > chain input { > type filter hook input priority 0; policy accept; > rt0 addr[1] a::2 > } > } I think this patch should be solved in userspace only. > + if (desc != NULL && desc->proto_key >= 0) { > + switch (desc->proto_key) { > + case 0: > + expr->exthdr.op = NFT_EXTHDR_OP_RT0; In particular, there is no need to store this in the kernel. I agree that doing it this way is easier, but still ... Here is a minimal patch. I write 'minimal' because it doesn't handle dependency correctly, but it should add correct rt0/rt2 (type was 0...) and also decode rt2 vs. hbh correctly. When asking for 'rt2' nft should insert a dependency for routing type 2, and similar, when asking for old rt0 it should add a check for rt type 0. Probably can be left as a future enhancement though. diff --git a/src/exthdr.c b/src/exthdr.c --- a/src/exthdr.c +++ b/src/exthdr.c @@ -156,13 +156,24 @@ void exthdr_init_raw(struct expr *expr, uint8_t type, if (expr->exthdr.desc == NULL) goto out; - +again: for (i = 0; i < array_size(expr->exthdr.desc->templates); i++) { tmpl = &expr->exthdr.desc->templates[i]; if (tmpl->offset == offset && tmpl->len == len) goto out; } + if (type == IPPROTO_ROUTING) { + /* rt2/rt0 descriptions extend rt one */ + if (expr->exthdr.desc == &exthdr_rt) { + expr->exthdr.desc = &exthdr_rt2; + goto again; + } else if (expr->exthdr.desc == &exthdr_rt2) { + expr->exthdr.desc = &exthdr_rt0; + goto again; + } + } + tmpl = &exthdr_unknown_template; out: expr->exthdr.tmpl = tmpl; @@ -236,6 +247,8 @@ const struct exthdr_desc exthdr_hbh = { */ const struct exthdr_desc exthdr_rt2 = { + .name = "rt2", + .type = IPPROTO_ROUTING, .templates = { [RT2HDR_RESERVED] = {}, [RT2HDR_ADDR] = {}, @@ -246,6 +259,8 @@ const struct exthdr_desc exthdr_rt2 = { HDR_TEMPLATE(__name, __dtype, struct ip6_rthdr0, __member) const struct exthdr_desc exthdr_rt0 = { + .name = "rt0", + .type = IPPROTO_ROUTING, .templates = { [RT0HDR_RESERVED] = RT0_FIELD("reserved", ip6r0_reserved, &integer_type), [RT0HDR_ADDR_1] = RT0_FIELD("addr[1]", ip6r0_addr[0], &ip6addr_type), @@ -260,13 +275,6 @@ const struct exthdr_desc exthdr_rt0 = { const struct exthdr_desc exthdr_rt = { .name = "rt", .type = IPPROTO_ROUTING, -#if 0 - .protocol_key = RTHDR_TYPE, - .protocols = { - [0] = &exthdr_rt0, - [2] = &exthdr_rt2, - }, -#endif .templates = { [RTHDR_NEXTHDR] = RT_FIELD("nexthdr", ip6r_nxt, &inet_protocol_type), [RTHDR_HDRLENGTH] = RT_FIELD("hdrlength", ip6r_len, &integer_type), -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Florian, On Tue, 27 Feb 2018 17:33:53 +0100 Florian Westphal <fw@strlen.de> wrote: > Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > > Type 0 and 2 of the IPv6 Routing extension header are not handled > > properly by exthdr_init_raw() in src/exthdr.c > > > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > > differentiate between rt, rt0, and rt2. > > > > This patch should fix the bug. We tested the patch against the > > same configuration reported in the bug and the output is as > > shown below. > > > > table ip6 filter { > > chain input { > > type filter hook input priority 0; policy accept; > > rt0 addr[1] a::2 > > } > > } > > I think this patch should be solved in userspace only. > > > > + if (desc != NULL && desc->proto_key >= 0) { > > + switch (desc->proto_key) { > > + case 0: > > + expr->exthdr.op = NFT_EXTHDR_OP_RT0; > > In particular, there is no need to store this in the kernel. > I agree that doing it this way is easier, but still ... > > Here is a minimal patch. > > I write 'minimal' because it doesn't handle dependency correctly, > but it should add correct rt0/rt2 (type was 0...) and also decode > rt2 vs. hbh correctly. > I think Routing type 0, 2 and 4 (SRH) shouldn't be implemented as an extension to General IPv6 routing header. I agree they share some fields, but Routing header is just a template. In real world, we use either routing type 0, 2, or 4. I think, If I, as a user of nftables, want to write an nft rule for routing type_0, I would prefer to write as below $ nft add rule ip6 filter input rt0 nexthdr 6 rt0 seg-left 2 rt0 hdrlength rt0 addr [1]A::2 Instead, using the current implmentation, I will need to write half of the rule using rt and the second half with rt0. something like $ nft add rule ip6 filter input rt nexthdr 6 rt seg-left 2 rt hdrlength rt0 addr [1]A::2 If you agree, I think we should extend the templates of exthdr_rt0 and exthdr_rt2. I can send another patch also for routing type 4. > When asking for 'rt2' nft should insert a dependency for > routing type 2, and similar, when asking for old rt0 it should add > a check for rt type 0. > > Probably can be left as a future enhancement though. > > diff --git a/src/exthdr.c b/src/exthdr.c > --- a/src/exthdr.c > +++ b/src/exthdr.c > @@ -156,13 +156,24 @@ void exthdr_init_raw(struct expr *expr, uint8_t type, > > if (expr->exthdr.desc == NULL) > goto out; > - > +again: > for (i = 0; i < array_size(expr->exthdr.desc->templates); i++) { > tmpl = &expr->exthdr.desc->templates[i]; > if (tmpl->offset == offset && tmpl->len == len) > goto out; > } > > + if (type == IPPROTO_ROUTING) { > + /* rt2/rt0 descriptions extend rt one */ > + if (expr->exthdr.desc == &exthdr_rt) { > + expr->exthdr.desc = &exthdr_rt2; > + goto again; > + } else if (expr->exthdr.desc == &exthdr_rt2) { > + expr->exthdr.desc = &exthdr_rt0; > + goto again; > + } > + } > + > tmpl = &exthdr_unknown_template; > out: > expr->exthdr.tmpl = tmpl; > @@ -236,6 +247,8 @@ const struct exthdr_desc exthdr_hbh = { > */ > > const struct exthdr_desc exthdr_rt2 = { > + .name = "rt2", > + .type = IPPROTO_ROUTING, > .templates = { > [RT2HDR_RESERVED] = {}, > [RT2HDR_ADDR] = {}, > @@ -246,6 +259,8 @@ const struct exthdr_desc exthdr_rt2 = { > HDR_TEMPLATE(__name, __dtype, struct ip6_rthdr0, __member) > > const struct exthdr_desc exthdr_rt0 = { > + .name = "rt0", > + .type = IPPROTO_ROUTING, > .templates = { > [RT0HDR_RESERVED] = RT0_FIELD("reserved", ip6r0_reserved, &integer_type), > [RT0HDR_ADDR_1] = RT0_FIELD("addr[1]", ip6r0_addr[0], &ip6addr_type), > @@ -260,13 +275,6 @@ const struct exthdr_desc exthdr_rt0 = { > const struct exthdr_desc exthdr_rt = { > .name = "rt", > .type = IPPROTO_ROUTING, > -#if 0 > - .protocol_key = RTHDR_TYPE, > - .protocols = { > - [0] = &exthdr_rt0, > - [2] = &exthdr_rt2, > - }, > -#endif > .templates = { > [RTHDR_NEXTHDR] = RT_FIELD("nexthdr", ip6r_nxt, &inet_protocol_type), > [RTHDR_HDRLENGTH] = RT_FIELD("hdrlength", ip6r_len, &integer_type), Thanks, Ahmed
Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > > Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > > > Type 0 and 2 of the IPv6 Routing extension header are not handled > > > properly by exthdr_init_raw() in src/exthdr.c > > > > > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > > > differentiate between rt, rt0, and rt2. > > > > > > This patch should fix the bug. We tested the patch against the > > > same configuration reported in the bug and the output is as > > > shown below. > > > > > > table ip6 filter { > > > chain input { > > > type filter hook input priority 0; policy accept; > > > rt0 addr[1] a::2 > > > } > > > } > > > > I think this patch should be solved in userspace only. > > > > > > > + if (desc != NULL && desc->proto_key >= 0) { > > > + switch (desc->proto_key) { > > > + case 0: > > > + expr->exthdr.op = NFT_EXTHDR_OP_RT0; > > > > In particular, there is no need to store this in the kernel. > > I agree that doing it this way is easier, but still ... > > > > Here is a minimal patch. > > > > I write 'minimal' because it doesn't handle dependency correctly, > > but it should add correct rt0/rt2 (type was 0...) and also decode > > rt2 vs. hbh correctly. > > > > I think Routing type 0, 2 and 4 (SRH) shouldn't be implemented as > an extension to General IPv6 routing header. > > I agree they share some fields, but Routing header is just a template. > In real world, we use either routing type 0, 2, or 4. OK. > I think, If I, as a user of nftables, want to write an nft rule for routing type_0, > I would prefer to write as below > > $ nft add rule ip6 filter input rt0 nexthdr 6 rt0 seg-left 2 rt0 hdrlength rt0 addr [1]A::2 This should insert a 'rt0 type 0' check too, right (as a dependency to not match other route header type). > Instead, using the current implmentation, I will need to write half of the rule using rt > and the second half with rt0. something like > > $ nft add rule ip6 filter input rt nexthdr 6 rt seg-left 2 rt hdrlength rt0 addr [1]A::2 Right, thats looks ugly indeed. > If you agree, I think we should extend the templates of exthdr_rt0 and exthdr_rt2. > I can send another patch also for routing type 4. Would be good, thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 27 Feb 2018 18:48:20 +0100 Florian Westphal <fw@strlen.de> wrote: > Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > > > Ahmed Abdelsalam <amsalam20@gmail.com> wrote: > > > > Type 0 and 2 of the IPv6 Routing extension header are not handled > > > > properly by exthdr_init_raw() in src/exthdr.c > > > > > > > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > > > > differentiate between rt, rt0, and rt2. > > > > > > > > This patch should fix the bug. We tested the patch against the > > > > same configuration reported in the bug and the output is as > > > > shown below. > > > > > > > > table ip6 filter { > > > > chain input { > > > > type filter hook input priority 0; policy accept; > > > > rt0 addr[1] a::2 > > > > } > > > > } > This should insert a 'rt0 type 0' check too, right (as a dependency to > not match other route header type). > Yes, we should implement this dependency. Do you think of any proposal for this dependency ? > > Instead, using the current implmentation, I will need to write half of the rule using rt > > and the second half with rt0. something like > > > > $ nft add rule ip6 filter input rt nexthdr 6 rt seg-left 2 rt hdrlength rt0 addr [1]A::2 > > Right, thats looks ugly indeed. > > > If you agree, I think we should extend the templates of exthdr_rt0 and exthdr_rt2. > > > I can send another patch also for routing type 4. > > Would be good, thanks.
On Tue, Feb 27, 2018 at 07:25:14AM +0100, Ahmed Abdelsalam wrote: > Type 0 and 2 of the IPv6 Routing extension header are not handled > properly by exthdr_init_raw() in src/exthdr.c > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > differentiate between rt, rt0, and rt2. > > This patch should fix the bug. We tested the patch against the > same configuration reported in the bug and the output is as > shown below. > > table ip6 filter { > chain input { > type filter hook input priority 0; policy accept; > rt0 addr[1] a::2 > } > } Applied, thanks Ahmed. Would you also update tests/py to cover this? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 11 Mar 2018 23:00:41 +0100 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Feb 27, 2018 at 07:25:14AM +0100, Ahmed Abdelsalam wrote: > > Type 0 and 2 of the IPv6 Routing extension header are not handled > > properly by exthdr_init_raw() in src/exthdr.c > > > > In order to fix the bug, we extended the "enum nft_exthdr_op" to > > differentiate between rt, rt0, and rt2. > > > > This patch should fix the bug. We tested the patch against the > > same configuration reported in the bug and the output is as > > shown below. > > > > table ip6 filter { > > chain input { > > type filter hook input priority 0; policy accept; > > rt0 addr[1] a::2 > > } > > } > > Applied, thanks Ahmed. > > Would you also update tests/py to cover this? Thanks. Thanks Pablo! I will send you a patch with the required tests.
diff --git a/include/exthdr.h b/include/exthdr.h index 97ccc38..06bf628 100644 --- a/include/exthdr.h +++ b/include/exthdr.h @@ -14,6 +14,7 @@ struct exthdr_desc { const char *name; uint8_t type; + int proto_key; struct proto_hdr_template templates[10]; }; diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h index 2efbf97..baa7caf 100644 --- a/include/linux/netfilter/nf_tables.h +++ b/include/linux/netfilter/nf_tables.h @@ -721,6 +721,9 @@ enum nft_exthdr_flags { enum nft_exthdr_op { NFT_EXTHDR_OP_IPV6, NFT_EXTHDR_OP_TCPOPT, + NFT_EXTHDR_OP_RT0, + NFT_EXTHDR_OP_RT2, + NFT_EXTHDR_OP_RT4, __NFT_EXTHDR_OP_MAX }; #define NFT_EXTHDR_OP_MAX (__NFT_EXTHDR_OP_MAX - 1) diff --git a/src/exthdr.c b/src/exthdr.c index f5c20ac..3757f33 100644 --- a/src/exthdr.c +++ b/src/exthdr.c @@ -93,6 +93,16 @@ struct expr *exthdr_expr_alloc(const struct location *loc, BYTEORDER_BIG_ENDIAN, tmpl->len); expr->exthdr.desc = desc; expr->exthdr.tmpl = tmpl; + if (desc != NULL && desc->proto_key >= 0) { + switch (desc->proto_key) { + case 0: + expr->exthdr.op = NFT_EXTHDR_OP_RT0; + break; + case 2: + expr->exthdr.op = NFT_EXTHDR_OP_RT2; + break; + } + } return expr; } @@ -151,7 +161,11 @@ void exthdr_init_raw(struct expr *expr, uint8_t type, expr->exthdr.offset = offset; expr->exthdr.desc = NULL; - if (type < array_size(exthdr_protocols)) + if (op == NFT_EXTHDR_OP_RT0) + expr->exthdr.desc = &exthdr_rt0; + else if (op == NFT_EXTHDR_OP_RT2) + expr->exthdr.desc = &exthdr_rt2; + else if (type < array_size(exthdr_protocols)) expr->exthdr.desc = exthdr_protocols[type]; if (expr->exthdr.desc == NULL) @@ -236,6 +250,9 @@ const struct exthdr_desc exthdr_hbh = { */ const struct exthdr_desc exthdr_rt2 = { + .name = "rt2", + .type = IPPROTO_ROUTING, + .proto_key = 2, .templates = { [RT2HDR_RESERVED] = {}, [RT2HDR_ADDR] = {}, @@ -246,6 +263,9 @@ const struct exthdr_desc exthdr_rt2 = { HDR_TEMPLATE(__name, __dtype, struct ip6_rthdr0, __member) const struct exthdr_desc exthdr_rt0 = { + .name = "rt0", + .type = IPPROTO_ROUTING, + .proto_key = 0, .templates = { [RT0HDR_RESERVED] = RT0_FIELD("reserved", ip6r0_reserved, &integer_type), [RT0HDR_ADDR_1] = RT0_FIELD("addr[1]", ip6r0_addr[0], &ip6addr_type), @@ -260,6 +280,7 @@ const struct exthdr_desc exthdr_rt0 = { const struct exthdr_desc exthdr_rt = { .name = "rt", .type = IPPROTO_ROUTING, + .proto_key = -1, #if 0 .protocol_key = RTHDR_TYPE, .protocols = {
Type 0 and 2 of the IPv6 Routing extension header are not handled properly by exthdr_init_raw() in src/exthdr.c In order to fix the bug, we extended the "enum nft_exthdr_op" to differentiate between rt, rt0, and rt2. This patch should fix the bug. We tested the patch against the same configuration reported in the bug and the output is as shown below. table ip6 filter { chain input { type filter hook input priority 0; policy accept; rt0 addr[1] a::2 } } Signed-off-by: Ahmed Abdelsalam <amsalam20@gmail.com> --- include/exthdr.h | 1 + include/linux/netfilter/nf_tables.h | 3 +++ src/exthdr.c | 23 ++++++++++++++++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)