netdev - Re: Occasional oops with IPSec and IPv6.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4EC75C99.4000304@iki.fi>
Date:	Sat, 19 Nov 2011 09:36:57 +0200
From:	Timo Teräs <timo.teras@....fi>
To:	Nick Bowler <nbowler@...iptictech.com>
CC:	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: Occasional oops with IPSec and IPv6.

On 11/18/2011 11:21 PM, Nick Bowler wrote:
> On 2011-11-18 22:06 +0200, Timo Teräs wrote:
>> It's still headroom underrun.
>>
>> I'm not too familiar with the relevant IPv6 code, but it seems to be
>> mostly modelled after the IPv4 side. Looking at the back trace offset
>> inside ipv6_fragment, I'd say it was taking the "fast path" for
>> constructing the fragments. So first guess is that the headroom check
>> for allowing fast path to happen is not right.
>>
>> Since the code seems to be treating separately hlen and struct frag_hdr,
>> I'm wondering if the following patch would be in place?
>>
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index 1c9bf8b..c35d9fc 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -675,7 +675,7 @@ int ip6_fragment(struct sk_buff *skb, int
>> (*output)(struct sk_buff *))
>>  			/* Correct geometry. */
>>  			if (frag->len > mtu ||
>>  			    ((frag->len & 7) && frag->next) ||
>> -			    skb_headroom(frag) < hlen)
>> +			    skb_headroom(frag) < hlen + sizeof(struct frag_hdr))
>>  				goto slow_path_clean;
>>
>>  			/* Partially cloned skb? */
>>
>>
>> Alternatively, we could just run the "slow path" unconditionally with
>> the test load to see if it fixes the issue. At least that'd be pretty
>> good test if it's a problem in the ipv6 fragmentation code or something
>> else.
> 
> Good call.  I replaced the "correct geometry" check with an
> unconditional "goto slow_path_clean;", and I can no longer reproduce the
> crash.  So at the very least, I have a workaround now.  (I still have
> Herbert Xu's six patches applied on top of Linus' master).

Ok, so it's most likely ipv6 code issue then. My change just happened to
trigger it.

> I then tried the smaller change above, but this does not correct the
> issue.

That's not it then (likely).

I did notice that the headroom of the main skb is never checked. So my
other suggestion is to try something like:

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 1c9bf8b..735c4dc 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -668,7 +668,8 @@ int ip6_fragment(struct sk_buff *skb, int
(*output)(struct sk_buff *))

 		if (first_len - hlen > mtu ||
 		    ((first_len - hlen) & 7) ||
-		    skb_cloned(skb))
+		    skb_cloned(skb) ||
+		    skb_headroom(skb) < sizeof(struct frag_hdr))
 			goto slow_path;

 		skb_walk_frags(skb, frag) {

Other than that, I hope some of the ipv6 people could take a look at it.
But the problem is that somewhere some headroom check isn't taking
place, or is checking for too little of headroom.

- Timo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html