[PATCH][IPSEC][6/7] inter address family ipsec tunnel

[PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Kazunori MIYAZAWA]

This patch fixes mtu calculation of IPv4

ip_append_data should refer the mtu of "dst" not "path".
if "dst" is stacked, "path" is the actual dst_entry in
the routing table.
therefore the mtu of "path" equals link mtu which is
depends on the device so that it ignores the header length
and the trailer length
"dst" has mtu for creating packet.

Signed-off-by: Miika Komu <miika@iki.fi>
Signed-off-by: Diego Beltrami <Diego.Beltrami@hiit.fi>
Signed-off-by: Kazunori Miyazawa <miyazawa@linux-ipv6.org>

---
 net/ipv4/ip_output.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 1da3d32..f324449 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -806,7 +806,7 @@ int ip_append_data(struct sock *sk,
 			inet->cork.addr = ipc->addr;
 		}
 		dst_hold(&rt->u.dst);
-		inet->cork.fragsize = mtu = dst_mtu(rt->u.dst.path);
+		inet->cork.fragsize = mtu = dst_mtu(&rt->u.dst);
 		inet->cork.rt = rt;
 		inet->cork.length = 0;
 		sk->sk_sndmsg_page = NULL;

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller]

From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
Date: Fri, 24 Nov 2006 14:39:01 +0900

> This patch fixes mtu calculation of IPv4
> 
> ip_append_data should refer the mtu of "dst" not "path".
> if "dst" is stacked, "path" is the actual dst_entry in
> the routing table.
> therefore the mtu of "path" equals link mtu which is
> depends on the device so that it ignores the header length
> and the trailer length
> "dst" has mtu for creating packet.
> 
> Signed-off-by: Miika Komu <miika@iki.fi>
> Signed-off-by: Diego Beltrami <Diego.Beltrami@hiit.fi>
> Signed-off-by: Kazunori Miyazawa <miyazawa@linux-ipv6.org>

I'm not sure about this change.

If you look at the code in this function, "mtu" is always used with
adjustments via 'exthdrlen' (which is set to rt->u.dst.header_len).
So it seems the encapsulation is taken into account.

Perhaps any problem you are seeing is some artifact of the ipv6 in
ipv4 tunnel implementation.  Otherwise we'd have other reports of this
problem, wouldn't we?

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Kazunori MIYAZAWA]

David Miller wrote:
> From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
> Date: Fri, 24 Nov 2006 14:39:01 +0900
> 
>> This patch fixes mtu calculation of IPv4
>>
>> ip_append_data should refer the mtu of "dst" not "path".
>> if "dst" is stacked, "path" is the actual dst_entry in
>> the routing table.
>> therefore the mtu of "path" equals link mtu which is
>> depends on the device so that it ignores the header length
>> and the trailer length
>> "dst" has mtu for creating packet.
>>
>> Signed-off-by: Miika Komu <miika@iki.fi>
>> Signed-off-by: Diego Beltrami <Diego.Beltrami@hiit.fi>
>> Signed-off-by: Kazunori Miyazawa <miyazawa@linux-ipv6.org>
> 
> I'm not sure about this change.
> 
> If you look at the code in this function, "mtu" is always used with
> adjustments via 'exthdrlen' (which is set to rt->u.dst.header_len).
> So it seems the encapsulation is taken into account.
> 

Oh, sorry. I misread the code.
I tested with IPv4 over IPv4 IPsec tunnel. The original code works.
Sorry this patch was wrong. Please throw away [6/7] and [7/7].

> Perhaps any problem you are seeing is some artifact of the ipv6 in
> ipv4 tunnel implementation.  Otherwise we'd have other reports of this
> problem, wouldn't we?

The easy test is that you create IPv6 over IPv6 IPsec tunnel between 2 
hosts, the tunnel is terminated by themselves, and send icmp echo
packets which are longer than the mtu such as 2000. Then it seems
that the kernel  returns too big messages to ping6.
I guess mtu calculation and/or building packets of IPv6 has some problem

If you use another host (SGW) to encapsulate the packets,
I think it succeeds because SGW returns a too big message and the
packet sending host learns and fragments properly.

I had same situation like IPv6 over IPv6 IPsec self tunneling
under IPv6 over IPv4 IPsec tunneling test.

I'm going to trace the problem.

Thank you.

--
Kazunori Miyazawa

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller]

Kazunori, a bug from the changes I did apply:

[  761.318131] kernel BUG at net/key/af_key.c:1925!
[  761.318285] Caller[000000001010c0e0]: pfkey_send_policy_notify+0x6c/0x250 [af_key]
[  761.318296] Caller[000000000062f6dc]: km_policy_notify+0x28/0x50
[  761.318309] Caller[000000001014a94c]: xfrm_add_policy+0xd8/0x100 [xfrm_user]
[  761.318326] Caller[0000000010148b68]: xfrm_user_rcv_msg+0x1a4/0x1d0 [xfrm_user]
[  761.318336] Caller[00000000005ecef4]: netlink_run_queue+0x60/0x138
[  761.318346] Caller[00000000101487b0]: xfrm_netlink_rcv+0x28/0x48 [xfrm_user]
[  761.318356] Caller[00000000005ecffc]: netlink_data_ready+0x30/0x70
[  761.318363] Caller[00000000005eaa18]: netlink_sendskb+0x28/0x50
[  761.318370] Caller[00000000005ecc70]: netlink_sendmsg+0x2f0/0x300
[  761.318377] Caller[00000000005d22d0]: sock_aio_write+0xe0/0xec
[  761.318391] Caller[0000000000490ccc]: do_sync_write+0x88/0xd0
[  761.318400] Caller[00000000004913c8]: vfs_write+0x90/0x124
[  761.318408] Caller[0000000000491928]: sys_write+0x34/0x60
[  761.318415] Caller[0000000000406a54]: linux_sparc_syscall32+0x3c/0x40
[  761.318428] Caller[0000000000035cc8]: 0x35cd0

I think there is a mistake about the guarentees of the application
causing the xp->family field to be set properly.

Or in this specific case, xfrm_user netlink setting p->sel.family
in the struct xfrm_userpolicy of a XFRM_MSG_NEWPOLICY or
XFRM_MSG_UPDPOLICY netlink message.

This was with openswan.

But the user application doesn't matter, if we have requirements
about xp->family, we should enforce it, and this does not happen
which is why the above BUG() is able to trigger.

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sun, 03 Dec 2006 17:58:47 -0800 (PST)

> Kazunori, a bug from the changes I did apply:
> 
> [  761.318131] kernel BUG at net/key/af_key.c:1925!

I found the problem, it's because of the xfrm_user.c change where
we clobber the xp->family value with ut->family.

But we never ever verified nor cared about the ut->family value
because previously templates were all of the same family as the
policy, so there was no reason to check or verify the ut->family
value.

So applications left it at zero.

This means you did no testing of the xfrm_user.c netlink changes.

We can "fix" this with some patch like the below, changing
ut->family to xp->family if it is left at zero, but it is clear
that since we've never checked this value it can be any value.
What if it is left uninitialized by the application and the
garbage value just happens to be AF_INET6 or something?

To me this means that ut->family is %100 unreliable and we cannot
count on it in any way, and we'll need to specify the family in
some other way.

BTW, is it OK to clobber the entire policy's xp->family with the
top-most ut->family?  Shouldn't the application set the policy's
family to AF_INET6 if it wants the outer-most template to be
AF_INET6?

How can changing the policy family be valid?  Doing this means we'll
interpret the selectors of the policy differently from what the
application originally provided.  This setting of xp->family therefore
cannot make any sense, it must remain at whatever value the
application gave us.

I really regret applying these patches, they are in a very bad shape
and poorly designed.  Now every openswan user will get an OOPS
when they try to bring up their tunnels with Linus's current tree.

I think instead of the patch below, I'm going to revert at least
the xfrm_user part of these changes. :-/

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6f97665..76c7cdc 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -857,6 +857,11 @@ static void copy_templates(struct xfrm_p
 {
 	int i;
 
+
+	/* Backward compat for older applications.  */
+	if (ut->family == 0)
+		ut->family = xp->family;
+
 	xp->xfrm_nr = nr;
 	xp->family = ut->family;
 	for (i = 0; i < nr; i++, ut++) {

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Kazunori MIYAZAWA]

Hello David,

Thank you for your tracing the bug.

I understood the issue.
Mmm, if we can not use ut->family, can we use
ut->id.family instead?

Or is it also uninitialized?

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Sun, 03 Dec 2006 17:58:47 -0800 (PST)
> 
>> Kazunori, a bug from the changes I did apply:
>>
>> [  761.318131] kernel BUG at net/key/af_key.c:1925!
> 
> I found the problem, it's because of the xfrm_user.c change where
> we clobber the xp->family value with ut->family.
> 
> But we never ever verified nor cared about the ut->family value
> because previously templates were all of the same family as the
> policy, so there was no reason to check or verify the ut->family
> value.
> 
> So applications left it at zero.
> 
> This means you did no testing of the xfrm_user.c netlink changes.
> 
> We can "fix" this with some patch like the below, changing
> ut->family to xp->family if it is left at zero, but it is clear
> that since we've never checked this value it can be any value.
> What if it is left uninitialized by the application and the
> garbage value just happens to be AF_INET6 or something?
> 
> To me this means that ut->family is %100 unreliable and we cannot
> count on it in any way, and we'll need to specify the family in
> some other way.
> 
> BTW, is it OK to clobber the entire policy's xp->family with the
> top-most ut->family?  Shouldn't the application set the policy's
> family to AF_INET6 if it wants the outer-most template to be
> AF_INET6?
> 
> How can changing the policy family be valid?  Doing this means we'll
> interpret the selectors of the policy differently from what the
> application originally provided.  This setting of xp->family therefore
> cannot make any sense, it must remain at whatever value the
> application gave us.
> 
> I really regret applying these patches, they are in a very bad shape
> and poorly designed.  Now every openswan user will get an OOPS
> when they try to bring up their tunnels with Linus's current tree.
> 
> I think instead of the patch below, I'm going to revert at least
> the xfrm_user part of these changes. :-/
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 6f97665..76c7cdc 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -857,6 +857,11 @@ static void copy_templates(struct xfrm_p
>  {
>  	int i;
>  
> +
> +	/* Backward compat for older applications.  */
> +	if (ut->family == 0)
> +		ut->family = xp->family;
> +
>  	xp->xfrm_nr = nr;
>  	xp->family = ut->family;
>  	for (i = 0; i < nr; i++, ut++) {
> 

--
Kazunori Miyazawa

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller]

From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
Date: Mon, 04 Dec 2006 11:50:09 +0900

> Thank you for your tracing the bug.
> 
> I understood the issue.
> Mmm, if we can not use ut->family, can we use
> ut->id.family instead?
> 
> Or is it also uninitialized?

struct xfrm_id does not have a family field

struct xfrm_id
{
	xfrm_address_t	daddr;
	__be32		spi;
	__u8		proto;
};

That's what ut->id is.

You're thinking of xfrm_usersa_id, which is used by
another structure, xfrm_aevent_id.

For the time being I'm thinking of using the following
patch.  I removed the xp->family clobbering, the AF_KEY
changes don't do this, so there is no reason for the
xfrm_user side to do it either.

Every path that leads to copy_templates() will perform
a validate_tmpl() which will change ut->family == 0
to the policy's family value.  Any other non-supported
value will trigger an error.

Let's hope this fix is sufficient.  I'm about to test
this now.

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6f97665..311205f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -858,7 +858,6 @@ static void copy_templates(struct xfrm_p
 	int i;
 
 	xp->xfrm_nr = nr;
-	xp->family = ut->family;
 	for (i = 0; i < nr; i++, ut++) {
 		struct xfrm_tmpl *t = &xp->xfrm_vec[i];
 
@@ -876,19 +875,53 @@ static void copy_templates(struct xfrm_p
 	}
 }
 
+static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
+{
+	int i;
+
+	if (nr > XFRM_MAX_DEPTH)
+		return -EINVAL;
+
+	for (i = 0; i < nr; i++) {
+		/* We never validated the ut->family value, so many
+		 * applications simply leave it at zero.  The check was
+		 * never made and ut->family was ignored because all
+		 * templates could be assumed to have the same family as
+		 * the policy itself.  Now that we will have ipv4-in-ipv6
+		 * and ipv6-in-ipv4 tunnels, this is no longer true.
+		 */
+		if (!ut[i].family)
+			ut[i].family = family;
+
+		switch (ut[i].family) {
+		case AF_INET:
+			break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		case AF_INET6:
+			break;
+#endif
+		default:
+			return -EINVAL;
+		};
+	}
+
+	return 0;
+}
+
 static int copy_from_user_tmpl(struct xfrm_policy *pol, struct rtattr **xfrma)
 {
 	struct rtattr *rt = xfrma[XFRMA_TMPL-1];
-	struct xfrm_user_tmpl *utmpl;
-	int nr;
 
 	if (!rt) {
 		pol->xfrm_nr = 0;
 	} else {
-		nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
+		struct xfrm_user_tmpl *utmpl = RTA_DATA(rt);
+		int nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
+		int err;
 
-		if (nr > XFRM_MAX_DEPTH)
-			return -EINVAL;
+		err = validate_tmpl(nr, utmpl, pol->family);
+		if (err)
+			return err;
 
 		copy_templates(pol, RTA_DATA(rt), nr);
 	}
@@ -1530,7 +1563,8 @@ static int xfrm_add_acquire(struct sk_bu
 	}
 
 	/*   build an XP */
-	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);        if (!xp) {
+	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);
+	if (!xp) {
 		kfree(x);
 		return err;
 	}
@@ -1979,7 +2013,7 @@ #endif
 		return NULL;
 
 	nr = ((len - sizeof(*p)) / sizeof(*ut));
-	if (nr > XFRM_MAX_DEPTH)
+	if (validate_tmpl(nr, ut, p->sel.family))
 		return NULL;
 
 	if (p->dir > XFRM_POLICY_OUT)

Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Herbert Xu]

On Sun, Dec 03, 2006 at 07:12:08PM -0800, David Miller wrote:
> 
> For the time being I'm thinking of using the following
> patch.  I removed the xp->family clobbering, the AF_KEY
> changes don't do this, so there is no reason for the
> xfrm_user side to do it either.

This patch looks fine to me.

I am to blame for the genesis of this problem so here is a bit
of background :)

At the very start we had one family field and that was
xfrm_userpolicy_info->sel->family.  Since the only transforms
were of the same family it was sufficient.

I then added xfrm_user_tmpl->family in order to allow for the
possibility of inter-family transforms.  Unfortunately I didn't
add a check to the kernel to verify its value.  As a result,
Openswan was able to get away with leaving it as zero.

If anyone is overly worried about the potential of apps leaving
garbage rather than zero in this field, we can combine it with
xfrm_userpolicy_info->flags which should currently be zero.

So all we need to do is add a new flag (perhaps XFRM_POLICY_INTERFAMILY)
which must be set for the template family fields to be used.


As for xfrm_policy->family, it's an internal field that has lived
past its use-by date.  It should be replaced either by xp->sel.family
or xp->xfrm_vec[x].family where appropriate.

The family only makes sense when interpreted together with a layer
of address.  So we don't really need an overall family for a policy.

I hope that at the end of this patch series xp->family can be removed
altogether.

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: (usagi-core 31727) Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[Kazunori MIYAZAWA]

David Miller wrote:
> From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
> Date: Mon, 04 Dec 2006 11:50:09 +0900
> 
>> Thank you for your tracing the bug.
>>
>> I understood the issue.
>> Mmm, if we can not use ut->family, can we use
>> ut->id.family instead?
>>
>> Or is it also uninitialized?
> 
> struct xfrm_id does not have a family field
> 
> struct xfrm_id
> {
> 	xfrm_address_t	daddr;
> 	__be32		spi;
> 	__u8		proto;
> };
> 
> That's what ut->id is.
> 
> You're thinking of xfrm_usersa_id, which is used by
> another structure, xfrm_aevent_id.
> 

Sorry I misread.

> For the time being I'm thinking of using the following
> patch.  I removed the xp->family clobbering, the AF_KEY
> changes don't do this, so there is no reason for the
> xfrm_user side to do it either.
> 
> Every path that leads to copy_templates() will perform
> a validate_tmpl() which will change ut->family == 0
> to the policy's family value.  Any other non-supported
> value will trigger an error.
> 
> Let's hope this fix is sufficient.  I'm about to test
> this now.
> 

Thank you very much.

If uninitialized ut->family is AF_INET or AF_INET6 by chance
and the family of outer addresses (ut->saddr) is differnt
ut->family, it results some garbage in the kernel as you know.

I think it does not results any oops or a segmentation fault
because xfrm_address always has enough length (16 bytes) to wrong
access.

 From the point of view of security, the policy has garbege
templates, but the selector is valid and it mangates applying
IPsec. So it result blocking the traffic.
Accordingly, I think it falls down to secure side.

Anyway I will test the patch.

Thank you,

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 6f97665..311205f 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -858,7 +858,6 @@ static void copy_templates(struct xfrm_p
>  	int i;
>  
>  	xp->xfrm_nr = nr;
> -	xp->family = ut->family;
>  	for (i = 0; i < nr; i++, ut++) {
>  		struct xfrm_tmpl *t = &xp->xfrm_vec[i];
>  
> @@ -876,19 +875,53 @@ static void copy_templates(struct xfrm_p
>  	}
>  }
>  
> +static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
> +{
> +	int i;
> +
> +	if (nr > XFRM_MAX_DEPTH)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nr; i++) {
> +		/* We never validated the ut->family value, so many
> +		 * applications simply leave it at zero.  The check was
> +		 * never made and ut->family was ignored because all
> +		 * templates could be assumed to have the same family as
> +		 * the policy itself.  Now that we will have ipv4-in-ipv6
> +		 * and ipv6-in-ipv4 tunnels, this is no longer true.
> +		 */
> +		if (!ut[i].family)
> +			ut[i].family = family;
> +
> +		switch (ut[i].family) {
> +		case AF_INET:
> +			break;
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> +		case AF_INET6:
> +			break;
> +#endif
> +		default:
> +			return -EINVAL;
> +		};
> +	}
> +
> +	return 0;
> +}
> +
>  static int copy_from_user_tmpl(struct xfrm_policy *pol, struct rtattr **xfrma)
>  {
>  	struct rtattr *rt = xfrma[XFRMA_TMPL-1];
> -	struct xfrm_user_tmpl *utmpl;
> -	int nr;
>  
>  	if (!rt) {
>  		pol->xfrm_nr = 0;
>  	} else {
> -		nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
> +		struct xfrm_user_tmpl *utmpl = RTA_DATA(rt);
> +		int nr = (rt->rta_len - sizeof(*rt)) / sizeof(*utmpl);
> +		int err;
>  
> -		if (nr > XFRM_MAX_DEPTH)
> -			return -EINVAL;
> +		err = validate_tmpl(nr, utmpl, pol->family);
> +		if (err)
> +			return err;
>  
>  		copy_templates(pol, RTA_DATA(rt), nr);
>  	}
> @@ -1530,7 +1563,8 @@ static int xfrm_add_acquire(struct sk_bu
>  	}
>  
>  	/*   build an XP */
> -	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);        if (!xp) {
> +	xp = xfrm_policy_construct(&ua->policy, (struct rtattr **) xfrma, &err);
> +	if (!xp) {
>  		kfree(x);
>  		return err;
>  	}
> @@ -1979,7 +2013,7 @@ #endif
>  		return NULL;
>  
>  	nr = ((len - sizeof(*p)) / sizeof(*ut));
> -	if (nr > XFRM_MAX_DEPTH)
> +	if (validate_tmpl(nr, ut, p->sel.family))
>  		return NULL;
>  
>  	if (p->dir > XFRM_POLICY_OUT)
> 
> 

Re: (usagi-core 31727) Re: [PATCH][IPSEC][6/7] inter address family ipsec tunnel

[David Miller]

From: Kazunori MIYAZAWA <kazunori@miyazawa.org>
Date: Mon, 04 Dec 2006 13:26:29 +0900

> If uninitialized ut->family is AF_INET or AF_INET6 by chance
> and the family of outer addresses (ut->saddr) is differnt
> ut->family, it results some garbage in the kernel as you know.
> 
> I think it does not results any oops or a segmentation fault
> because xfrm_address always has enough length (16 bytes) to wrong
> access.
> 
>  From the point of view of security, the policy has garbege
> templates, but the selector is valid and it mangates applying
> IPsec. So it result blocking the traffic.
> Accordingly, I think it falls down to secure side.

Yes, I am beginning to think it is safe too.