⬢ DragonFlyBSD Kernel Audit
DF-3433 / fix.diff
← back to finding ↓ download raw
--- a/sys/net/stf/if_stf.c
+++ b/sys/net/stf/if_stf.c
@@ -209,6 +209,20 @@
 		break;
 	case MOD_UNLOAD:
 		sc = stf;
+		/*
+		 * DF-3433: tearing the softc down while the interface is
+		 * configured (IFF_UP) races every in-flight consumer of
+		 * the softc: the lock-free encaptab walk in encap4_input()
+		 * (ip_encap.c), the asynchronous IPv6 input of already
+		 * decapsulated packets (m_pkthdr.rcvif = &sc->sc_if
+		 * survives netisr_queue), and the nd6 teardown itself.
+		 * stf_encapcheck() only matches while IFF_UP is set, so
+		 * refuse to unload a configured interface; the admin must
+		 * take it down first (which stops new matches and lets the
+		 * serializers drain).
+		 */
+		if (sc->sc_if.if_flags & IFF_UP)
+			return (EBUSY);
 		bpfdetach(&sc->sc_if);
 		if_detach(&sc->sc_if);
 		err = encap_detach(sc->encap_cookie);
--- a/sys/netinet/ip_encap.c
+++ b/sys/netinet/ip_encap.c
@@ -92,6 +92,13 @@
 #include <sys/thread2.h>
 MALLOC_DEFINE(M_IPENCAP, "IP Encapsulation", "IP Encapsulation");
 
+/*
+ * The encaptab walk in encap4_input()/encap6_input() runs from netisr
+ * transport processing, i.e. with mp_token held.  Mutators must take
+ * mp_token too so they cannot unlink+free an entry (or the caller's
+ * softc, via ep->arg) while a walk is in flight (DF-3425, DF-3433).
+ */
+
 static void encap_add (struct encaptab *);
 static int mask_match (const struct encaptab *, const struct sockaddr *,
 		const struct sockaddr *);
@@ -326,6 +333,7 @@
 {
 	struct encaptab *ep;
 
+	lwkt_gettoken(&mp_token);
 	crit_enter();
 	/* sanity check on args */
 	if (sp->sa_len > sizeof ep->src || dp->sa_len > sizeof ep->dst)
@@ -369,10 +377,12 @@
 	encap_add(ep);
 
 	crit_exit();
+	lwkt_reltoken(&mp_token);
 	return ep;
 
 fail:
 	crit_exit();
+	lwkt_reltoken(&mp_token);
 	return NULL;
 }
 
@@ -383,6 +393,7 @@
 {
 	struct encaptab *ep;
 
+	lwkt_gettoken(&mp_token);
 	crit_enter();
 	/* sanity check on args */
 	if (!func)
@@ -401,10 +412,12 @@
 	encap_add(ep);
 
 	crit_exit();
+	lwkt_reltoken(&mp_token);
 	return ep;
 
 fail:
 	crit_exit();
+	lwkt_reltoken(&mp_token);
 	return NULL;
 }
 
@@ -414,14 +427,17 @@
 	const struct encaptab *ep = cookie;
 	struct encaptab *p;
 
+	lwkt_gettoken(&mp_token);
 	for (p = LIST_FIRST(&encaptab); p; p = LIST_NEXT(p, chain)) {
 		if (p == ep) {
 			LIST_REMOVE(p, chain);
 			kfree(p, M_IPENCAP);	/*XXX*/
+			lwkt_reltoken(&mp_token);
 			return 0;
 		}
 	}
 
+	lwkt_reltoken(&mp_token);
 	return EINVAL;
 }
 
--- a/sys/net/ipfw3_basic/ip_fw3_state.c
+++ b/sys/net/ipfw3_basic/ip_fw3_state.c
@@ -315,9 +315,26 @@
 		}
 	}
 	if (*the_count <= the_max) {
+		s = RB_FIND(fw3_state_tree, the_tree, k);
+		if (s != NULL) {
+			/*
+			 * Already tracked: limit-type rules (arg3 != 0)
+			 * skip the RB_FIND above, so re-check here instead
+			 * of allocating a duplicate the tree will reject
+			 * (DF-3434: the rejected allocation was leaked and
+			 * kprintf("oops") fired once per packet).
+			 */
+			s->timestamp = time_uptime;
+			goto done;
+		}
 		(*the_count)++;
 		s = kmalloc(LEN_FW3_STATE, M_IPFW3_STATE,
 				M_INTWAIT | M_NULLOK | M_ZERO);
+		if (s == NULL) {
+			/* DF-3435: M_NULLOK permits NULL at the type
+			 * limit; do not write through it */
+			goto done;
+		}
 		s->src_addr = k->src_addr;
 		s->dst_addr = k->dst_addr;
 		s->src_port = k->src_port;
@@ -325,7 +342,7 @@
 		s->stub = *f;
 		s->timestamp = time_uptime;
 		if (RB_INSERT(fw3_state_tree, the_tree, s)) {
-			kprintf("oops\n");
+			kfree(s, M_IPFW3_STATE);
 		}
 	}
 done:
@@ -542,6 +559,11 @@
 	struct ipfw3_state_context *state_ctx = fw3_state_ctx[mycpuid];
 	struct ipfw3_state *s, *tmp;
 
+	if (state_ctx == NULL) {
+		/* DF-3436: fini_dispatch may have run first on this cpu */
+		netisr_forwardmsg_all(&nmsg->base, mycpuid + 1);
+		return;
+	}
 	RB_FOREACH_SAFE(s, fw3_state_tree, &state_ctx->rb_icmp_in, tmp) {
 		if (time_uptime - s->timestamp > sysctl_var_icmp_timeout) {
 			RB_REMOVE(fw3_state_tree, &state_ctx->rb_icmp_in, s);
@@ -688,7 +710,9 @@
 		0, ip_fw3_state_fini_dispatch);
 
 	netisr_domsg(&msg, 0);
-	callout_stop(&ip_fw3_state_cleanup_callout);
+	/* DF-3436: the handler re-arms itself; cancel synchronously and
+	 * disconnect the callout (re-initialized by the next MOD_LOAD) */
+	callout_terminate(&ip_fw3_state_cleanup_callout);
 }
 
 void