Re: [PATCH] xfrm: Duplicate SPI Handling

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

On Mon, Jun 16, 2025 at 03:36:21PM +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 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>

Nit: Please remove the leading whitespaces from the commit message
so that I don't have to hand edit it when applying.

> ---
>  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);

Do we really need to export this function? It is used just
in net/xfrm/xfrm_state.c