[PATCH] ipcomp: double free at ipcomp_destroy()

[PATCH] ipcomp: double free at ipcomp_destroy()

[Alexey Dobriyan]

Consider using ipcomp with tunnel mode:

	pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state

1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed
   first time (synchronously), but stale pointer is left.
2. xfrm_state_init() failed, all right, we're going to do error unwind
   but this time asynchronously and we're going to double free x->data
   asynchronously.

Fix by clearing x->data pointer, so second time it'll be fine.

Note, second time can happen in quite arbitrary time, double free
messages were seen in completely irrelevant functions, e. g. 

	INFO: Allocated in icmp_sk_init
	INFO: Freed in icmp_sk_exit

	[<ffffffff810b5ceb>] kfree+0xab/0x140
	[<ffffffff810719ae>] free_sect_attrs	(!)
	[<ffffffff81072353>] free_module

The only common thing was kmalloc-16 cache.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 net/xfrm/xfrm_ipcomp.c |    1 +
 1 file changed, 1 insertion(+)

--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -332,6 +332,7 @@ void ipcomp_destroy(struct xfrm_state *x)
 	ipcomp_free_data(ipcd);
 	mutex_unlock(&ipcomp_resource_mutex);
 	kfree(ipcd);
+	x->data = NULL;
 }
 EXPORT_SYMBOL_GPL(ipcomp_destroy);
 

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Sun, Feb 14, 2010 at 04:44:15PM +0200, Alexey Dobriyan wrote:
> Consider using ipcomp with tunnel mode:
> 
> 	pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state
> 
> 1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed
>    first time (synchronously), but stale pointer is left.
> 2. xfrm_state_init() failed, all right, we're going to do error unwind
>    but this time asynchronously and we're going to double free x->data
>    asynchronously.

Sorry, I don't see the async path, where is it?

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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Alexey Dobriyan]

On Mon, Feb 15, 2010 at 2:18 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sun, Feb 14, 2010 at 04:44:15PM +0200, Alexey Dobriyan wrote:
>> Consider using ipcomp with tunnel mode:
>>
>>       pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state
>>
>> 1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed
>>    first time (synchronously), but stale pointer is left.
>> 2. xfrm_state_init() failed, all right, we're going to do error unwind
>>    but this time asynchronously and we're going to double free x->data
>>    asynchronously.
>
> Sorry, I don't see the async path, where is it?

pfkey_add
pfkey_msg2xfrm_state
xfrm_init_state         [fails]
xfrm_state_put
__xfrm_state_destroy    [puts xfrm_state into GC list, schedule work]
xfrm_state_gc_task
xfrm_state_gc_destroy
x->type->destructor

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Mon, Feb 15, 2010 at 09:32:33AM +0200, Alexey Dobriyan wrote:
>
> pfkey_add
> pfkey_msg2xfrm_state
> xfrm_init_state         [fails]
> xfrm_state_put
> __xfrm_state_destroy    [puts xfrm_state into GC list, schedule work]
> xfrm_state_gc_task
> xfrm_state_gc_destroy
> x->type->destructor

Doh, I was looking at the buggy xfrm_state_clone path (which
incidently needs to be fixed to use xfrm_state_put).

How about just getting rid of the ipcomp_destroy call in ipcomp's
init_state functions? Calling the destructor twice is a bit iffy
even if we can make it work by setting various things to NULL and
testing for it.

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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Mon, Feb 15, 2010 at 04:08:46PM +0800, Herbert Xu wrote:
> 
> How about just getting rid of the ipcomp_destroy call in ipcomp's
> init_state functions? Calling the destructor twice is a bit iffy
> even if we can make it work by setting various things to NULL and
> testing for it.

Something like this (totally untested):

diff --git a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
index 38fbf04..544ce08 100644
--- a/net/ipv4/ipcomp.c
+++ b/net/ipv4/ipcomp.c
@@ -124,16 +124,12 @@ static int ipcomp4_init_state(struct xfrm_state *x)
 	if (x->props.mode == XFRM_MODE_TUNNEL) {
 		err = ipcomp_tunnel_attach(x);
 		if (err)
-			goto error_tunnel;
+			goto out;
 	}
 
 	err = 0;
 out:
 	return err;
-
-error_tunnel:
-	ipcomp_destroy(x);
-	goto out;
 }
 
 static const struct xfrm_type ipcomp_type = {
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 2f2a5ca..002e6ee 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -154,16 +154,12 @@ static int ipcomp6_init_state(struct xfrm_state *x)
 	if (x->props.mode == XFRM_MODE_TUNNEL) {
 		err = ipcomp6_tunnel_attach(x);
 		if (err)
-			goto error_tunnel;
+			goto out;
 	}
 
 	err = 0;
 out:
 	return err;
-error_tunnel:
-	ipcomp_destroy(x);
-
-	goto out;
 }
 
 static const struct xfrm_type ipcomp6_type =

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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Alexey Dobriyan]

On Mon, Feb 15, 2010 at 04:10:52PM +0800, Herbert Xu wrote:
> On Mon, Feb 15, 2010 at 04:08:46PM +0800, Herbert Xu wrote:
> > 
> > How about just getting rid of the ipcomp_destroy call in ipcomp's
> > init_state functions? Calling the destructor twice is a bit iffy
> > even if we can make it work by setting various things to NULL and
> > testing for it.
> 
> Something like this (totally untested):
> 
> diff --git a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
> index 38fbf04..544ce08 100644
> --- a/net/ipv4/ipcomp.c
> +++ b/net/ipv4/ipcomp.c
> @@ -124,16 +124,12 @@ static int ipcomp4_init_state(struct xfrm_state *x)
>  	if (x->props.mode == XFRM_MODE_TUNNEL) {
>  		err = ipcomp_tunnel_attach(x);
>  		if (err)
> -			goto error_tunnel;
> +			goto out;
>  	}
>  
>  	err = 0;
>  out:
>  	return err;
> -
> -error_tunnel:
> -	ipcomp_destroy(x);
> -	goto out;
>  }
>  
>  static const struct xfrm_type ipcomp_type = {
> diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
> index 2f2a5ca..002e6ee 100644
> --- a/net/ipv6/ipcomp6.c
> +++ b/net/ipv6/ipcomp6.c
> @@ -154,16 +154,12 @@ static int ipcomp6_init_state(struct xfrm_state *x)
>  	if (x->props.mode == XFRM_MODE_TUNNEL) {
>  		err = ipcomp6_tunnel_attach(x);
>  		if (err)
> -			goto error_tunnel;
> +			goto out;
>  	}
>  
>  	err = 0;
>  out:
>  	return err;
> -error_tunnel:
> -	ipcomp_destroy(x);
> -
> -	goto out;

Then checks for NULL ->tunnel, and NULL ->data should be removed.
In fact, I don't quite understand why destruction was done this way,
so simply cleared ->data pointer.

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Alexey Dobriyan]

On Mon, Feb 15, 2010 at 04:10:52PM +0800, Herbert Xu wrote:
> On Mon, Feb 15, 2010 at 04:08:46PM +0800, Herbert Xu wrote:
> > 
> > How about just getting rid of the ipcomp_destroy call in ipcomp's
> > init_state functions? Calling the destructor twice is a bit iffy
> > even if we can make it work by setting various things to NULL and
> > testing for it.
> 
> Something like this (totally untested):

OK, it survives beating here.

> --- a/net/ipv4/ipcomp.c
> +++ b/net/ipv4/ipcomp.c
> @@ -124,16 +124,12 @@ static int ipcomp4_init_state(struct xfrm_state *x)
>  	if (x->props.mode == XFRM_MODE_TUNNEL) {
>  		err = ipcomp_tunnel_attach(x);
>  		if (err)
> -			goto error_tunnel;
> +			goto out;
>  	}
>  
>  	err = 0;
>  out:
>  	return err;
> -
> -error_tunnel:
> -	ipcomp_destroy(x);
> -	goto out;
>  }
>  
>  static const struct xfrm_type ipcomp_type = {
> diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
> index 2f2a5ca..002e6ee 100644
> --- a/net/ipv6/ipcomp6.c
> +++ b/net/ipv6/ipcomp6.c
> @@ -154,16 +154,12 @@ static int ipcomp6_init_state(struct xfrm_state *x)
>  	if (x->props.mode == XFRM_MODE_TUNNEL) {
>  		err = ipcomp6_tunnel_attach(x);
>  		if (err)
> -			goto error_tunnel;
> +			goto out;
>  	}
>  
>  	err = 0;
>  out:
>  	return err;
> -error_tunnel:
> -	ipcomp_destroy(x);
> -
> -	goto out;
>  }
>  
>  static const struct xfrm_type ipcomp6_type =
> 
> 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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Mon, Feb 15, 2010 at 07:28:10PM +0200, Alexey Dobriyan wrote:
>
> OK, it survives beating here.

Thanks a lot for testing! I'll do the clean-up you suggested in
another patch.  Let's get this fixed first.

ipcomp: Avoid duplicate calls to ipcomp_destroy

When ipcomp_tunnel_attach fails we will call ipcomp_destroy twice.
This may lead to double-frees on certain structures.

As there is no reason to explicitly call ipcomp_destroy, this patch
removes it from ipcomp*.c and lets the standard xfrm_state destruction
take place.

This is based on the discovery and patch by Alexey Dobriyan.

Tested-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
index 38fbf04..544ce08 100644
--- a/net/ipv4/ipcomp.c
+++ b/net/ipv4/ipcomp.c
@@ -124,16 +124,12 @@ static int ipcomp4_init_state(struct xfrm_state *x)
 	if (x->props.mode == XFRM_MODE_TUNNEL) {
 		err = ipcomp_tunnel_attach(x);
 		if (err)
-			goto error_tunnel;
+			goto out;
 	}
 
 	err = 0;
 out:
 	return err;
-
-error_tunnel:
-	ipcomp_destroy(x);
-	goto out;
 }
 
 static const struct xfrm_type ipcomp_type = {
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 2f2a5ca..002e6ee 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -154,16 +154,12 @@ static int ipcomp6_init_state(struct xfrm_state *x)
 	if (x->props.mode == XFRM_MODE_TUNNEL) {
 		err = ipcomp6_tunnel_attach(x);
 		if (err)
-			goto error_tunnel;
+			goto out;
 	}
 
 	err = 0;
 out:
 	return err;
-error_tunnel:
-	ipcomp_destroy(x);
-
-	goto out;
 }
 
 static const struct xfrm_type ipcomp6_type =

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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Mon, Feb 15, 2010 at 05:50:49PM +0200, Alexey Dobriyan wrote:
>
> Then checks for NULL ->tunnel, and NULL ->data should be removed.
> In fact, I don't quite understand why destruction was done this way,
> so simply cleared ->data pointer.

I had a look and I don't think we can do that (unless I misunderstood
what you meant).

The NULL ipcd check in ipcomp_destroy is to handle the case where
ipcomp_init_state fails before it even gets to allocating ipcd.

While the NULL tunnel check in xfrm_state_delete_tunnel is for the
case where we failed before successfully attaching a tunnel.

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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[Herbert Xu]

On Mon, Feb 15, 2010 at 04:08:46PM +0800, Herbert Xu wrote:
>
> Doh, I was looking at the buggy xfrm_state_clone path (which
> incidently needs to be fixed to use xfrm_state_put).

Here's a fix for that problem.

xfrm: Fix xfrm_state_clone leak

xfrm_state_clone calls kfree instead of xfrm_state_put to free
a failed state.  Depending on the state of the failed state, it
can cause leaks to things like module references.

All states should be freed by xfrm_state_put past the point of
xfrm_init_state.

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

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index b36cc34..f445ea1 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1102,7 +1102,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
 	int err = -ENOMEM;
 	struct xfrm_state *x = xfrm_state_alloc(net);
 	if (!x)
-		goto error;
+		goto out;
 
 	memcpy(&x->id, &orig->id, sizeof(x->id));
 	memcpy(&x->sel, &orig->sel, sizeof(x->sel));
@@ -1160,16 +1160,10 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
 	return x;
 
  error:
+	xfrm_state_put(x);
+out:
 	if (errp)
 		*errp = err;
-	if (x) {
-		kfree(x->aalg);
-		kfree(x->ealg);
-		kfree(x->calg);
-		kfree(x->encap);
-		kfree(x->coaddr);
-	}
-	kfree(x);
 	return NULL;
 }
 
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

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 16 Feb 2010 13:24:30 +0800

> On Mon, Feb 15, 2010 at 07:28:10PM +0200, Alexey Dobriyan wrote:
>>
>> OK, it survives beating here.
> 
> Thanks a lot for testing! I'll do the clean-up you suggested in
> another patch.  Let's get this fixed first.
> 
> ipcomp: Avoid duplicate calls to ipcomp_destroy
> 
> When ipcomp_tunnel_attach fails we will call ipcomp_destroy twice.
> This may lead to double-frees on certain structures.
> 
> As there is no reason to explicitly call ipcomp_destroy, this patch
> removes it from ipcomp*.c and lets the standard xfrm_state destruction
> take place.
> 
> This is based on the discovery and patch by Alexey Dobriyan.
> 
> Tested-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to net-2.6

Re: [PATCH] ipcomp: double free at ipcomp_destroy()

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 16 Feb 2010 14:00:51 +0800

> On Mon, Feb 15, 2010 at 04:08:46PM +0800, Herbert Xu wrote:
>>
>> Doh, I was looking at the buggy xfrm_state_clone path (which
>> incidently needs to be fixed to use xfrm_state_put).
> 
> Here's a fix for that problem.
> 
> xfrm: Fix xfrm_state_clone leak
> 
> xfrm_state_clone calls kfree instead of xfrm_state_put to free
> a failed state.  Depending on the state of the failed state, it
> can cause leaks to things like module references.
> 
> All states should be freed by xfrm_state_put past the point of
> xfrm_init_state.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to net-2.6