ipcomp: double free at ipcomp_destroy()

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


Cheers,

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.
--
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

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

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,