IPSec: setkey -DP freezes machine

IPSec: setkey -DP freezes machine

[Tom Lendacky]

I found the reason for the hang problem when issuing the "setkey -DP"
command while racoon is running.  The racoon program sets a socket option
on the socket(s) it listens on.  The socket options are effectively "in
bypass" and "out bypass" for the IP_IPSEC_POLICY option name.  The
af_key.c/pfkey_compile_policy function is ultimately invoked to create an
xfrm_policy structure.  The xfrm_policy structure's family value is not set
(since this information is not available to pfkey_compile_policy).  The
xfrm_policy structure is then added to the xfrm_policy_list[] array by
calling xfrm_policy.c/xfrm_sk_policy_insert.

When the "setkey -DP" command is issued, the list of policies is walked and
translated from the xfrm_policy structure to sadb_ messages by
af_key.c/pfkey_xfrm_policy2msg.  A change was added in 2.5.61 so that if
the xfrm_policy family is not AF_INET or AF_INET6 then BUG() is executed.
Since it is zero, BUG() is executed.

This can be fixed in xfrm_state.c/xfrm_user_policy by assigning the socket
family (the sock structure is an argument provided to xfrm_user_policy) to
the xfrm_policy family before calling xfrm_sk_policy_insert.  But, in the
case of IP_XFRM_POLICY the xfrm_user.c, xfrm_compile_policy function sets
the xfrm_policy family.  And in the future, other "compile_policy"
functions may be added.

So for the fix, would it be preferable to have the xfrm_policy family
always be assigned the socket family value or should it retain the current
setting and only be set to the socket family value if the current value is
0 (AF_UNSPEC)?

Tom

Re: IPSec: setkey -DP freezes machine

[James Morris]

On Thu, 27 Feb 2003, Tom Lendacky wrote:

> So for the fix, would it be preferable to have the xfrm_policy family
> always be assigned the socket family value or should it retain the current
> setting and only be set to the socket family value if the current value is
> 0 (AF_UNSPEC)?

The first may be necessary, as the family field is needed along the
following path:

pfkey_compile_policy()
 -> parse_ipsecrequests()
   -> parse_ipsecrequest() {
              ... 
              if (t->mode) {
                      switch (xp->family) {
    
               ...
      }

In the code snippet above, xp->family will be zero as xp was allocated in 
pfkey_compile_policy() and not set after being zeroed.

This is assuming we want to be able to set tunnel mode on a socket (which
is supported in some implementations e.g. Solaris, and can be very
useful).

If so, it would be good if we could make use of half of the
sadb_x_policy_reserved2 field to carry the socket family value, and copy
it during pfkey_compile_policy().

Alternatively, a family parameter could be added to the compile_policy() 
operation, but this duplicates data already present in our native 
xfrm_userpolicy_info format.


- James
-- 
James Morris
<jmorris@intercode.com.au>

Re: IPSec: setkey -DP freezes machine

[David S. Miller]

   From: James Morris <jmorris@intercode.com.au>
   Date: Sat, 1 Mar 2003 03:01:04 +1100 (EST)
   
   Alternatively, a family parameter could be added to the compile_policy() 
   operation, but this duplicates data already present in our native 
   xfrm_userpolicy_info format.

I like this solution, it seems the cleanest.

Could someone implement this fix and send me the patch?
I'm very backlogged for the next day or so...

[PATCH] Re: IPSec: setkey -DP freezes machine

[James Morris]

On Mon, 3 Mar 2003, David S. Miller wrote:

>    Alternatively, a family parameter could be added to the compile_policy() 
>    operation, but this duplicates data already present in our native 
>    xfrm_userpolicy_info format.
> 
> I like this solution, it seems the cleanest.
> 

Ok, here's a patch which does this.

I've also added check to verify_newpolicy_info() so that we don't run into
the same problem for policies provided via the netlink interface.

Tom, would you let me know if this works for you, as my racoon isn't
working yet.


- James
-- 
James Morris
<jmorris@intercode.com.au>



diff -urN -X dontdiff linux-2.5.63.orig/include/net/xfrm.h linux-2.5.63.w1/include/net/xfrm.h
--- linux-2.5.63.orig/include/net/xfrm.h	Fri Feb 21 00:44:01 2003
+++ linux-2.5.63.w1/include/net/xfrm.h	Mon Mar  3 22:19:40 2003
@@ -223,7 +223,7 @@
 	char			*id;
 	int			(*notify)(struct xfrm_state *x, int event);
 	int			(*acquire)(struct xfrm_state *x, struct xfrm_tmpl *, struct xfrm_policy *xp, int dir);
-	struct xfrm_policy	*(*compile_policy)(int opt, u8 *data, int len, int *dir);
+	struct xfrm_policy	*(*compile_policy)(u16 family, int opt, u8 *data, int len, int *dir);
 };
 
 extern int xfrm_register_km(struct xfrm_mgr *km);
diff -urN -X dontdiff linux-2.5.63.orig/net/ipv4/xfrm_state.c linux-2.5.63.w1/net/ipv4/xfrm_state.c
--- linux-2.5.63.orig/net/ipv4/xfrm_state.c	Fri Feb 21 00:44:01 2003
+++ linux-2.5.63.w1/net/ipv4/xfrm_state.c	Mon Mar  3 22:23:53 2003
@@ -680,7 +680,7 @@
 	err = -EINVAL;
 	read_lock(&xfrm_km_lock);
 	list_for_each_entry(km, &xfrm_km_list, list) {
-		pol = km->compile_policy(optname, data, optlen, &err);
+		pol = km->compile_policy(sk->family, optname, data, optlen, &err);
 		if (err >= 0)
 			break;
 	}
diff -urN -X dontdiff linux-2.5.63.orig/net/ipv4/xfrm_user.c linux-2.5.63.w1/net/ipv4/xfrm_user.c
--- linux-2.5.63.orig/net/ipv4/xfrm_user.c	Tue Feb 25 15:03:26 2003
+++ linux-2.5.63.w1/net/ipv4/xfrm_user.c	Mon Mar  3 22:56:34 2003
@@ -538,6 +538,21 @@
 		return -EINVAL;
 	};
 
+	switch (p->family) {
+	case AF_INET:
+		break;
+
+	case AF_INET6:
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+		break;
+#else
+		return  -EAFNOSUPPORT;
+#endif
+
+	default:
+		return -EINVAL;
+	};
+
 	return verify_policy_dir(p->dir);
 }
 
@@ -1057,7 +1072,8 @@
 /* User gives us xfrm_user_policy_info followed by an array of 0
  * or more templates.
  */
-struct xfrm_policy *xfrm_compile_policy(int opt, u8 *data, int len, int *dir)
+struct xfrm_policy *xfrm_compile_policy(u16 family, int opt,
+                                        u8 *data, int len, int *dir)
 {
 	struct xfrm_userpolicy_info *p = (struct xfrm_userpolicy_info *)data;
 	struct xfrm_user_tmpl *ut = (struct xfrm_user_tmpl *) (p + 1);
diff -urN -X dontdiff linux-2.5.63.orig/net/key/af_key.c linux-2.5.63.w1/net/key/af_key.c
--- linux-2.5.63.orig/net/key/af_key.c	Tue Feb 25 15:03:26 2003
+++ linux-2.5.63.w1/net/key/af_key.c	Mon Mar  3 22:30:56 2003
@@ -2420,7 +2420,8 @@
 	return pfkey_broadcast(skb, GFP_ATOMIC, BROADCAST_REGISTERED, NULL);
 }
 
-static struct xfrm_policy *pfkey_compile_policy(int opt, u8 *data, int len, int *dir)
+static struct xfrm_policy *pfkey_compile_policy(u16 family, int opt,
+                                                u8 *data, int len, int *dir)
 {
 	struct xfrm_policy *xp;
 	struct sadb_x_policy *pol = (struct sadb_x_policy*)data;
@@ -2451,6 +2452,7 @@
 	xp->lft.hard_byte_limit = XFRM_INF;
 	xp->lft.soft_packet_limit = XFRM_INF;
 	xp->lft.hard_packet_limit = XFRM_INF;
+	xp->family = family;
 
 	xp->xfrm_nr = 0;
 	if (pol->sadb_x_policy_type == IPSEC_POLICY_IPSEC &&

Re: [PATCH] Re: IPSec: setkey -DP freezes machine

[David S. Miller]

   From: James Morris <jmorris@intercode.com.au>
   Date: Mon, 3 Mar 2003 23:13:55 +1100 (EST)

   On Mon, 3 Mar 2003, David S. Miller wrote:
   
   >    Alternatively, a family parameter could be added to the compile_policy() 
   >    operation, but this duplicates data already present in our native 
   >    xfrm_userpolicy_info format.
   > 
   > I like this solution, it seems the cleanest.
   
   Ok, here's a patch which does this.
   
Looks good, I'll apply this.

If more problems are found, we can patch on top of this.

Re: [PATCH] Re: IPSec: setkey -DP freezes machine

[David S. Miller]

   From: "Tom Lendacky" <toml@us.ibm.com>
   Date: Mon, 3 Mar 2003 09:37:37 -0600

   > Tom, would you let me know if this works for you, as my racoon isn't
   > working yet.
   
   The patch works for me, setkey -DP no longer freezes the machine and the
   proper output is displayed.

Thank you for testing.

Re: [PATCH] Re: IPSec: setkey -DP freezes machine

[Tom Lendacky]

> Ok, here's a patch which does this.
>
> I've also added check to verify_newpolicy_info() so that we don't run
into
> the same problem for policies provided via the netlink interface.
>
> Tom, would you let me know if this works for you, as my racoon isn't
> working yet.

The patch works for me, setkey -DP no longer freezes the machine and the
proper output is displayed.

Tom