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

[Steffen Klassert <steffen.klassert@secunet.com>]

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.