Fix possible memory leak and several races in the IPsec policy management

code.

Resurrect the state field in the struct secpolicy, it has
IPSEC_SPSTATE_ALIVE value when security policy linked in the chain,
and IPSEC_SPSTATE_DEAD value in all other cases. This field protects
from trying to unlink one security policy several times from the different
threads.

Take additional reference in the key_flush_spd() to be sure that policy
won't be freed from the different thread while we are sending SPDEXPIRE message.

Add KEY_FREESP() call to the key_unlink() to release additional reference
that we take when use key_getsp*() functions.

Differential Revision:	https://reviews.freebsd.org/D1914
Tested by:		Emeric POUPON <emeric.poupon at stormshield dot eu>
Reviewed by:	hrs
Sponsored by:	Yandex LLC

2 files changed, 18 insertions, 1 deletions

diff --git a/sys/netipsec/ipsec.h b/sys/netipsec/ipsec.h
index 5e3e1c3..226ef70 100644
--- a/sys/netipsec/ipsec.h
+++ b/sys/netipsec/ipsec.h
@@ -89,6 +89,9 @@ struct secpolicy {
 				/* if policy == IPSEC else this value == NULL.*/
 	u_int refcnt;			/* reference count */
 	u_int policy;			/* policy_type per pfkeyv2.h */
+	u_int state;
+#define	IPSEC_SPSTATE_DEAD	0
+#define	IPSEC_SPSTATE_ALIVE	1
 	u_int32_t id;			/* It's unique number on the system. */
 	/*
 	 * lifetime handler.
diff --git a/sys/netipsec/key.c b/sys/netipsec/key.c
index c38dae8..ef06ef4 100644
--- a/sys/netipsec/key.c
+++ b/sys/netipsec/key.c
@@ -1198,8 +1198,14 @@ key_unlink(struct secpolicy *sp)
 	SPTREE_UNLOCK_ASSERT();
 
 	SPTREE_WLOCK();
+	if (sp->state == IPSEC_SPSTATE_DEAD) {
+		SPTREE_WUNLOCK();
+		return;
+	}
+	sp->state = IPSEC_SPSTATE_DEAD;
 	TAILQ_REMOVE(&V_sptree[sp->spidx.dir], sp, chain);
 	SPTREE_WUNLOCK();
+	KEY_FREESP(&sp);
 }
 
 /*
@@ -1900,6 +1906,7 @@ key_spdadd(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 
 	SPTREE_WLOCK();
 	TAILQ_INSERT_TAIL(&V_sptree[newsp->spidx.dir], newsp, chain);
+	newsp->state = IPSEC_SPSTATE_ALIVE;
 	SPTREE_WUNLOCK();
 
 	/* delete the entry in spacqtree */
@@ -2335,6 +2342,12 @@ key_spdflush(struct socket *so, struct mbuf *m, const struct sadb_msghdr *mhp)
 	for (dir = 0; dir < IPSEC_DIR_MAX; dir++) {
 		TAILQ_CONCAT(&drainq, &V_sptree[dir], chain);
 	}
+	/*
+	 * We need to set state to DEAD for each policy to be sure,
+	 * that another thread won't try to unlink it.
+	 */
+	TAILQ_FOREACH(sp, &drainq, chain)
+		sp->state = IPSEC_SPSTATE_DEAD;
 	SPTREE_WUNLOCK();
 	sp = TAILQ_FIRST(&drainq);
 	while (sp != NULL) {
@@ -4209,9 +4222,10 @@ restart:
 			    now - sp->created > sp->lifetime) ||
 			    (sp->validtime &&
 			    now - sp->lastused > sp->validtime)) {
+				SP_ADDREF(sp);
 				SPTREE_RUNLOCK();
-				key_unlink(sp);
 				key_spdexpire(sp);
+				key_unlink(sp);
 				KEY_FREESP(&sp);
 				goto restart;
 			}