[Openswan dev] Re: SPI generation by netlink_get_spi()

[Herbert Xu]

On Thu, Jul 29, 2004 at 03:04:53PM +0200, Andreas Steffen wrote:
>
> : |    message ID:  65 7e 1a 0f
> : | netlink_get_spi: allocated 0x9f4c9788 for esp.0 at 145.254.54.68
> : | SPI  9f 4c 97 88
> 
> The netlink interface of the 2.6 kernel is used to request an SPI for
> the IPsec SA.
> 
> Immediately after the first Quick Mode message the second pending Quick Mode
> is inititated:

> : |    message ID:  a1 01 a2 b2
> : | netlink_get_spi: allocated 0x9f4c9788 for esp.0 at 145.254.54.68
> : | SPI  9f 4c 97 88
> 
> And here the error happens. The two Quick Mode negotiations have different
> Message IDs (65 7e 1a 0f versus a1 01 a2 b2) which will cause two phase2
> state objects to be created on the peer side but the generated SPI 9f 4c 97 
> 88
> is the same. This will trigger the assertion passert(0) in 
> kernel_pfkey.c:finish_pfkey_msg() in freeswan-2.0x because twice the same 
> SADB_ADD command is executed for the outbound esp. Removing the assertion
> as in Openswan does not help - several retrials will not succeed in setting
> up the IPsec SA.

Yes this is a kernel bug.

The issue is that two successive calls to netlink_get_spi is returning
the same SA.  Since netlink_get_spi is meant to be a creation operation
this is incorrect.

The netlink_get_spi operation is modelled off the PFKEY SADB_GETSPI
command which is specified in RFC 2367.  The purpose of SADB_GETSPI
is to create a new larval SA that can then be filled in by SADB_UPDATE.

Its semantics does not allow two SADB_GETSPI calls to return the same
SA, even if there is no SADB_UPDATE call in between.

The reason the second netlink_get_spi is returning the same SA is
because in find_acq(), the code is looking at all larval states as
opposed to only larval states with an SPI of zero.

Since the only other caller of find_acq() -- xfrm_state_add() intentionally
ignores all return values with a non-zero SPI, it is safe to not look at
SAs with non-zero SPIs at all in find_acq().

The following patch does exactly that.

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

In fact, the find_acq() call in xfrm_state_add() is a remnant from
the days when we had xfrm_state_replace() instead of xfrm_state_add()
and xfrm_state_update().  It can now be safely removed.

I'll post a separate patch for that.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert at gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-------------- next part --------------
===== net/ipv4/xfrm4_state.c 1.10 vs edited =====
--- 1.10/net/ipv4/xfrm4_state.c	2004-07-09 20:19:08 +10:00
+++ edited/net/ipv4/xfrm4_state.c	2004-07-30 21:26:21 +10:00
@@ -74,11 +74,8 @@
 		    proto == x->id.proto &&
 		    saddr->a4 == x->props.saddr.a4 &&
 		    reqid == x->props.reqid &&
-		    x->km.state == XFRM_STATE_ACQ) {
-			    if (!x0)
-				    x0 = x;
-			    if (x->id.spi)
-				    continue;
+		    x->km.state == XFRM_STATE_ACQ &&
+		    !x->id.spi) {
 			    x0 = x;
 			    break;
 		    }
===== net/ipv6/xfrm6_state.c 1.11 vs edited =====
--- 1.11/net/ipv6/xfrm6_state.c	2004-05-27 18:57:44 +10:00
+++ edited/net/ipv6/xfrm6_state.c	2004-07-30 21:27:07 +10:00
@@ -81,11 +81,8 @@
 		    proto == x->id.proto &&
 		    !ipv6_addr_cmp((struct in6_addr *)saddr, (struct in6_addr *)x->props.saddr.a6) &&
 		    reqid == x->props.reqid &&
-		    x->km.state == XFRM_STATE_ACQ) {
-			    if (!x0)
-				    x0 = x;
-			    if (x->id.spi)
-				    continue;
+		    x->km.state == XFRM_STATE_ACQ &&
+		    !x->id.spi) {
 			    x0 = x;
 			    break;
 		    }