[IPSEC]: Fix potential dst leak in xfrm_lookup

[IPSEC]: Fix potential dst leak in xfrm_lookup

[Herbert Xu]

Hi Dave:

This patch fixes a possible dst leak that has existed for years.

[IPSEC]: Fix potential dst leak in xfrm_lookup

If we get an error during the actual policy lookup we don't free the
original dst while the caller expects us to always free the original
dst in case of error.

This patch fixes that.

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

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 0cb3e8c..265c679 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1464,8 +1464,9 @@ restart:
 
 	if (sk && sk->sk_policy[XFRM_POLICY_OUT]) {
 		policy = xfrm_sk_policy_lookup(sk, XFRM_POLICY_OUT, fl);
+		err = PTR_ERR(policy);
 		if (IS_ERR(policy))
-			return PTR_ERR(policy);
+			goto dropdst;
 	}
 
 	if (!policy) {
@@ -1476,8 +1477,9 @@ restart:
 
 		policy = flow_cache_lookup(fl, dst_orig->ops->family,
 					   dir, xfrm_policy_lookup);
+		err = PTR_ERR(policy);
 		if (IS_ERR(policy))
-			return PTR_ERR(policy);
+			goto dropdst;
 	}
 
 	if (!policy)
@@ -1642,8 +1644,9 @@ restart:
 	return 0;
 
 error:
-	dst_release(dst_orig);
 	xfrm_pols_put(pols, npols);
+dropdst:
+	dst_release(dst_orig);
 	*dst_p = NULL;
 	return err;
 }

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: [IPSEC]: Fix potential dst leak in xfrm_lookup

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 11 Dec 2007 20:07:29 +0800

> [IPSEC]: Fix potential dst leak in xfrm_lookup
> 
> If we get an error during the actual policy lookup we don't free the
> original dst while the caller expects us to always free the original
> dst in case of error.
> 
> This patch fixes that.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Sigh, thanks for fixing this.  Applied to net-2.6 and I'll toss it
over to -stable too.

I bet the __xfrm_lookup() callers could stand a major audit, with the
special -EREMOTE logic I bet there are non-EREMOTE code paths there
that don't handle the dst ref semantics properly.

This is a very error prone interface, both at the implementation
and in the callers.

Re: [IPSEC]: Fix potential dst leak in xfrm_lookup

[Herbert Xu]

On Tue, Dec 11, 2007 at 04:40:16AM -0800, David Miller wrote:
> 
> I bet the __xfrm_lookup() callers could stand a major audit, with the
> special -EREMOTE logic I bet there are non-EREMOTE code paths there
> that don't handle the dst ref semantics properly.
> 
> This is a very error prone interface, both at the implementation
> and in the callers.

Heh, didn't we already do an audit a couple of years ago when
we made the decision that the dst is always going to be freed
in case of an error?

But yeah another audit for this and EREMOTE wouldn't hurt.

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