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

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

[Aakash Kumar S]

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 alone. Therefore, relying on additional fields
(such as destination addresses, proto) to disambiguate SPIs contradicts
the RFC and undermines protocol correctness.

Hence, the change is necessary to enforce strict SPI uniqueness for inbound SAs,
ensuring deterministic lookup behavior and compliance with the IPsec specification.

Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
---
 net/xfrm/xfrm_hash.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_hash.h b/net/xfrm/xfrm_hash.h
index d12bb906c9c9..a71b6dbdf532 100644
--- a/net/xfrm/xfrm_hash.h
+++ b/net/xfrm/xfrm_hash.h
@@ -116,18 +116,11 @@ static inline unsigned int __xfrm_src_hash(const xfrm_address_t *daddr,
 }
 
 static inline unsigned int
-__xfrm_spi_hash(const xfrm_address_t *daddr, __be32 spi, u8 proto,
-		unsigned short family, unsigned int hmask)
+__xfrm_spi_hash(const xfrm_address_t * __maybe_unused daddr, __be32 spi,
+		u8 __maybe_unused proto, unsigned short __maybe_unused family,
+		unsigned int hmask)
 {
-	unsigned int h = (__force u32)spi ^ proto;
-	switch (family) {
-	case AF_INET:
-		h ^= __xfrm4_addr_hash(daddr);
-		break;
-	case AF_INET6:
-		h ^= __xfrm6_addr_hash(daddr);
-		break;
-	}
+	unsigned int h = (__force u32)spi;
 	return (h ^ (h >> 10) ^ (h >> 20)) & hmask;
 }
 
-- 
2.48.1

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

[Aakash Kumar S]

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 alone. Therefore, relying on additional fields
(such as destination addresses, proto) to disambiguate SPIs contradicts
the RFC and undermines protocol correctness.

Hence, the change is necessary to enforce strict SPI uniqueness for inbound SAs,
ensuring deterministic lookup behavior and compliance with the IPsec specification.

Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
---
 net/xfrm/xfrm_hash.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/xfrm/xfrm_hash.h b/net/xfrm/xfrm_hash.h
index d12bb906c9c9..a71b6dbdf532 100644
--- a/net/xfrm/xfrm_hash.h
+++ b/net/xfrm/xfrm_hash.h
@@ -116,18 +116,11 @@ static inline unsigned int __xfrm_src_hash(const xfrm_address_t *daddr,
 }
 
 static inline unsigned int
-__xfrm_spi_hash(const xfrm_address_t *daddr, __be32 spi, u8 proto,
-		unsigned short family, unsigned int hmask)
+__xfrm_spi_hash(const xfrm_address_t * __maybe_unused daddr, __be32 spi,
+		u8 __maybe_unused proto, unsigned short __maybe_unused family,
+		unsigned int hmask)
 {
-	unsigned int h = (__force u32)spi ^ proto;
-	switch (family) {
-	case AF_INET:
-		h ^= __xfrm4_addr_hash(daddr);
-		break;
-	case AF_INET6:
-		h ^= __xfrm6_addr_hash(daddr);
-		break;
-	}
+	unsigned int h = (__force u32)spi;
 	return (h ^ (h >> 10) ^ (h >> 20)) & hmask;
 }
 
-- 
2.48.1

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

[Herbert Xu]

On Mon, May 26, 2025 at 12:13:22PM +0530, Aakash Kumar S wrote:
>
>  static inline unsigned int
> -__xfrm_spi_hash(const xfrm_address_t *daddr, __be32 spi, u8 proto,
> -		unsigned short family, unsigned int hmask)
> +__xfrm_spi_hash(const xfrm_address_t * __maybe_unused daddr, __be32 spi,
> +		u8 __maybe_unused proto, unsigned short __maybe_unused family,
> +		unsigned int hmask)
>  {
> -	unsigned int h = (__force u32)spi ^ proto;
> -	switch (family) {
> -	case AF_INET:
> -		h ^= __xfrm4_addr_hash(daddr);
> -		break;
> -	case AF_INET6:
> -		h ^= __xfrm6_addr_hash(daddr);
> -		break;
> -	}
> +	unsigned int h = (__force u32)spi;
>  	return (h ^ (h >> 10) ^ (h >> 20)) & hmask;
>  }

I don't think this patch is sufficient.  The logic around state
lookups need to be changed to exclude the destination address
comparison to achieve your objective.

It's also dangerous to unilaterally do this since existing deployments
could rely on the old behaviour.  You'd need to add a toggle for
compatibility.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Aakash Kumar S]

    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 alone. Therefore, relying on additional fields
    (such as destination addresses, proto) 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 or protocols,
    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_byspi() does a truly global search - across all states,
    regardless of hash bucket and matches SPI for a specified family

    Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
---
 net/xfrm/xfrm_state.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 341d79ecb5c2..d0b221a4a625 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2550,7 +2550,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,7 +2564,7 @@ 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);
+		x0 = xfrm_state_lookup_byspi(net, minspi, x->props.family);
 		if (x0) {
 			NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
 			xfrm_state_put(x0);
@@ -2576,7 +2575,7 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 		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_byspi(net, htonl(spi), x->props.family);
 			if (x0 == NULL) {
 				newspi = htonl(spi);
 				break;
-- 
2.48.1

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

[Herbert Xu]

On Mon, Jun 02, 2025 at 11:49:48PM +0530, Aakash Kumar S wrote:
>
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 341d79ecb5c2..d0b221a4a625 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2550,7 +2550,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,7 +2564,7 @@ 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);
> +		x0 = xfrm_state_lookup_byspi(net, minspi, x->props.family);
>  		if (x0) {
>  			NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
>  			xfrm_state_put(x0);
> @@ -2576,7 +2575,7 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
>  		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_byspi(net, htonl(spi), x->props.family);
>  			if (x0 == NULL) {
>  				newspi = htonl(spi);
>  				break;

The patch looks OK to me.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

That function in general has some issues though.  First of all
the SPI search is racy.  We only take the lock and update the
state database after the search.  That means the supposedly unique
SPI may no longer be unique.

The search for an SPI in the range is also prone to DoS attacks
if the SPI range is large and dense at the same time.  That depends
on how user-space constructs the range of course.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Aakash Kumar S]

        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 alone. Therefore, relying on additional fields
        (such as destination addresses, proto) 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 or protocols,
        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_byspi() does a truly global search - across all states,
        regardless of hash bucket and matches SPI for a specified family

        Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
---
 net/xfrm/xfrm_state.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 341d79ecb5c2..4a3d6fbb3fba 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2550,7 +2550,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 +2564,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_byspi(net, htonl(spi), x->props.family);
 			if (x0 == NULL) {
 				newspi = htonl(spi);
 				break;
@@ -2587,6 +2580,14 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
 	if (newspi) {
 		spin_lock_bh(&net->xfrm.xfrm_state_lock);
 		x->id.spi = newspi;
+
+		x0 = xfrm_state_lookup_byspi(net, newspi, x->props.family);
+		if (x0) {
+			NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
+			xfrm_state_put(x0);
+			goto unlock;
+		}
+
 		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);
-- 
2.43.0

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

[Steffen Klassert]

On Mon, Jun 09, 2025 at 12:20:14PM +0530, Aakash Kumar S wrote:
>         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 alone. Therefore, relying on additional fields
>         (such as destination addresses, proto) to disambiguate SPIs contradicts
>         the RFC and undermines protocol correctness.

This is not quite right, RFC 4301 says:

If the packet is addressed to the IPsec device and AH or ESP is
specified as the protocol, the packet is looked up in the SAD.
For unicast traffic, use only the SPI (or SPI plus protocol).

So using the potocol as as lookup key is OK.

> 
>         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 or protocols,
>         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_byspi() does a truly global search - across all states,
>         regardless of hash bucket and matches SPI for a specified family
> 
>         Signed-off-by: Aakash Kumar S <saakashkumar@marvell.com>
> ---
>  net/xfrm/xfrm_state.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 341d79ecb5c2..4a3d6fbb3fba 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2550,7 +2550,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 +2564,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_byspi(net, htonl(spi), x->props.family);
>  			if (x0 == NULL) {
>  				newspi = htonl(spi);
>  				break;
> @@ -2587,6 +2580,14 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
>  	if (newspi) {
>  		spin_lock_bh(&net->xfrm.xfrm_state_lock);
>  		x->id.spi = newspi;
> +
> +		x0 = xfrm_state_lookup_byspi(net, newspi, x->props.family);
> +		if (x0) {
> +			NL_SET_ERR_MSG(extack, "Requested SPI is already in use");
> +			xfrm_state_put(x0);
> +			goto unlock;
> +		}
> +

This looks wrong. xfrm_state_lookup_byspi takes the xfrm_state_lock as
well. Also, now we do the lookup twice if minspi != maxspi and the
extack message is wrong in that case. And the SPI is still not guaraneed
to be unique because the lookup depends on the address family.

Maybe it is better to create a new lookup function that does exactly
what we need for this case.

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

[Steffen Klassert]

On Wed, Jun 11, 2025 at 11:39:59AM +0000, Aakash Kumar Shankarappa wrote:
> Hi Steffen,
> Thanks for the review.
> Agreed. As per the RFC, for unicast traffic, the packet is looked up in the SAD based on the SPI and optionally the protocol.
> Since the protocol is optional and no existing lookup incorporates spi + protocol , I used the closest available function — xfrm_state_lookup_byspi(). If you agree, I can add a new lookup function that matches on both SPI and protocol, as shown below.
> Let me know your comment.

Yes, something like this should do it.

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

[Aakash Kumar S]

        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@marvell.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

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

[Antony Antony]

On Wed, Jun 11, 2025 at 12:51:51PM +0200, Steffen Klassert wrote:
> On Mon, Jun 09, 2025 at 12:20:14PM +0530, Aakash Kumar S wrote:
> >         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.

Is this an issue in xfrm stack without hardware offload? Does it affect packet
flow? It seems SA install will allow non uniqe SPI for incoming, even
after this. Which is the correct behaviour.

I have a feeling this is an issue due to hardware implementer's choice
of only support one option from RFC 4301 section 4.4.2?

> > 
> >         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 alone. Therefore, relying on additional fields
> >         (such as destination addresses, proto) to disambiguate SPIs contradicts
> >         the RFC and undermines protocol correctness.

> 
> This is not quite right, RFC 4301 says:
> 
> If the packet is addressed to the IPsec device and AH or ESP is
> specified as the protocol, the packet is looked up in the SAD.
> For unicast traffic, use only the SPI (or SPI plus protocol).
> 
> So using the potocol as as lookup key is OK.

That’s correct—the commit message should not reference RFC 6701. I would suggest instead referring to RFC 4301, specifically sections 4.1 and 4.4.2. These sections describe the three SPI uniqueness constraints allowed by the architecture for the incoming path.

- Unique SPI
- Unique (SPI + protocol (AH/ESP) + destination IP of the incoming SA)
- Unique (SPI + protocol + destination IP + source IP)

The IPsec architecture RFC 4301 explicitly allows all three options for 
incoming SAs, which is why I don’t believe anything is currently broken in 
the data path.

So it is not complaince issue. In my opinion, RFC 6071 is not standard RFC 
to be complaint with. It looks like an odd RFC among IPsec standards, what 
is IPsec-v3? IPsec IMHO should refer RFC 7296, 4303 and 4301 and related 
RFCs. We can also take this on IETF mailing list, instead of netdev.

As for SPI allocation, it is indeed permissible to be restrictive, as proposed by this patch.
However, section 4.4.2 of RFC 4301—which indirectly governs SPI 
allocation—is only applicable to unicast SAs.

So I’m also supportive of the patch, provided the commit message offers a 
clearer explanation and outlines what exactly is broken under the current 
behavior. Ideally also how to re-produce it.  This is important. I would 
love to see generic hardware supporting all 3 cases above, just like xfrm 
implementation.

PS: I am ccing more IPsec experts.
latest version of the patch is at:
https://lore.kernel.org/all/20250612055017.806273-1-saakashkumar@marvell.com/

-antony