netdev - [PATCH] xfrm: Duplicate SPI Handling – IPsec-v3 Compliance Concern

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20250612055017.806273-1-saakashkumar@marvell.com>
Date: Thu, 12 Jun 2025 11:20:17 +0530
From: Aakash Kumar S <saakashkumar@...vell.com>
To: <netdev@...r.kernel.org>
CC: <steffen.klassert@...unet.com>, <herbert@...dor.apana.org.au>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <horms@...nel.org>, <saakashkumar@...vell.com>,
        <akamaluddin@...vell.com>
Subject: [PATCH]    xfrm: Duplicate SPI Handling – IPsec-v3 Compliance Concern

        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 6071, in IPsec-v3, a unicast SA is uniquely identified
        by the SPI and optionally protocol. Therefore, relying on additional fields
        (such as destination addresses) to disambiguate SPIs contradicts
        the RFC and undermines protocol correctness.

        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@...vell.com>
---
 include/net/xfrm.h    |  3 +++
 net/xfrm/xfrm_state.c | 39 +++++++++++++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 39365fd2ea17..bd128980e8fd 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1693,6 +1693,9 @@ struct xfrm_state *xfrm_stateonly_find(struct net *net, u32 mark, u32 if_id,
 				       u8 mode, u8 proto, u32 reqid);
 struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
 					      unsigned short family);
+struct xfrm_state *xfrm_state_lookup_spi_proto(struct net *net, __be32 spi,
+						u8 proto);
+
 int xfrm_state_check_expire(struct xfrm_state *x);
 void xfrm_state_update_stats(struct net *net);
 #ifdef CONFIG_XFRM_OFFLOAD
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 341d79ecb5c2..9820025610ee 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1714,6 +1714,29 @@ struct xfrm_state *xfrm_state_lookup_byspi(struct net *net, __be32 spi,
 }
 EXPORT_SYMBOL(xfrm_state_lookup_byspi);
 
+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;
+}
+EXPORT_SYMBOL(xfrm_state_lookup_spi_proto);
+
 static void __xfrm_state_insert(struct xfrm_state *x)
 {
 	struct net *net = xs_net(x);
@@ -2550,7 +2573,6 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 	__be32 minspi = htonl(low);
 	__be32 maxspi = htonl(high);
 	__be32 newspi = 0;
-	u32 mark = x->mark.v & x->mark.m;
 
 	spin_lock_bh(&x->lock);
 	if (x->km.state == XFRM_STATE_DEAD) {
@@ -2565,18 +2587,12 @@ 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);
-			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);
+			x0 = xfrm_state_lookup_spi_proto(net, htonl(spi), x->id.proto);
 			if (x0 == NULL) {
 				newspi = htonl(spi);
 				break;
@@ -2586,6 +2602,13 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 	}
 	if (newspi) {
 		spin_lock_bh(&net->xfrm.xfrm_state_lock);
+		x0 = xfrm_state_lookup_spi_proto(net, newspi, x->id.proto);
+		if (x0) {
+			NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
+			xfrm_state_put(x0);
+			goto unlock;
+		}
+
 		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,
-- 
2.43.0