DragonFlyBSD Kernel Audit
DF-0315 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/net/wg/if_wg.c b/sys/net/wg/if_wg.c
--- a/sys/net/wg/if_wg.c
+++ b/sys/net/wg/if_wg.c
@@ -41,6 +41,7 @@
 #include <sys/sockio.h> /* SIOC* ioctl commands */
 #include <sys/taskqueue.h>
 #include <sys/time.h>
+#include <sys/refcount.h>
 
 #include <machine/atomic.h>
 
@@ -261,6 +262,8 @@
 	uint64_t		*p_tx_bytes;
 	uint64_t		*p_rx_bytes;
 
+	u_int			 p_refcnt;	/* DF-0315: peer lifetime refcount */
+
 	LIST_HEAD(, wg_aip)	 p_aips;
 	size_t			 p_aips_num;
 };
@@ -588,6 +591,9 @@
 
 	peer = kmalloc(sizeof(*peer), M_WG, M_WAITOK | M_ZERO);
 
+	/* DF-0315: the create reference (owned by the sc->sc_peers table). */
+	refcount_init(&peer->p_refcnt, 1);
+
 	peer->p_remote = noise_remote_alloc(sc->sc_local, pub_key, peer);
 	if ((*errp = noise_remote_enable(peer->p_remote)) != 0) {
 		noise_remote_free(peer->p_remote);
@@ -636,6 +642,48 @@
 	return (peer);
 }
 
+/*
+ * DF-0315: reference counting for wg_peer.  The data-plane paths
+ * (wg_output/wg_aip_lookup, wg_encrypt/wg_decrypt, wg_deliver_in/out) obtain a
+ * peer and use it WITHOUT holding sc->sc_lock; the previous code kfree()'d the
+ * peer in wg_peer_destroy() while those paths could still be dereferencing it
+ * (the noise_remote refcount protects only the remote, not the peer).  We now
+ * keep the peer alive until the last reference is dropped.
+ */
+struct wg_peer *
+wg_peer_ref(struct wg_peer *peer)
+{
+	refcount_acquire(&peer->p_refcnt);
+	return (peer);
+}
+
+void
+wg_peer_put(struct wg_peer *peer)
+{
+	if (refcount_release(&peer->p_refcnt)) {
+		/*
+		 * Last reference gone.  All data-plane / task references have
+		 * been released, so it is now safe to tear the peer down and
+		 * free it.  No sc_lock is required here.
+		 */
+		wg_queue_deinit(&peer->p_decrypt_serial);
+		wg_queue_deinit(&peer->p_encrypt_serial);
+		wg_queue_deinit(&peer->p_stage_queue);
+
+		kfree(peer->p_tx_bytes, M_WG);
+		kfree(peer->p_rx_bytes, M_WG);
+
+		lockuninit(&peer->p_endpoint_lock);
+		lockuninit(&peer->p_handshake_mtx);
+
+		noise_remote_free(peer->p_remote);
+		cookie_maker_free(peer->p_cookie);
+
+		DPRINTF(peer->p_sc, "Peer %ld freed\n", peer->p_id);
+		kfree(peer, M_WG);
+	}
+}
+
 static void
 wg_peer_destroy(struct wg_peer *peer)
 {
@@ -644,52 +692,33 @@
 	KKASSERT(lockstatus(&sc->sc_lock, curthread) == LK_EXCLUSIVE);
 
 	/*
-	 * Disable remote and timers.  This will prevent any new handshakes
-	 * from occuring.
+	 * Disable remote and timers, and remove the peer's allowed IPs and its
+	 * sc_peers entry.  After this no NEW data-plane references can be taken
+	 * (wg_aip_lookup() will no longer find it), so once the in-flight ones
+	 * are dropped the peer will be freed by wg_peer_put().
 	 */
 	noise_remote_disable(peer->p_remote);
 	wg_timers_disable(peer);
 
-	/*
-	 * Remove all allowed IPs, so no more packets will be routed to
-	 * this peer.
-	 */
 	wg_aip_remove_all(sc, peer);
 
-	/* Remove peer from the interface, then free. */
 	sc->sc_peers_num--;
 	TAILQ_REMOVE(&sc->sc_peers, peer, p_entry);
 
-	/*
-	 * While there are no references remaining, we may still have
-	 * p_{send,recv}_task executing (think empty queue, but
-	 * wg_deliver_{in,out} needs to check the queue).  We should wait
-	 * for them and then free.
-	 */
-	taskqueue_drain(peer->p_recv_taskqueue, &peer->p_recv_task);
-	taskqueue_drain(peer->p_send_taskqueue, &peer->p_send_task);
-
 	callout_terminate(&peer->p_new_handshake);
 	callout_terminate(&peer->p_send_keepalive);
 	callout_terminate(&peer->p_retry_handshake);
 	callout_terminate(&peer->p_persistent_keepalive);
 	callout_terminate(&peer->p_zero_key_material);
 
-	wg_queue_deinit(&peer->p_decrypt_serial);
-	wg_queue_deinit(&peer->p_encrypt_serial);
-	wg_queue_deinit(&peer->p_stage_queue);
-
-	kfree(peer->p_tx_bytes, M_WG);
-	kfree(peer->p_rx_bytes, M_WG);
-
-	lockuninit(&peer->p_endpoint_lock);
-	lockuninit(&peer->p_handshake_mtx);
-
-	noise_remote_free(peer->p_remote);
-	cookie_maker_free(peer->p_cookie);
+	taskqueue_drain(peer->p_recv_taskqueue, &peer->p_recv_task);
+	taskqueue_drain(peer->p_send_taskqueue, &peer->p_send_task);
 
 	DPRINTF(sc, "Peer %ld destroyed\n", peer->p_id);
-	kfree(peer, M_WG);
+
+	/* Drop the table's reference; frees the peer once all data-plane
+	 * references held by wg_output()/wg_aip_lookup() etc. are released. */
+	wg_peer_put(peer);
 }
 
 static void
@@ -878,6 +907,9 @@
 	if (node != NULL) {
 		peer = ((struct wg_aip *)node)->a_peer;
 		noise_remote_ref(peer->p_remote);
+		/* DF-0315: keep the peer (not just its remote) alive for the
+		 * caller, which uses it without holding sc_lock. */
+		wg_peer_ref(peer);
 	} else {
 		peer = NULL;
 	}
@@ -1848,6 +1880,7 @@
 
 	remote = noise_keypair_remote(pkt->p_keypair);
 	peer = noise_remote_arg(remote);
+	wg_peer_ref(peer);	/* DF-0315: hold peer ref for the encrypt path */
 	m = pkt->p_mbuf;
 
 	padlen = calculate_padding(pkt);
@@ -1872,6 +1905,7 @@
 	atomic_store_rel_int(&pkt->p_state, state);
 	taskqueue_enqueue(peer->p_send_taskqueue, &peer->p_send_task);
 	noise_remote_put(remote);
+	wg_peer_put(peer);	/* DF-0315 */
 }
 
 static void
@@ -1885,6 +1919,7 @@
 
 	remote = noise_keypair_remote(pkt->p_keypair);
 	peer = noise_remote_arg(remote);
+	wg_peer_ref(peer);	/* DF-0315: hold peer ref for the decrypt path */
 	m = pkt->p_mbuf;
 
 	pkt->p_counter = le64toh(mtod(m, struct wg_pkt_data *)->counter);
@@ -1922,8 +1957,10 @@
 	}
 
 	/* Drop the reference, since no need to dereference it. */
-	if (allowed_peer != NULL)
+	if (allowed_peer != NULL) {
 		noise_remote_put(allowed_peer->p_remote);
+		wg_peer_put(allowed_peer);	/* DF-0315: wg_aip_lookup() ref */
+	}
 
 	if (__predict_false(peer != allowed_peer)) {
 		DPRINTF(sc, "Packet has disallowed src IP from peer %ld\n",
@@ -1938,6 +1975,7 @@
 	atomic_store_rel_int(&pkt->p_state, state);
 	taskqueue_enqueue(peer->p_recv_taskqueue, &peer->p_recv_task);
 	noise_remote_put(remote);
+	wg_peer_put(peer);	/* DF-0315 */
 }
 
 static void
@@ -2186,12 +2224,14 @@
 
 		remote = noise_keypair_remote(pkt->p_keypair);
 		peer = noise_remote_arg(remote);
+		wg_peer_ref(peer);	/* DF-0315: hold peer ref across queue/decrypt */
 		if (!wg_queue_both(&sc->sc_decrypt_parallel,
 				   &peer->p_decrypt_serial, pkt))
 			IFNET_STAT_INC(sc->sc_ifp, iqdrops, 1);
 
 		wg_decrypt_dispatch(sc);
 		noise_remote_put(remote);
+		wg_peer_put(peer);	/* DF-0315 */
 		return;
 	}
 
@@ -2350,6 +2390,7 @@
 	wg_queue_push_staged(&peer->p_stage_queue, pkt);
 	wg_peer_send_staged(peer);
 	noise_remote_put(peer->p_remote);
+	wg_peer_put(peer);	/* DF-0315: drop the wg_aip_lookup() peer ref */
 
 	return (0);
 
@@ -2374,8 +2415,10 @@
 		pkt->p_mbuf = NULL; /* m already freed above */
 		wg_packet_free(pkt);
 	}
-	if (peer != NULL)
+	if (peer != NULL) {
 		noise_remote_put(peer->p_remote);
+		wg_peer_put(peer);	/* DF-0315: drop the wg_aip_lookup() peer ref */
+	}
 	return (ret);
 }