From: bluhm <bluhm@openbsd.org>
Date: Sat, 11 Dec 2021 16:33:46 +0000 (+0000)
Subject: Protect the write access to the TDB flags field with a mutex per
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=59b9936bc2964483a405b05916023a07d1536ce6;p=openbsd

Protect the write access to the TDB flags field with a mutex per
TDB.  Clearing the timeout flags just before pool put in tdb_free()
does not make sense.  Move this to tdb_delete().  While there make
the parentheses in the flag check consistent.
tested by Hrvoje Popovski; OK tobhe@
---

diff --git a/sys/net/pfkeyv2_convert.c b/sys/net/pfkeyv2_convert.c
index 0cd94552d53..2d648d5a7eb 100644
--- a/sys/net/pfkeyv2_convert.c
+++ b/sys/net/pfkeyv2_convert.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfkeyv2_convert.c,v 1.76 2021/11/25 13:46:02 bluhm Exp $	*/
+/*	$OpenBSD: pfkeyv2_convert.c,v 1.77 2021/12/11 16:33:46 bluhm Exp $	*/
 /*
  * The author of this code is Angelos D. Keromytis (angelos@keromytis.org)
  *
@@ -122,6 +122,7 @@ import_sa(struct tdb *tdb, struct sadb_sa *sadb_sa, struct ipsecinit *ii)
 	if (!sadb_sa)
 		return;
 
+	mtx_enter(&tdb->tdb_mtx);
 	if (ii) {
 		ii->ii_encalg = sadb_sa->sadb_sa_encrypt;
 		ii->ii_authalg = sadb_sa->sadb_sa_auth;
@@ -145,6 +146,7 @@ import_sa(struct tdb *tdb, struct sadb_sa *sadb_sa, struct ipsecinit *ii)
 
 	if (sadb_sa->sadb_sa_state != SADB_SASTATE_MATURE)
 		tdb->tdb_flags |= TDBF_INVALID;
+	mtx_leave(&tdb->tdb_mtx);
 }
 
 /*
@@ -282,6 +284,7 @@ import_lifetime(struct tdb *tdb, struct sadb_lifetime *sadb_lifetime, int type)
 	if (!sadb_lifetime)
 		return;
 
+	mtx_enter(&tdb->tdb_mtx);
 	switch (type) {
 	case PFKEYV2_LIFETIME_HARD:
 		if ((tdb->tdb_exp_allocations =
@@ -348,6 +351,7 @@ import_lifetime(struct tdb *tdb, struct sadb_lifetime *sadb_lifetime, int type)
 		tdb->tdb_established = sadb_lifetime->sadb_lifetime_addtime;
 		tdb->tdb_first_use = sadb_lifetime->sadb_lifetime_usetime;
 	}
+	mtx_leave(&tdb->tdb_mtx);
 }
 
 /*
diff --git a/sys/netinet/ip_ah.c b/sys/netinet/ip_ah.c
index 009f5c1c9c3..63ef3eb5edf 100644
--- a/sys/netinet/ip_ah.c
+++ b/sys/netinet/ip_ah.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ah.c,v 1.168 2021/12/02 12:39:15 bluhm Exp $ */
+/*	$OpenBSD: ip_ah.c,v 1.169 2021/12/11 16:33:46 bluhm Exp $ */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr) and
@@ -612,8 +612,8 @@ ah_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
 	ahstat_add(ahs_ibytes, ibytes);
 
 	/* Hard expiration. */
-	if (tdb->tdb_flags & TDBF_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes) {
+	if ((tdb->tdb_flags & TDBF_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes)) {
 		ipsecstat_inc(ipsec_exctdb);
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_HARD);
 		tdb_delete(tdb);
@@ -621,11 +621,15 @@ ah_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
 	}
 
 	/* Notify on expiration. */
-	if (tdb->tdb_flags & TDBF_SOFT_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes) {
+	mtx_enter(&tdb->tdb_mtx);
+	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking. */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 
 	/* Get crypto descriptors. */
 	crp = crypto_getreq(1);
@@ -952,8 +956,8 @@ ah_output(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
 	ahstat_add(ahs_obytes, m->m_pkthdr.len - skip);
 
 	/* Hard expiration. */
-	if (tdb->tdb_flags & TDBF_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes) {
+	if ((tdb->tdb_flags & TDBF_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes)) {
 		ipsecstat_inc(ipsec_exctdb);
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_HARD);
 		tdb_delete(tdb);
@@ -962,11 +966,15 @@ ah_output(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
 	}
 
 	/* Notify on expiration. */
-	if (tdb->tdb_flags & TDBF_SOFT_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes) {
+	mtx_enter(&tdb->tdb_mtx);
+	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES; /* Turn off checking */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 
 	/*
 	 * Loop through mbuf chain; if we find a readonly mbuf,
diff --git a/sys/netinet/ip_esp.c b/sys/netinet/ip_esp.c
index 4c8df549f8c..1aa4d4b58e8 100644
--- a/sys/netinet/ip_esp.c
+++ b/sys/netinet/ip_esp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_esp.c,v 1.188 2021/11/21 16:17:48 mvs Exp $ */
+/*	$OpenBSD: ip_esp.c,v 1.189 2021/12/11 16:33:47 bluhm Exp $ */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr) and
@@ -433,11 +433,15 @@ esp_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
 	}
 
 	/* Notify on soft expiration */
+	mtx_enter(&tdb->tdb_mtx);
 	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
 	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;       /* Turn off checking */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 
 	/* Get crypto descriptors */
 	crp = crypto_getreq(esph && espx ? 2 : 1);
@@ -781,8 +785,8 @@ esp_output(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
 	espstat_add(esps_obytes, m->m_pkthdr.len - skip);
 
 	/* Hard byte expiration. */
-	if (tdb->tdb_flags & TDBF_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes) {
+	if ((tdb->tdb_flags & TDBF_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_exp_bytes)) {
 		ipsecstat_inc(ipsec_exctdb);
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_HARD);
 		tdb_delete(tdb);
@@ -791,11 +795,15 @@ esp_output(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
 	}
 
 	/* Soft byte expiration. */
-	if (tdb->tdb_flags & TDBF_SOFT_BYTES &&
-	    tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes) {
+	mtx_enter(&tdb->tdb_mtx);
+	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
+	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;    /* Turn off checking. */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 
 	/*
 	 * Loop through mbuf chain; if we find a readonly mbuf,
diff --git a/sys/netinet/ip_ipcomp.c b/sys/netinet/ip_ipcomp.c
index 3bfb99c8daf..65026466d48 100644
--- a/sys/netinet/ip_ipcomp.c
+++ b/sys/netinet/ip_ipcomp.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ip_ipcomp.c,v 1.88 2021/11/21 16:17:48 mvs Exp $ */
+/* $OpenBSD: ip_ipcomp.c,v 1.89 2021/12/11 16:33:47 bluhm Exp $ */
 
 /*
  * Copyright (c) 2001 Jean-Jacques Bernard-Gundol (jj@wabbitt.org)
@@ -205,11 +205,15 @@ ipcomp_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
 		goto drop;
 	}
 	/* Notify on soft expiration */
+	mtx_enter(&tdb->tdb_mtx);
 	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
 	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;	/* Turn off checking */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 
 	/* In case it's not done already, adjust the size of the mbuf chain */
 	m->m_pkthdr.len = clen + hlen + skip;
@@ -393,12 +397,18 @@ ipcomp_output(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
 		error = EINVAL;
 		goto drop;
 	}
+
 	/* Soft byte expiration */
+	mtx_enter(&tdb->tdb_mtx);
 	if ((tdb->tdb_flags & TDBF_SOFT_BYTES) &&
 	    (tdb->tdb_cur_bytes >= tdb->tdb_soft_bytes)) {
+		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;  /* Turn off checking */
+		mtx_leave(&tdb->tdb_mtx);
+		/* may sleep in solock() for the pfkey socket */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_BYTES;	/* Turn off checking */
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
+
 	/*
 	 * Loop through mbuf chain; if we find a readonly mbuf,
 	 * copy the packet.
diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c
index 5146dcbf248..971112ea86b 100644
--- a/sys/netinet/ip_ipsp.c
+++ b/sys/netinet/ip_ipsp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ipsp.c,v 1.263 2021/12/08 14:24:18 bluhm Exp $	*/
+/*	$OpenBSD: ip_ipsp.c,v 1.264 2021/12/11 16:33:47 bluhm Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -311,11 +311,13 @@ reserve_spi(u_int rdomain, u_int32_t sspi, u_int32_t tspi,
 #ifdef IPSEC
 		/* Setup a "silent" expiration (since TDBF_INVALID's set). */
 		if (ipsec_keep_invalid > 0) {
+			mtx_enter(&tdbp->tdb_mtx);
 			tdbp->tdb_flags |= TDBF_TIMER;
 			tdbp->tdb_exp_timeout = ipsec_keep_invalid;
 			if (timeout_add_sec(&tdbp->tdb_timer_tmo,
 			    ipsec_keep_invalid))
 				tdb_ref(tdbp);
+			mtx_leave(&tdbp->tdb_mtx);
 		}
 #endif
 
@@ -701,11 +703,14 @@ tdb_soft_timeout(void *v)
 	struct tdb *tdb = v;
 
 	NET_LOCK();
+	mtx_enter(&tdb->tdb_mtx);
 	if (tdb->tdb_flags & TDBF_SOFT_TIMER) {
+		tdb->tdb_flags &= ~TDBF_SOFT_TIMER;
+		mtx_leave(&tdb->tdb_mtx);
 		/* Soft expirations. */
 		pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_TIMER;
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 	/* decrement refcount of the timeout argument */
 	tdb_unref(tdb);
 	NET_UNLOCK();
@@ -717,12 +722,15 @@ tdb_soft_firstuse(void *v)
 	struct tdb *tdb = v;
 
 	NET_LOCK();
+	mtx_enter(&tdb->tdb_mtx);
 	if (tdb->tdb_flags & TDBF_SOFT_FIRSTUSE) {
+		tdb->tdb_flags &= ~TDBF_SOFT_FIRSTUSE;
+		mtx_leave(&tdb->tdb_mtx);
 		/* If the TDB hasn't been used, don't renew it. */
 		if (tdb->tdb_first_use != 0)
 			pfkeyv2_expire(tdb, SADB_EXT_LIFETIME_SOFT);
-		tdb->tdb_flags &= ~TDBF_SOFT_FIRSTUSE;
-	}
+	} else
+		mtx_leave(&tdb->tdb_mtx);
 	/* decrement refcount of the timeout argument */
 	tdb_unref(tdb);
 	NET_UNLOCK();
@@ -958,6 +966,9 @@ tdb_unbundle(struct tdb *tdbp)
 void
 tdb_deltimeouts(struct tdb *tdbp)
 {
+	mtx_enter(&tdbp->tdb_mtx);
+	tdbp->tdb_flags &= ~(TDBF_FIRSTUSE | TDBF_SOFT_FIRSTUSE | TDBF_TIMER |
+	    TDBF_SOFT_TIMER);
 	if (timeout_del(&tdbp->tdb_timer_tmo))
 		tdb_unref(tdbp);
 	if (timeout_del(&tdbp->tdb_first_tmo))
@@ -966,6 +977,7 @@ tdb_deltimeouts(struct tdb *tdbp)
 		tdb_unref(tdbp);
 	if (timeout_del(&tdbp->tdb_sfirst_tmo))
 		tdb_unref(tdbp);
+	mtx_leave(&tdbp->tdb_mtx);
 }
 
 struct tdb *
@@ -1005,9 +1017,13 @@ tdb_dodelete(struct tdb *tdbp, int locked)
 {
 	NET_ASSERT_LOCKED();
 
-	if (tdbp->tdb_flags & TDBF_DELETED)
+	mtx_enter(&tdbp->tdb_mtx);
+	if (tdbp->tdb_flags & TDBF_DELETED) {
+		mtx_leave(&tdbp->tdb_mtx);
 		return;
+	}
 	tdbp->tdb_flags |= TDBF_DELETED;
+	mtx_leave(&tdbp->tdb_mtx);
 	if (locked)
 		tdb_unlink_locked(tdbp);
 	else
@@ -1034,6 +1050,7 @@ tdb_alloc(u_int rdomain)
 	tdbp = pool_get(&tdb_pool, PR_WAITOK | PR_ZERO);
 
 	refcnt_init(&tdbp->tdb_refcnt);
+	mtx_init(&tdbp->tdb_mtx, IPL_SOFTNET);
 	TAILQ_INIT(&tdbp->tdb_policy_head);
 
 	/* Record establishment time. */
@@ -1085,8 +1102,6 @@ tdb_free(struct tdb *tdbp)
 	KASSERT(tdbp->tdb_inext == NULL);
 
 	/* Remove expiration timeouts. */
-	tdbp->tdb_flags &= ~(TDBF_FIRSTUSE | TDBF_SOFT_FIRSTUSE | TDBF_TIMER |
-	    TDBF_SOFT_TIMER);
 	KASSERT(timeout_pending(&tdbp->tdb_timer_tmo) == 0);
 	KASSERT(timeout_pending(&tdbp->tdb_first_tmo) == 0);
 	KASSERT(timeout_pending(&tdbp->tdb_stimer_tmo) == 0);
diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
index f8ccdbf5c24..fd822cfb9f8 100644
--- a/sys/netinet/ip_ipsp.h
+++ b/sys/netinet/ip_ipsp.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ipsp.h,v 1.229 2021/12/08 14:24:18 bluhm Exp $	*/
+/*	$OpenBSD: ip_ipsp.h,v 1.230 2021/12/11 16:33:47 bluhm Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -314,6 +314,7 @@ struct ipsec_policy {
  *	I	immutable after creation
  *	N	net lock
  *	s	tdb_sadb_mtx
+ *	m	tdb_mtx
  */
 struct tdb {				/* tunnel descriptor block */
 	/*
@@ -331,6 +332,7 @@ struct tdb {				/* tunnel descriptor block */
 	struct tdb	*tdb_onext;
 
 	struct refcnt	tdb_refcnt;
+	struct mutex	tdb_mtx;
 
 	const struct xformsw	*tdb_xform;		/* Transform to use */
 	const struct enc_xform	*tdb_encalgxform;	/* Enc algorithm */
@@ -364,7 +366,7 @@ struct tdb {				/* tunnel descriptor block */
 	"\21USEDTUNNEL\22UDPENCAP\23PFSYNC\24PFSYNC_RPL" \
 	"\25ESN")
 
-	u_int32_t	tdb_flags;	/* Flags related to this TDB */
+	u_int32_t	tdb_flags;	/* [m] Flags related to this TDB */
 
 	struct timeout	tdb_timer_tmo;
 	struct timeout	tdb_first_tmo;
diff --git a/sys/netinet/ipsec_output.c b/sys/netinet/ipsec_output.c
index 621755318d0..7d863f1657a 100644
--- a/sys/netinet/ipsec_output.c
+++ b/sys/netinet/ipsec_output.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ipsec_output.c,v 1.93 2021/12/02 12:39:15 bluhm Exp $ */
+/*	$OpenBSD: ipsec_output.c,v 1.94 2021/12/11 16:33:47 bluhm Exp $ */
 /*
  * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu)
  *
@@ -268,7 +268,9 @@ ipsp_process_packet(struct mbuf *m, struct tdb *tdb, int af, int tunalready)
 			}
 
 			/* Remember that we appended a tunnel header. */
+			mtx_enter(&tdb->tdb_mtx);
 			tdb->tdb_flags |= TDBF_USEDTUNNEL;
+			mtx_leave(&tdb->tdb_mtx);
 		}
 	}