Patchwork ipcomp: double free at ipcomp_destroy()

login
register
mail settings
Submitter Alexey Dobriyan
Date Feb. 14, 2010, 2:44 p.m.
Message ID <20100214144415.GA8115@x200>
Download mbox | patch
Permalink /patch/45302/
State Superseded
Delegated to: stephen hemminger
Headers show

Comments

Alexey Dobriyan - Feb. 14, 2010, 2:44 p.m.
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(+)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Feb. 15, 2010, 12:18 a.m.
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,
Alexey Dobriyan - Feb. 15, 2010, 7:32 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Feb. 15, 2010, 8:08 a.m.
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,

Patch

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