Patchwork [net-next,RFC] netfilter: ip6_tables: use reasm skb for matching

login
register
mail settings
Submitter Jiri Pirko
Date Oct. 30, 2013, 10:50 a.m.
Message ID <1383130201-6198-1-git-send-email-jiri@resnulli.us>
Download mbox | patch
Permalink /patch/287192/
State Not Applicable
Headers show

Comments

Jiri Pirko - Oct. 30, 2013, 10:50 a.m.
Currently, when ipv6 fragment goes through the netfilter, match
functions are called on them directly. This might cause match function
to fail. So benefit from the fact that nf_defrag_ipv6 constructs
reassembled skb for us and use this reassembled skb for matching.

This patch fixes for example following situation:
On HOSTA do:
ip6tables -I INPUT -p icmpv6 -j DROP
ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT

and on HOSTB you do:
ping6 HOSTA -s2000    (MTU is 1500)

Incoming echo requests will be filtered out on HOSTA. This issue does
not occur with smaller packets than MTU (where fragmentation does not happen).

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/ipv6/netfilter/ip6_tables.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Florian Westphal - Oct. 30, 2013, 1:41 p.m.
Jiri Pirko <jiri@resnulli.us> wrote:
> Currently, when ipv6 fragment goes through the netfilter, match
> functions are called on them directly. This might cause match function
> to fail. So benefit from the fact that nf_defrag_ipv6 constructs
> reassembled skb for us and use this reassembled skb for matching.
> 
> This patch fixes for example following situation:
> On HOSTA do:
> ip6tables -I INPUT -p icmpv6 -j DROP
> ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
> 
> and on HOSTB you do:
> ping6 HOSTA -s2000    (MTU is 1500)
> 
> Incoming echo requests will be filtered out on HOSTA. This issue does
> not occur with smaller packets than MTU (where fragmentation does not happen).

[..]
> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
> index 44400c2..5421beb0 100644
> --- a/net/ipv6/netfilter/ip6_tables.c
> +++ b/net/ipv6/netfilter/ip6_tables.c
> @@ -328,6 +328,7 @@ ip6t_do_table(struct sk_buff *skb,
>  	const struct xt_table_info *private;
>  	struct xt_action_param acpar;
>  	unsigned int addend;
> +	struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb;
>  
>  	/* Initialization */
>  	indev = in ? in->name : nulldevname;
> @@ -363,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb,
>  
>  		IP_NF_ASSERT(e);
>  		acpar.thoff = 0;
> -		if (!ip6_packet_match(skb, indev, outdev, &e->ipv6,
> +		if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6,
>  		    &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) {

[..]

This is a bit backwards, I think.
- We gather frags
- Then we invoke ip6t_do_table for each individual fragment

So basically your patch is equivalent to
for_each_frag( )
  ip6t_do_table(reassembled_skb)

Which makes no sense to me - why traverse the ruleset n times with the same
packet?
--
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
Jiri Pirko - Oct. 30, 2013, 2:13 p.m.
Wed, Oct 30, 2013 at 02:41:00PM CET, fw@strlen.de wrote:
>Jiri Pirko <jiri@resnulli.us> wrote:
>> Currently, when ipv6 fragment goes through the netfilter, match
>> functions are called on them directly. This might cause match function
>> to fail. So benefit from the fact that nf_defrag_ipv6 constructs
>> reassembled skb for us and use this reassembled skb for matching.
>> 
>> This patch fixes for example following situation:
>> On HOSTA do:
>> ip6tables -I INPUT -p icmpv6 -j DROP
>> ip6tables -I INPUT -p icmpv6 -m icmp6 --icmpv6-type 128 -j ACCEPT
>> 
>> and on HOSTB you do:
>> ping6 HOSTA -s2000    (MTU is 1500)
>> 
>> Incoming echo requests will be filtered out on HOSTA. This issue does
>> not occur with smaller packets than MTU (where fragmentation does not happen).
>
>[..]
>> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
>> index 44400c2..5421beb0 100644
>> --- a/net/ipv6/netfilter/ip6_tables.c
>> +++ b/net/ipv6/netfilter/ip6_tables.c
>> @@ -328,6 +328,7 @@ ip6t_do_table(struct sk_buff *skb,
>>  	const struct xt_table_info *private;
>>  	struct xt_action_param acpar;
>>  	unsigned int addend;
>> +	struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb;
>>  
>>  	/* Initialization */
>>  	indev = in ? in->name : nulldevname;
>> @@ -363,7 +364,7 @@ ip6t_do_table(struct sk_buff *skb,
>>  
>>  		IP_NF_ASSERT(e);
>>  		acpar.thoff = 0;
>> -		if (!ip6_packet_match(skb, indev, outdev, &e->ipv6,
>> +		if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6,
>>  		    &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) {
>
>[..]
>
>This is a bit backwards, I think.
>- We gather frags
>- Then we invoke ip6t_do_table for each individual fragment
>
>So basically your patch is equivalent to
>for_each_frag( )
>  ip6t_do_table(reassembled_skb)
>
>Which makes no sense to me - why traverse the ruleset n times with the same
>packet?


Because each fragment need to be pushed through separately.

What different approach would you suggest?

Thanks

Jiri
--
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
Florian Westphal - Oct. 30, 2013, 2:44 p.m.
Jiri Pirko <jiri@resnulli.us> wrote:
> >This is a bit backwards, I think.
> >- We gather frags
> >- Then we invoke ip6t_do_table for each individual fragment
> >
> >So basically your patch is equivalent to
> >for_each_frag( )
> >  ip6t_do_table(reassembled_skb)
> >
> >Which makes no sense to me - why traverse the ruleset n times with the same
> >packet?
> 
> Because each fragment need to be pushed through separately.

Why?  AFAIU we only need to ensure that (in forwarding case) we
send out the original fragments instead of the reassembled packet.

> What different approach would you suggest?

I am sure that current behaviour is intentional, so I'd first like to
understand WHY this was implemented this way.

Also, this would change very long standing behaviour so one might argue that
this is a no-go anyway.

What is the exact problem that this is supposed to solve?
--
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
Jiri Pirko - Nov. 4, 2013, 3:22 p.m.
Wed, Oct 30, 2013 at 03:44:00PM CET, fw@strlen.de wrote:
>Jiri Pirko <jiri@resnulli.us> wrote:
>> >This is a bit backwards, I think.
>> >- We gather frags
>> >- Then we invoke ip6t_do_table for each individual fragment
>> >
>> >So basically your patch is equivalent to
>> >for_each_frag( )
>> >  ip6t_do_table(reassembled_skb)
>> >
>> >Which makes no sense to me - why traverse the ruleset n times with the same
>> >packet?
>> 
>> Because each fragment need to be pushed through separately.
>
>Why?  AFAIU we only need to ensure that (in forwarding case) we
>send out the original fragments instead of the reassembled packet.

I don't knot why, that's the way it is done now. From the top of my head
I can't think of any scenario why it would hurt to push the reassebled
packet instead (and of course send out original fragments at the end of
the way for forwarding)

>
>> What different approach would you suggest?
>
>I am sure that current behaviour is intentional, so I'd first like to
>understand WHY this was implemented this way.
>
>Also, this would change very long standing behaviour so one might argue that
>this is a no-go anyway.

Can you think aof any sane use case this change could possible break?

>
>What is the exact problem that this is supposed to solve?

Look at the patch description. There's an example. The problem is that
fragments are not correctly matched.


--
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

Patch

diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 44400c2..5421beb0 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -328,6 +328,7 @@  ip6t_do_table(struct sk_buff *skb,
 	const struct xt_table_info *private;
 	struct xt_action_param acpar;
 	unsigned int addend;
+	struct sk_buff *reasm = skb->nfct_reasm ? skb->nfct_reasm : skb;
 
 	/* Initialization */
 	indev = in ? in->name : nulldevname;
@@ -363,7 +364,7 @@  ip6t_do_table(struct sk_buff *skb,
 
 		IP_NF_ASSERT(e);
 		acpar.thoff = 0;
-		if (!ip6_packet_match(skb, indev, outdev, &e->ipv6,
+		if (!ip6_packet_match(reasm, indev, outdev, &e->ipv6,
 		    &acpar.thoff, &acpar.fragoff, &acpar.hotdrop)) {
  no_match:
 			e = ip6t_next_entry(e);
@@ -373,7 +374,7 @@  ip6t_do_table(struct sk_buff *skb,
 		xt_ematch_foreach(ematch, e) {
 			acpar.match     = ematch->u.kernel.match;
 			acpar.matchinfo = ematch->data;
-			if (!acpar.match->match(skb, &acpar))
+			if (!acpar.match->match(reasm, &acpar))
 				goto no_match;
 		}