[PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[Steffen Klassert]

Move the selector family initialization behind the check for AF_UNSPEC
and call xfrm_ip2inner_mode() in any case. So the selector family is
intitalized and we can choose for the right inner_mode.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_input.c |   19 ++++---------------
 net/xfrm/xfrm_state.c |    2 ++
 net/xfrm/xfrm_user.c  |    4 ----
 3 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 7527940..d220ecf 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -91,11 +91,9 @@ int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
 	if (err)
 		return err;
 
-	if (x->sel.family == AF_UNSPEC) {
-		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-		if (inner_mode == NULL)
-			return -EAFNOSUPPORT;
-	}
+	inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
+	if (inner_mode == NULL)
+		return -EAFNOSUPPORT;
 
 	skb->protocol = inner_mode->afinfo->eth_proto;
 	return inner_mode->input2(x, skb);
@@ -108,7 +106,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	__be32 seq;
 	struct xfrm_state *x;
 	xfrm_address_t *daddr;
-	struct xfrm_mode *inner_mode;
 	unsigned int family;
 	int decaps = 0;
 	int async = 0;
@@ -215,15 +212,7 @@ resume:
 
 		XFRM_MODE_SKB_CB(skb)->protocol = nexthdr;
 
-		inner_mode = x->inner_mode;
-
-		if (x->sel.family == AF_UNSPEC) {
-			inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-			if (inner_mode == NULL)
-				goto drop;
-		}
-
-		if (inner_mode->input(x, skb)) {
+		if (x->inner_mode->input(x, skb)) {
 			XFRM_INC_STATS(LINUX_MIB_XFRMINSTATEMODEERROR);
 			goto drop;
 		}
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 72fddaf..9ea1008 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2030,6 +2030,8 @@ int xfrm_init_state(struct xfrm_state *x)
 			x->inner_mode = inner_mode_iaf;
 			x->inner_mode_iaf = inner_mode;
 		}
+
+		x->sel.family = family;
 	}
 
 	x->type = xfrm_get_type(x->id.proto, family);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b976d9e..dae0956 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -276,10 +276,6 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
 	x->props.family = p->family;
 	memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
 	x->props.flags = p->flags;
-
-	if (!x->sel.family)
-		x->sel.family = p->family;
-
 }
 
 /*
-- 
1.5.3

Re: [PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[Herbert Xu]

On Sat, Jul 05, 2008 at 09:19:50AM +0200, Steffen Klassert wrote:
> Move the selector family initialization behind the check for AF_UNSPEC
> and call xfrm_ip2inner_mode() in any case. So the selector family is
> intitalized and we can choose for the right inner_mode.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/xfrm/xfrm_input.c |   19 ++++---------------
>  net/xfrm/xfrm_state.c |    2 ++
>  net/xfrm/xfrm_user.c  |    4 ----
>  3 files changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 7527940..d220ecf 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -91,11 +91,9 @@ int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
>  	if (err)
>  		return err;
>  
> -	if (x->sel.family == AF_UNSPEC) {
> -		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
> -		if (inner_mode == NULL)
> -			return -EAFNOSUPPORT;
> -	}
> +	inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
> +	if (inner_mode == NULL)
> +		return -EAFNOSUPPORT;

If sel.family is specified, inner_mode should already be set, no?
Overriding it would seem insecure.

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 v2] xfrm: Fix inter family IPsec tunnel handling again

[Steffen Klassert]

On Sat, Jul 05, 2008 at 07:56:55PM +0800, Herbert Xu wrote:
> > 
> > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > index 7527940..d220ecf 100644
> > --- a/net/xfrm/xfrm_input.c
> > +++ b/net/xfrm/xfrm_input.c
> > @@ -91,11 +91,9 @@ int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
> >  	if (err)
> >  		return err;
> >  
> > -	if (x->sel.family == AF_UNSPEC) {
> > -		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
> > -		if (inner_mode == NULL)
> > -			return -EAFNOSUPPORT;
> > -	}
> > +	inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
> > +	if (inner_mode == NULL)
> > +		return -EAFNOSUPPORT;
> 
> If sel.family is specified, inner_mode should already be set, no?
> Overriding it would seem insecure.
> 

Actually we are setting sel.family to the outer mode family if it is 
unspecified from userspace, so it is always specified at this point. This was 
changed by 
commit bcf0dda8 ([XFRM]: xfrm_user: fix selector family initialization)
because openswan seems to have problems if sel.family is not initialized.

But now the checks for AF_UNSPEC that are introduced by 
commit df9dcb45 ([IPSEC]: Fix inter address family IPsec tunnel handling)
return false. So the code that tries to choose for the right inner mode if
sel.family was not specified from userspace is not executed and an 
inter family tunnel does not work in this case.

I tried to move the selector family initialization behind the check for 
AF_UNSPEC in xfrm_init_state(). This brings an inter family tunnel to work if 
no selector family is specified from userspace. But yes, if a selector family 
is specified we may send a packet that is not allowed to go. So this patch is 
probaply not what we want.

But what's the right thing to do for inter family tunnels?

Is it expected that the selector family is set explicitly from userspace in 
this case?
Unfortunalely many userspace applications don't do this by default.
In this case there is nothing to fix here but most of the code introduced by 
commit df9dcb45 is obsolete then. 

Or should the kernel try to do the right thing if the selector is not 
specified?
This was probaply the idea of commit df9dcb45. But if we have to initialize the
selector family with something that is not AF_UNSPEC at initialization time of
the xfrm_state, we can't know whether we got a selector family from userspace
or whether it was set by the kernel when we process packets with that 
xfrm_state.  Thus, we can't decide whether we are allowed to modify the 
inner mode. At the moment I don't see an easy solution for this case, 
but perhaps somebody else has an idea.

Steffen

Re: [PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[Herbert Xu]

On Tue, Jul 08, 2008 at 11:53:35AM +0200, Steffen Klassert wrote:
>
> Actually we are setting sel.family to the outer mode family if it is 
> unspecified from userspace, so it is always specified at this point. This was 
> changed by 
> commit bcf0dda8 ([XFRM]: xfrm_user: fix selector family initialization)
> because openswan seems to have problems if sel.family is not initialized.
> 
> But now the checks for AF_UNSPEC that are introduced by 
> commit df9dcb45 ([IPSEC]: Fix inter address family IPsec tunnel handling)
> return false. So the code that tries to choose for the right inner mode if
> sel.family was not specified from userspace is not executed and an 
> inter family tunnel does not work in this case.

I discussed this with Miyazawa-san earlier in the year.  In order
to maintain backwards compatibility, the use of AF_UNSPEC needs to
be combined with an SA flag.  So let's create a new flag that can
be set by the applications which needs the AF_UNSPEC behaviour.

That way you get what you want without breaking existing users such
as Openswan.

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 v2] xfrm: Fix inter family IPsec tunnel handling again

[Steffen Klassert]

On Tue, Jul 08, 2008 at 06:40:22PM +0800, Herbert Xu wrote:
> 
> I discussed this with Miyazawa-san earlier in the year.  In order
> to maintain backwards compatibility, the use of AF_UNSPEC needs to
> be combined with an SA flag.  So let's create a new flag that can
> be set by the applications which needs the AF_UNSPEC behaviour.
> 

Just to get you right, do you mean something like the patch below?
With this and a patched iproute2 I've got the inter family tunnels running.

Steffen

---
 include/linux/xfrm.h |    1 +
 net/xfrm/xfrm_user.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
index 2ca6bae..fb0c215 100644
--- a/include/linux/xfrm.h
+++ b/include/linux/xfrm.h
@@ -339,6 +339,7 @@ struct xfrm_usersa_info {
 #define XFRM_STATE_NOPMTUDISC	4
 #define XFRM_STATE_WILDRECV	8
 #define XFRM_STATE_ICMP		16
+#define XFRM_STATE_AF_UNSPEC	32
 };
 
 struct xfrm_usersa_id {
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b976d9e..04c4150 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -277,9 +277,8 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
 	memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
 	x->props.flags = p->flags;
 
-	if (!x->sel.family)
+	if (!x->sel.family && !(p->flags & XFRM_STATE_AF_UNSPEC))
 		x->sel.family = p->family;
-
 }
 
 /*
-- 
1.5.3

Re: [PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[Herbert Xu]

On Wed, Jul 09, 2008 at 04:24:18PM +0200, Steffen Klassert wrote:
>
> Just to get you right, do you mean something like the patch below?
> With this and a patched iproute2 I've got the inter family tunnels running.

Precisely.

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: [PATCH v2] xfrm: Fix inter family IPsec tunnel handling again

[Steffen Klassert]

On Thu, Jul 10, 2008 at 09:12:48AM +0800, Herbert Xu wrote:
> On Wed, Jul 09, 2008 at 04:24:18PM +0200, Steffen Klassert wrote:
> >
> > Just to get you right, do you mean something like the patch below?
> > With this and a patched iproute2 I've got the inter family tunnels running.
> 
> Precisely.
> 
So I'll write a commit message and send it later today.
For the case that this one makes it in I will send a patch for iproute2
too. Then we would have at least one application that uses the new flag
for now.

Thanks for all the review,

Steffen