netdev - Re: [PATCH] xfrm: Duplicate SPI Handling

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aFptePFscRTqKHT6@gauss3.secunet.de>
Date: Tue, 24 Jun 2025 11:18:48 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Aakash Kumar S <saakashkumar@...vell.com>
CC: <netdev@...r.kernel.org>, <herbert@...dor.apana.org.au>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <horms@...nel.org>, <akamaluddin@...vell.com>,
	<antony@...nome.org>
Subject: Re: [PATCH] xfrm: Duplicate SPI Handling

On Fri, Jun 20, 2025 at 03:08:23PM +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@...vell.com>
> ---
>  include/net/xfrm.h    |  3 +++
>  net/xfrm/xfrm_state.c | 38 ++++++++++++++++++++++++++++++--------
>  2 files changed, 33 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..fb05f47898fe 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1714,6 +1714,28 @@ 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;
> +}

Now, that the function is not exported anymore, it can be static.