From 485f7645a2822d34f95bde8848251c2f1697306b Mon Sep 17 00:00:00 2001
From: Tobias Brunner <tobias@strongswan.org>
Date: Thu, 9 Jul 2026 19:29:26 +0200
Subject: [PATCH] kernel-wfp: Add support for SA deletion in either order

The code was written with `child_sa_t::destroy` in mind, which deletes
the inbound SA before the outbound SA.  The problem is that the rekeying
code was changed meanwhile so the outbound SA is removed before the
inbound SA in order to avoid traffic loss.  That could cause a
use-after-free as the already destroyed item remained in the `isas` list.

This change fixes this so the SAs can be removed in any order.  The SPIs
are used as marker for whether a specific direction is installed.  It
also fixes an issue in `expire_job()`, which removed the entry from
`osas` without holding the lock.

Fixes: f351d9ef7d70 ("kernel-wfp: Reference SA/SP sets by SPI and destination, not reqid")
Fixes: 44107cb7b755 ("child-delete: Delay the removal of the inbound SA of rekeyed CHILD_SAs")
---
 .../plugins/kernel_wfp/kernel_wfp_ipsec.c     | 58 +++++++++++++------
 1 file changed, 40 insertions(+), 18 deletions(-)

diff --git a/src/libcharon/plugins/kernel_wfp/kernel_wfp_ipsec.c b/src/libcharon/plugins/kernel_wfp/kernel_wfp_ipsec.c
index 34f882626..bc4001fa5 100644
--- a/src/libcharon/plugins/kernel_wfp/kernel_wfp_ipsec.c
+++ b/src/libcharon/plugins/kernel_wfp/kernel_wfp_ipsec.c
@@ -2055,16 +2055,19 @@ static job_requeue_t expire_job(expire_data_t *data)
 	{
 		this->mutex->lock(this->mutex);
 		entry = this->isas->remove(this->isas, &key);
+		if (entry)
+		{
+			if (entry->osa.spi)
+			{
+				key.spi = entry->osa.spi;
+				key.dst = entry->osa.dst;
+				this->osas->remove(this->osas, &key);
+			}
+		}
 		this->mutex->unlock(this->mutex);
 		if (entry)
 		{
 			protocol = entry->isa.protocol;
-			if (entry->osa.dst)
-			{
-				key.dst = entry->osa.dst;
-				key.spi = entry->osa.spi;
-				this->osas->remove(this->osas, &key);
-			}
 			entry_destroy(this, entry);
 		}
 	}
@@ -2328,27 +2331,46 @@ METHOD(kernel_ipsec_t, del_sa, status_t,
 		.dst = id->dst,
 		.spi = id->spi,
 	};
+	bool destroy = FALSE;
 
+	/* make sure we support deleting in either order (regular is inbound before
+	 * outbound, during rekeying, the inbound is usually kept installed longer).
+	 * only once both directions are deleted can the entry be destroyed */
 	this->mutex->lock(this->mutex);
 	entry = this->isas->remove(this->isas, &key);
+	if (entry)
+	{
+		entry->isa.spi = 0;
+		if (!entry->osa.spi)
+		{
+			destroy = TRUE;
+		}
+	}
 	this->mutex->unlock(this->mutex);
 
+	if (!entry)
+	{
+		this->mutex->lock(this->mutex);
+		entry = this->osas->remove(this->osas, &key);
+		if (entry)
+		{
+			entry->osa.spi = 0;
+			if (!entry->isa.spi)
+			{
+				destroy = TRUE;
+			}
+		}
+		this->mutex->unlock(this->mutex);
+	}
+
 	if (entry)
 	{
-		/* keep entry until removal of outbound */
+		if (destroy)
+		{
+			entry_destroy(this, entry);
+		}
 		return SUCCESS;
 	}
-
-	this->mutex->lock(this->mutex);
-	entry = this->osas->remove(this->osas, &key);
-	this->mutex->unlock(this->mutex);
-
-	if (entry)
-	{
-		entry_destroy(this, entry);
-		return SUCCESS;
-	}
-
 	return NOT_FOUND;
 }