net-2.6.25 splat

net-2.6.25 splat

[Andrew Morton]

Pulled the tree a couple of hours ago.  The machine was running the full
-mm lineup, had been compiling kernels for an hour or so then oopsed in
icmpv6_rcv+0x5b/0x832.

I have a partial photo of the scrolled-off backtrace but the camera cable
is at home.

I don't know why it was doing ipv6 things at all.  Maybe there's ip6 stuff
running around google's corp network, dunno.

gdb says:

(gdb) l *0x17387
0x17387 is in icmpv6_rcv (net/ipv6/icmp.c:649).
644             struct in6_addr *saddr, *daddr;
645             struct ipv6hdr *orig_hdr;
646             struct icmp6hdr *hdr;
647             int type;
648     
649             if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb) &&
650                 skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
651                     int nh;
652     
653                     if (!pskb_may_pull(skb, sizeof(*hdr) + sizeof(*orig_hdr)))


I'll set the display to 80x50 and retry..	

Re: net-2.6.25 splat

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 12 Dec 2007 16:21:40 -0800

> I don't know why it was doing ipv6 things at all.

When you bring up any interface, with explicit IPV6
addresses or not, the IPV6 stack seeks out local routers
and whatnot using multicast and assigns the interface
a link-local IPV6 address.

So no matter what you're doing IPV6.

I suspect the OOPS you hit might be related to
IPSEC stack work done recently by Herbert Xu who
has been added to the CC:.

I doubt poor Ilpo should be contacted as I doubt his
TCP work is involved :-)

Re: net-2.6.25 splat

[Andrew Morton]

On Wed, 12 Dec 2007 16:26:13 -0800 (PST) David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Wed, 12 Dec 2007 16:21:40 -0800
> 
> > I don't know why it was doing ipv6 things at all.
> 
> When you bring up any interface, with explicit IPV6
> addresses or not, the IPV6 stack seeks out local routers
> and whatnot using multicast and assigns the interface
> a link-local IPV6 address.
> 
> So no matter what you're doing IPV6.
> 
> I suspect the OOPS you hit might be related to
> IPSEC stack work done recently by Herbert Xu who
> has been added to the CC:.
> 
> I doubt poor Ilpo should be contacted as I doubt his
> TCP work is involved :-)

Here's the screen-shot (actually more like a reen-hot):
http://userweb.kernel.org/~akpm/pc121694.jpg

I'm awaiting a reoccurrence with the screen in 50-row mode.

[IPSEC]: Fix reversed ICMP6 policy check

[Herbert Xu]

On Wed, Dec 12, 2007 at 05:39:51PM -0800, Andrew Morton wrote:
>
> Here's the screen-shot (actually more like a reen-hot):
> http://userweb.kernel.org/~akpm/pc121694.jpg
> 
> I'm awaiting a reoccurrence with the screen in 50-row mode.

Sorry, I didn't test IPv6.

[IPSEC]: Fix reversed ICMP6 policy check

The policy check I added for ICMP on IPv6 is reversed.  This
patch fixes that.

It also adds an skb->sp check so that unprotected packets that
fail the policy check do not crash the machine.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 4e3bfcd..132e879 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -978,7 +978,7 @@ int icmp_rcv(struct sk_buff *skb)
 	struct icmphdr *icmph;
 	struct rtable *rt = (struct rtable *)skb->dst;
 
-	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb) &&
+	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb) && skb->sp &&
 	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
 		int nh;
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 478ee77..64d78c9 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -646,7 +646,7 @@ static int icmpv6_rcv(struct sk_buff *skb)
 	struct icmp6hdr *hdr;
 	int type;
 
-	if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb) &&
+	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb) && skb->sp &&
 	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
 		int nh;
 

Re: [IPSEC]: Fix reversed ICMP6 policy check

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 13 Dec 2007 09:58:56 +0800

> [IPSEC]: Fix reversed ICMP6 policy check
> 
> The policy check I added for ICMP on IPv6 is reversed.  This
> patch fixes that.
> 
> It also adds an skb->sp check so that unprotected packets that
> fail the policy check do not crash the machine.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.

Re: [IPSEC]: Fix reversed ICMP6 policy check

[Herbert Xu]

On Thu, Dec 13, 2007 at 09:58:56AM +0800, Herbert Xu wrote:
>
> [IPSEC]: Fix reversed ICMP6 policy check

While that won't crash anymore, it's still logically wrong.

Here's a more complete fix.

[IPSEC]: Fix reversed ICMP6 policy check

The policy check I added for ICMP on IPv6 is reversed.  We were also
letting packets through incorrectly if the ICMP flag isn't set.  This
patch fixes that.

It also adds an skb->sp check so that unprotected packets that
fail the policy check do not crash the machine.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 4e3bfcd..ccdef9a 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -978,10 +978,13 @@ int icmp_rcv(struct sk_buff *skb)
 	struct icmphdr *icmph;
 	struct rtable *rt = (struct rtable *)skb->dst;
 
-	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb) &&
-	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
+	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 		int nh;
 
+		if (!(skb->sp && skb->sp->xvec[skb->sp->len - 1]->props.flags &
+				 XFRM_STATE_ICMP))
+			goto drop;
+
 		if (!pskb_may_pull(skb, sizeof(*icmph) + sizeof(struct iphdr)))
 			goto drop;
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 478ee77..bbf4162 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -646,10 +646,13 @@ static int icmpv6_rcv(struct sk_buff *skb)
 	struct icmp6hdr *hdr;
 	int type;
 
-	if (xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb) &&
-	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
+	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 		int nh;
 
+		if (!(skb->sp && skb->sp->xvec[skb->sp->len - 1]->props.flags &
+				 XFRM_STATE_ICMP))
+			goto drop_no_count;
+
 		if (!pskb_may_pull(skb, sizeof(*hdr) + sizeof(*orig_hdr)))
 			goto drop_no_count;
 

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPSEC]: Fix reversed ICMP6 policy check

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 13 Dec 2007 10:49:18 +0800

> On Thu, Dec 13, 2007 at 09:58:56AM +0800, Herbert Xu wrote:
> >
> > [IPSEC]: Fix reversed ICMP6 policy check
> 
> While that won't crash anymore, it's still logically wrong.
> 
> Here's a more complete fix.

I already applied the first one, please send a relative
fixup which I'll combine as appropriate next rebase.

Re: [IPSEC]: Fix reversed ICMP6 policy check

[Herbert Xu]

On Wed, Dec 12, 2007 at 06:48:30PM -0800, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 13 Dec 2007 09:58:56 +0800
> 
> > [IPSEC]: Fix reversed ICMP6 policy check
> 
> Applied, thanks Herbert.

You're too quick :) Before you ask for an incremental patch, here's
a preemptive strike :)

[IPSEC]: Do not let packets pass when ICMP flag is off

This fixes a logical error in ICMP policy checks which lets
packets through if the state ICMP flag is off.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 132e879..ccdef9a 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -978,10 +978,13 @@ int icmp_rcv(struct sk_buff *skb)
 	struct icmphdr *icmph;
 	struct rtable *rt = (struct rtable *)skb->dst;
 
-	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb) && skb->sp &&
-	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
+	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 		int nh;
 
+		if (!(skb->sp && skb->sp->xvec[skb->sp->len - 1]->props.flags &
+				 XFRM_STATE_ICMP))
+			goto drop;
+
 		if (!pskb_may_pull(skb, sizeof(*icmph) + sizeof(struct iphdr)))
 			goto drop;
 
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 64d78c9..bbf4162 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -646,10 +646,13 @@ static int icmpv6_rcv(struct sk_buff *skb)
 	struct icmp6hdr *hdr;
 	int type;
 
-	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb) && skb->sp &&
-	    skb->sp->xvec[skb->sp->len - 1]->props.flags & XFRM_STATE_ICMP) {
+	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 		int nh;
 
+		if (!(skb->sp && skb->sp->xvec[skb->sp->len - 1]->props.flags &
+				 XFRM_STATE_ICMP))
+			goto drop_no_count;
+
 		if (!pskb_may_pull(skb, sizeof(*hdr) + sizeof(*orig_hdr)))
 			goto drop_no_count;
 

Re: [IPSEC]: Fix reversed ICMP6 policy check

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 13 Dec 2007 10:51:56 +0800

> You're too quick :) Before you ask for an incremental patch, here's
> a preemptive strike :)

Hehe :)

> [IPSEC]: Do not let packets pass when ICMP flag is off
> 
> This fixes a logical error in ICMP policy checks which lets
> packets through if the state ICMP flag is off.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!

Re: [IPSEC]: Fix reversed ICMP6 policy check

[Jarek Poplawski]

On 13-12-2007 03:49, Herbert Xu wrote:
> On Thu, Dec 13, 2007 at 09:58:56AM +0800, Herbert Xu wrote:
>> [IPSEC]: Fix reversed ICMP6 policy check
> 
> While that won't crash anymore, it's still logically wrong.
> 
> Here's a more complete fix.

...even more than this!

Since more than a year each time I read your patches I wonder what
kind of special attachments you use they are so "unreadble" (blurred)
in Mozilla Thunderbird (but, I was never so desperate to study all
these mail RFCs). And now - BINGO! So, they are simply treated as
signatures! Nice trick! But, I see, you forgot about something this
time, and now it's all clear! (Actually, I probably need one more
year to find, how to turn off this sh...)

Thanks,
Jarek P.