| 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
| ||
|
Message-ID: <11958ea7-8f4b-4ac3-8e6a-7ce4e1c13441@6wind.com>
Date: Mon, 30 Jun 2025 14:27:56 +0200
From: Nicolas Dichtel <nicolas.dichtel@...nd.com>
To: Aakash Kumar S <saakashkumar@...vell.com>, 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, akamaluddin@...vell.com,
antony@...nome.org
Subject: Re: [PATCH] xfrm: Duplicate SPI Handling
Le 24/06/2025 à 20:10, Aakash Kumar S a écrit :
> 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>
> ---
> net/xfrm/xfrm_state.c | 78 ++++++++++++++++++++++++++-----------------
> 1 file changed, 47 insertions(+), 31 deletions(-)
>
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 341d79ecb5c2..74855af27d15 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);
>
> +static 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;
> +}
The whole function is indented with spaces.
> +
> static void __xfrm_state_insert(struct xfrm_state *x)
> {
> struct net *net = xs_net(x);
> @@ -2547,10 +2569,8 @@ int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high,
> unsigned int h;
> struct xfrm_state *x0;
> int err = -ENOENT;
> - __be32 minspi = htonl(low);
> - __be32 maxspi = htonl(high);
> + u32 range = high - low + 1;
> __be32 newspi = 0;
> - u32 mark = x->mark.v & x->mark.m;
>
> spin_lock_bh(&x->lock);
> if (x->km.state == XFRM_STATE_DEAD) {
> @@ -2564,39 +2584,35 @@ 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);
> - if (x0 == NULL) {
> - newspi = htonl(spi);
> - break;
> - }
> - xfrm_state_put(x0);
> - }
> - }
> - if (newspi) {
> + for (h = 0; h < range; h++) {
> + u32 spi = (low == high) ? low : get_random_u32_inclusive(low, high);
> + newspi = htonl(spi);
> +
> spin_lock_bh(&net->xfrm.xfrm_state_lock);
> - 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,
> - x->xso.type);
> + x0 = xfrm_state_lookup_spi_proto(net, newspi, x->id.proto);
> + if (!x0) {
> + x->id.spi = newspi;
> + h = xfrm_spi_hash(net, &x->id.daddr, newspi, x->id.proto, x->props.family);
> + XFRM_STATE_INSERT(byspi, &x->byspi, net->xfrm.state_byspi + h, x->xso.type);
> + spin_unlock_bh(&net->xfrm.xfrm_state_lock);
> + err = 0;
> + goto unlock;
> + }
The above line also.
> + xfrm_state_put(x0);
> spin_unlock_bh(&net->xfrm.xfrm_state_lock);
>
> - err = 0;
> - } else {
> - NL_SET_ERR_MSG(extack, "No SPI available in the requested range");
> + if (signal_pending(current)) {
> + err = -ERESTARTSYS;
> + goto unlock;
> + }
And this one ^
> +
> + if (low == high)
> + break;
> }
>
> + if (err)
> + NL_SET_ERR_MSG(extack, "No SPI available in the requested range");
> +
> unlock:
> spin_unlock_bh(&x->lock);
>
Powered by blists - more mailing lists