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: <aEZpBsgcdTTKr98q@gondor.apana.org.au>
Date: Mon, 9 Jun 2025 12:54:30 +0800
From: Herbert Xu <herbert@...dor.apana.org.au>
To: Aakash Kumar S <saakashkumar@...vell.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, akamaluddin@...vell.com
Subject: Re: [PATCH] xfrm: Duplicate SPI Handling – IPsec-v3 Compliance Concern

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@...dor.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@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt