[PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[Ville Nuorvala]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: 0006-IPv6-Fix-infinite-loop-if-no-matching-IPv6-tunnel-found.txt --]
[-- Type: text/plain, Size: 1008 bytes --]

>From d9ecea2b1d88bc8702f70fbbca7cde2afb8312ee Mon Sep 17 00:00:00 2001
From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Thu, 2 Nov 2006 13:07:35 +0200
Subject: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

If no matching IPv6 tunnel was found ip6ip6_rcv() would cause
ip6_input_finish() to resubmit the same skb to ip6ip6_rcv().

Many thanks to Tero Kauppinen at Ericsson for reporting this issue.

Signed-off-by: Ville Nuorvala <vnuorval@tcs.hut.fi>
---
 net/ipv6/ip6_tunnel.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 4f64ed7..603ed0d 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -565,11 +565,11 @@ ip6ip6_rcv(struct sk_buff *skb)
 		return 0;
 	}
 	read_unlock(&ip6ip6_lock);
-	return 1;
-
+	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
+		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
 discard:
 	kfree_skb(skb);
-	return 0;
+	return -1;
 }
 
 struct ipv6_tel_txoption {
-- 
1.4.3.2

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4549D8E7.1040409@tcs.hut.fi> (at Thu, 02 Nov 2006 13:39:19 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:

>  	read_unlock(&ip6ip6_lock);
> -	return 1;
> -
> +	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
> +		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
>  discard:

I'd argue this.  We probably should not send back any ICMPv6 packets 
to the original sender in this case to avoid DoS.

--yoshfuji.

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4549EFA7.50004@tcs.hut.fi> (at Thu, 02 Nov 2006 15:16:23 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:

> On 11/02/06 14:59, YOSHIFUJI Hideaki wrote:
> > In article <4549D8E7.1040409@tcs.hut.fi> (at Thu, 02 Nov 2006 13:39:19 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
> > 
> >>  	read_unlock(&ip6ip6_lock);
> >> -	return 1;
> >> -
> >> +	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
> >> +		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
> >>  discard:
> > 
> > I'd argue this.  We probably should not send back any ICMPv6 packets 
> > to the original sender in this case to avoid DoS.
> 
> Sorry, I don't follow you. I don't see the DoS scenario here (after we
> apply the patch, that is ;-).

Well, leaving aside whether sending icmpv6 is good thing (*),
the code for sending icmpv6 was moved from ip6_tunnel.c
to tunnel6.c by commit-id 50fba2aa7cefa6b0e1768cb350c9e69042320c03
by Herbert.

The ip6_tunnel.c change that Herbert made does not seem consistent
with ipip.c change.  To fix your issue the appropriate change is just
fall through to discard section, as we're doing for ipip.c.

Please do not re-add sending icmpv6 logic here.
If you DO think it is appropriate, please fix other codes
such as ipip.c, and your comment.

*: As far as I remember, *BSD*s do not send icmpv6
   in this case.
   Anyway, I'd talk to people at ietf next week.

--yoshfuji

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[Ville Nuorvala]

On 11/02/06 16:18, YOSHIFUJI Hideaki wrote:
> In article <4549EFA7.50004@tcs.hut.fi> (at Thu, 02 Nov 2006 15:16:23 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
> 
>> On 11/02/06 14:59, YOSHIFUJI Hideaki wrote:
>>> In article <4549D8E7.1040409@tcs.hut.fi> (at Thu, 02 Nov 2006 13:39:19 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
>>>
>>>>  	read_unlock(&ip6ip6_lock);
>>>> -	return 1;
>>>> -
>>>> +	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
>>>> +		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
>>>>  discard:
>>> I'd argue this.  We probably should not send back any ICMPv6 packets 
>>> to the original sender in this case to avoid DoS.
>> Sorry, I don't follow you. I don't see the DoS scenario here (after we
>> apply the patch, that is ;-).
> 
> Well, leaving aside whether sending icmpv6 is good thing (*),
> the code for sending icmpv6 was moved from ip6_tunnel.c
> to tunnel6.c by commit-id 50fba2aa7cefa6b0e1768cb350c9e69042320c03
> by Herbert.
> 
> The ip6_tunnel.c change that Herbert made does not seem consistent
> with ipip.c change.  To fix your issue the appropriate change is just
> fall through to discard section, as we're doing for ipip.c.
> 
> Please do not re-add sending icmpv6 logic here.
> If you DO think it is appropriate, please fix other codes
> such as ipip.c, and your comment.

Ok, I'll resubmit a patch doesn't send an ICMPv6 error message.

Regards,
Ville

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[David Miller]

From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Thu, 02 Nov 2006 16:22:07 +0200

> Ok, I'll resubmit a patch doesn't send an ICMPv6 error message.

Is this coming soon?  I'd like to integrate this patch set into
net-2.6.20 if I can.

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[Ville Nuorvala]

David Miller wrote:
> From: Ville Nuorvala <vnuorval@tcs.hut.fi>
> Date: Thu, 02 Nov 2006 16:22:07 +0200
> 
>> Ok, I'll resubmit a patch doesn't send an ICMPv6 error message.
> 
> Is this coming soon?  I'd like to integrate this patch set into
> net-2.6.20 if I can.

No, it was a false alarm as Herbert's patch already addressed the problem.

Regards,
Ville

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[David Miller]

From: Ville Nuorvala <vnuorval@tcs.hut.fi>
Date: Tue, 14 Nov 2006 15:16:36 +0200

> David Miller wrote:
> > From: Ville Nuorvala <vnuorval@tcs.hut.fi>
> > Date: Thu, 02 Nov 2006 16:22:07 +0200
> > 
> >> Ok, I'll resubmit a patch doesn't send an ICMPv6 error message.
> > 
> > Is this coming soon?  I'd like to integrate this patch set into
> > net-2.6.20 if I can.
> 
> No, it was a false alarm as Herbert's patch already addressed the problem.

So I applied patches 1 to 5 to net-2.6.20, and did not apply patch 6.

Thanks!

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[Ville Nuorvala]

YOSHIFUJI Hideaki wrote:
> In article <4549EFA7.50004@tcs.hut.fi> (at Thu, 02 Nov 2006 15:16:23 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
> 
>> On 11/02/06 14:59, YOSHIFUJI Hideaki wrote:
>>> In article <4549D8E7.1040409@tcs.hut.fi> (at Thu, 02 Nov 2006 13:39:19 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
>>>
>>>>  	read_unlock(&ip6ip6_lock);
>>>> -	return 1;
>>>> -
>>>> +	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
>>>> +		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
>>>>  discard:
>>> I'd argue this.  We probably should not send back any ICMPv6 packets 
>>> to the original sender in this case to avoid DoS.
>> Sorry, I don't follow you. I don't see the DoS scenario here (after we
>> apply the patch, that is ;-).
> 
> Well, leaving aside whether sending icmpv6 is good thing (*),
> the code for sending icmpv6 was moved from ip6_tunnel.c
> to tunnel6.c by commit-id 50fba2aa7cefa6b0e1768cb350c9e69042320c03
> by Herbert.
> 
> The ip6_tunnel.c change that Herbert made does not seem consistent
> with ipip.c change.  To fix your issue the appropriate change is just
> fall through to discard section, as we're doing for ipip.c.

Ah, I hadn't noticed Herbert's patch. It actually appears to fix the
problem I was trying to fix here. AFAIK Tero experienced the infinite
loop on a 2.6.16 kernel.

Regards,
Ville

Re: [PATCH 6/6] IPv6: Fix infinite loop if no matching IPv6 tunnel found

[Tero Kauppinen (JO/LMF)]

Ville Nuorvala wrote:
> YOSHIFUJI Hideaki wrote:
>> In article <4549EFA7.50004@tcs.hut.fi> (at Thu, 02 Nov 2006 15:16:23 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
>>
>>> On 11/02/06 14:59, YOSHIFUJI Hideaki wrote:
>>>> In article <4549D8E7.1040409@tcs.hut.fi> (at Thu, 02 Nov 2006 13:39:19 +0200), Ville Nuorvala <vnuorval@tcs.hut.fi> says:
>>>>
>>>>>  	read_unlock(&ip6ip6_lock);
>>>>> -	return 1;
>>>>> -
>>>>> +	icmpv6_send(skb, ICMPV6_DEST_UNREACH,
>>>>> +		    ICMPV6_ADDR_UNREACH, 0, skb->dev);
>>>>>  discard:
>>>> I'd argue this.  We probably should not send back any ICMPv6 packets 
>>>> to the original sender in this case to avoid DoS.
>>> Sorry, I don't follow you. I don't see the DoS scenario here (after we
>>> apply the patch, that is ;-).
>> Well, leaving aside whether sending icmpv6 is good thing (*),
>> the code for sending icmpv6 was moved from ip6_tunnel.c
>> to tunnel6.c by commit-id 50fba2aa7cefa6b0e1768cb350c9e69042320c03
>> by Herbert.
>>
>> The ip6_tunnel.c change that Herbert made does not seem consistent
>> with ipip.c change.  To fix your issue the appropriate change is just
>> fall through to discard section, as we're doing for ipip.c.
> 
> Ah, I hadn't noticed Herbert's patch. It actually appears to fix the
> problem I was trying to fix here. AFAIK Tero experienced the infinite
> loop on a 2.6.16 kernel.

Correct, it was a 2.6.16.29 kernel patched with MIPL 2.0.2. The problem 
was obviously not whether an ICMP error was sent or not but that a wrong 
return value was used. However, if that's then already fixed in newer 
kernels where MIPL is included in the source tree, we all can be happy 
again. :)

-- 
Tero