git: 4920e38fecc3 - main - ipsec: fix race condition in key.c

[Wojciech Macek]

The branch main has been updated by wma:

URL: https://cgit.FreeBSD.org/src/commit/?id=4920e38fecc3d0274b03ae7151153e9d6b9eb526

commit 4920e38fecc3d0274b03ae7151153e9d6b9eb526
Author:     Wojciech Macek <wma at FreeBSD.org>
AuthorDate: 2021-08-13 10:52:38 +0000
Commit:     Wojciech Macek <wma at FreeBSD.org>
CommitDate: 2021-08-13 10:52:38 +0000

    ipsec: fix race condition in key.c
    
    Small patch that fixes a race condition in sys/netipsec/key.c
    
    Obtained from:          Stormshield
    Differential revision:  https://reviews.freebsd.org/D31271
---
 sys/netipsec/key.c | 53 +++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/sys/netipsec/key.c b/sys/netipsec/key.c
index 3ea3a81b90c3..72c598586d8e 100644
--- a/sys/netipsec/key.c
+++ b/sys/netipsec/key.c
@@ -616,6 +616,7 @@ static struct callout key_timer;
 #endif
 
 static void key_unlink(struct secpolicy *);
+static void key_detach(struct secpolicy *);
 static struct secpolicy *key_do_allocsp(struct secpolicyindex *spidx, u_int dir);
 static struct secpolicy *key_getsp(struct secpolicyindex *);
 static struct secpolicy *key_getspbyid(u_int32_t);
@@ -1200,18 +1201,26 @@ key_freesp(struct secpolicy **spp)
 static void
 key_unlink(struct secpolicy *sp)
 {
+	SPTREE_WLOCK();
+	key_detach(sp);
+	SPTREE_WUNLOCK();
+	if (SPDCACHE_ENABLED())
+		spdcache_clear();
+	key_freesp(&sp);
+}
 
+static void
+key_detach(struct secpolicy *sp)
+{
 	IPSEC_ASSERT(sp->spidx.dir == IPSEC_DIR_INBOUND ||
 	    sp->spidx.dir == IPSEC_DIR_OUTBOUND,
 	    ("invalid direction %u", sp->spidx.dir));
-	SPTREE_UNLOCK_ASSERT();
+	SPTREE_WLOCK_ASSERT();
 
 	KEYDBG(KEY_STAMP,
 	    printf("%s: SP(%p)\n", __func__, sp));
-	SPTREE_WLOCK();
 	if (sp->state != IPSEC_SPSTATE_ALIVE) {
 		/* SP is already unlinked */
-		SPTREE_WUNLOCK();
 		return;
 	}
 	sp->state = IPSEC_SPSTATE_DEAD;
@@ -1219,10 +1228,6 @@ key_unlink(struct secpolicy *sp)
 	V_spd_size--;
 	LIST_REMOVE(sp, idhash);
 	V_sp_genid++;
-	SPTREE_WUNLOCK();
-	if (SPDCACHE_ENABLED())
-		spdcache_clear();
-	key_freesp(&sp);
 }
 
 /*
@@ -1941,7 +1946,7 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 	struct sadb_address *src0, *dst0;
 	struct sadb_x_policy *xpl0, *xpl;
 	struct sadb_lifetime *lft = NULL;
-	struct secpolicy *newsp;
+	struct secpolicy *newsp, *oldsp;
 	int error;
 
 	IPSEC_ASSERT(so != NULL, ("null socket"));
@@ -2019,15 +2024,13 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 	                src0->sadb_address_proto,
 	                &spidx);
 	/* Checking there is SP already or not. */
-	newsp = key_getsp(&spidx);
-	if (newsp != NULL) {
+	oldsp = key_getsp(&spidx);
+	if (oldsp != NULL) {
 		if (mhp->msg->sadb_msg_type == SADB_X_SPDUPDATE) {
 			KEYDBG(KEY_STAMP,
 			    printf("%s: unlink SP(%p) for SPDUPDATE\n",
-				__func__, newsp));
-			KEYDBG(KEY_DATA, kdebug_secpolicy(newsp));
-			key_unlink(newsp);
-			key_freesp(&newsp);
+				__func__, oldsp));
+			KEYDBG(KEY_DATA, kdebug_secpolicy(oldsp));
 		} else {
 			key_freesp(&newsp);
 			ipseclog((LOG_DEBUG,
@@ -2038,6 +2041,10 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 
 	/* allocate new SP entry */
 	if ((newsp = key_msg2sp(xpl0, PFKEY_EXTLEN(xpl0), &error)) == NULL) {
+		if (oldsp != NULL) {
+			key_unlink(oldsp);
+			key_freesp(&oldsp); /* second for our reference */
+		}
 		return key_senderror(so, m, error);
 	}
 
@@ -2046,18 +2053,32 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 	newsp->validtime = lft ? lft->sadb_lifetime_usetime : 0;
 	bcopy(&spidx, &newsp->spidx, sizeof(spidx));
 
-	/* XXXAE: there is race between key_getsp() and key_insertsp() */
 	SPTREE_WLOCK();
 	if ((newsp->id = key_getnewspid()) == 0) {
+		if (oldsp != NULL)
+			key_detach(oldsp);
 		SPTREE_WUNLOCK();
+		if (oldsp != NULL) {
+			key_freesp(&oldsp); /* first for key_detach */
+			IPSEC_ASSERT(oldsp != NULL, ("null oldsp: refcount bug"));
+			key_freesp(&oldsp); /* second for our reference */
+			if (SPDCACHE_ENABLED()) /* refresh cache because of key_detach */
+				spdcache_clear();
+		}
 		key_freesp(&newsp);
 		return key_senderror(so, m, ENOBUFS);
 	}
+	if (oldsp != NULL)
+		key_detach(oldsp);
 	key_insertsp(newsp);
 	SPTREE_WUNLOCK();
+	if (oldsp != NULL) {
+		key_freesp(&oldsp); /* first for key_detach */
+		IPSEC_ASSERT(oldsp != NULL, ("null oldsp: refcount bug"));
+		key_freesp(&oldsp); /* second for our reference */
+	}
 	if (SPDCACHE_ENABLED())
 		spdcache_clear();
-
 	KEYDBG(KEY_STAMP,
 	    printf("%s: SP(%p)\n", __func__, newsp));
 	KEYDBG(KEY_DATA, kdebug_secpolicy(newsp));