xfrm: Duplicate SPI Handling

The issue originates when Strongswan initiates an XFRM_MSG_ALLOCSPI
Netlink message, which triggers the kernel function xfrm_alloc_spi().
This function is expected to ensure uniqueness of the Security Parameter
Index (SPI) for inbound Security Associations (SAs). However, it can
return success even when the requested SPI is already in use, leading
to duplicate SPIs assigned to multiple inbound SAs, differentiated
only by their destination addresses.

This behavior causes inconsistencies during SPI lookups for inbound packets.
Since the lookup may return an arbitrary SA among those with the same SPI,
packet processing can fail, resulting in packet drops.

According to RFC 4301 section 4.4.2 , for inbound processing a unicast SA
is uniquely identified by the SPI and optionally protocol.

Reproducing the Issue Reliably:
To consistently reproduce the problem, restrict the available SPI range in
charon.conf : spi_min = 0x10000000 spi_max = 0x10000002
This limits the system to only 2 usable SPI values.
Next, create more than 2 Child SA. each using unique pair of src/dst address.
As soon as the 3rd Child SA is initiated, it will be assigned a duplicate
SPI, since the SPI pool is already exhausted.
With a narrow SPI range, the issue is consistently reproducible.
With a broader/default range, it becomes rare and unpredictable.

Current implementation:
xfrm_spi_hash() lookup function computes hash using daddr, proto, and family.
So if two SAs have the same SPI but different destination addresses, then
they will:
a. Hash into different buckets
b. Be stored in different linked lists (byspi + h)
c. Not be seen in the same hlist_for_each_entry_rcu() iteration.
As a result, the lookup will result in NULL and kernel allows that Duplicate SPI

Proposed Change:
xfrm_state_lookup_spi_proto() does a truly global search - across all states,
regardless of hash bucket and matches SPI and proto.

Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 77cc418ad69eaa2aac4d8ca63abca69808355648..b3950234b150561a5fc77cd04966b89bc62e4e49 100644 (file)
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1711,6 +1711,26 @@ struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
 }
 EXPORT_SYMBOL(xfrm_state_lookup_byspi);
 
+static struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net, __be32 spi, u8 proto)
+{
+       struct xfrm_state *x;
+       unsigned int i;
+
+       rcu_read_lock();
+       for (i = 0; i <= net->xfrm.state_hmask; i++) {
+               hlist_for_each_entry_rcu(x, &net->xfrm.state_byspi[i], byspi) {
+                       if (x->id.spi == spi && x->id.proto == proto) {
+                               if (!xfrm_state_hold_rcu(x))
+                                       continue;
+                               rcu_read_unlock();
+                               return x;
+                       }
+               }
+       }
+       rcu_read_unlock();
+       return NULL;
+}
+
 static void __xfrm_state_insert(struct xfrm_state *x)
 {
        struct net *net = xs_net(x);
@@ -2555,10 +2575,8 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
        unsigned int h;
        struct xfrm_state *x0;
        int err = -ENOENT;
-       __be32 minspi = htonl(low);
-       __be32 maxspi = htonl(high);
+       u32 range = high - low + 1;
        __be32 newspi = 0;
-       u32 mark = x->mark.v & x->mark.m;
 
        spin_lock_bh(&x->lock);
        if (x->km.state == XFRM_STATE_DEAD) {
@@ -2572,38 +2590,34 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 
        err = -ENOENT;
 
-       if (minspi == maxspi) {
-               x0 = xfrm_state_lookup(net, mark, &x->id.daddr, minspi, x->id.proto, x->props.family);
-               if (x0) {
-                       NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
-                       xfrm_state_put(x0);
+       for (h = 0; h < range; h++) {
+               u32 spi = (low == high) ? low : get_random_u32_inclusive(low, high);
+               newspi = htonl(spi);
+
+               spin_lock_bh(&net->xfrm.xfrm_state_lock);
+               x0 = xfrm_state_lookup_spi_proto(net, newspi, x->id.proto);
+               if (!x0) {
+                       x->id.spi = newspi;
+                       h = xfrm_spi_hash(net, &x->id.daddr, newspi, x->id.proto, x->props.family);
+                       XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h, x->xso.type);
+                       spin_unlock_bh(&net->xfrm.xfrm_state_lock);
+                       err = 0;
                        goto unlock;
                }
-               newspi = minspi;
-       } else {
-               u32 spi = 0;
-               for (h = 0; h < high-low+1; h++) {
-                       spi = get_random_u32_inclusive(low, high);
-                       x0 = xfrm_state_lookup(net, mark, &x->id.daddr, htonl(spi), x->id.proto, x->props.family);
-                       if (x0 == NULL) {
-                               newspi = htonl(spi);
-                               break;
-                       }
-                       xfrm_state_put(x0);
+               xfrm_state_put(x0);
+               spin_unlock_bh(&net->xfrm.xfrm_state_lock);
+
+               if (signal_pending(current)) {
+                       err = -ERESTARTSYS;
+                       goto unlock;
                }
+
+               if (low == high)
+                       break;
        }
-       if (newspi) {
-               spin_lock_bh(&net->xfrm.xfrm_state_lock);
-               x->id.spi = newspi;
-               h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, x->props.family);
-               XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h,
-                                 x->xso.type);
-               spin_unlock_bh(&net->xfrm.xfrm_state_lock);
 
-               err = 0;
-       } else {
+       if (err)
                NL_SET_ERR_MSG(extack, "No SPI available in the requested range");
-       }
 
 unlock:
        spin_unlock_bh(&x->lock);