⬢ DragonFlyBSD Kernel Audit
DF-3453 / fix.diff
← back to finding ↓ download raw
--- a/sys/net/pfil.c
+++ b/sys/net/pfil.c
@@ -44,6 +44,8 @@
 #include <sys/queue.h>
 #include <sys/sysctl.h>
 
+#include <sys/msgport.h>
+#include <sys/msgport2.h>
+
 #include <net/if.h>
 #include <net/if_var.h>
 #include <net/pfil.h>
@@ -66,6 +68,17 @@ struct netmsg_pfil {
 	struct pfil_head	*pfil_ph;
 };
 
+/*
+ * DF-3453: serializes hook-list walks (pfil_run_hooks) against the COW
+ * list replacement and the post-sync free performed by
+ * pfil_{add,remove}_hook_dispatch.  netmsg_service_sync() only
+ * synchronizes with the netisr threads (netisr.c registers exactly
+ * those in netreglist), but pfil walks also execute on other threads:
+ *  - softclock: ip_mroute tbf_reprocess_q callout -> tbf_process_q ->
+ *    tbf_send_packet -> ip_output -> pfil_run_hooks(PFIL_OUT)
+ *  - driver interrupt threads with IFF_IDIRECT (ether_input_handler
+ *    direct dispatch -> bridge_pfil -> pfil_run_hooks(PFIL_IN))
+ * A walk in flight on one of those threads keeps iterating the old
+ * list after pfil_list_free() recycles it -> use-after-free with an
+ * indirect call through pfh->pfil_func.
+ */
+static struct lwkt_token pfil_token = LWKT_TOKEN_INITIALIZER(pfil_token);
+
 static LIST_HEAD(, pfil_head) pfil_head_list =
 	LIST_HEAD_INITIALIZER(&pfil_head_list);
 
@@ -106,11 +119,13 @@ pfil_run_hooks(struct pfil_head *ph, struct mbuf **mp, struct ifnet *ifp,
 
 	/* Make sure 'list' is really used. */
 	cpu_ccfence();
+	lwkt_gettoken(&pfil_token);
 	TAILQ_FOREACH(pfh, list, pfil_link) {
 		if (pfh->pfil_func != NULL) {
 			rv = pfh->pfil_func(pfh->pfil_arg, &m, ifp, dir);
 			if (rv != 0 || m == NULL)
 				break;
 		}
 	}
+	lwkt_reltoken(&pfil_token);
 
 	*mp = m;
 	return (rv);
@@ -208,6 +223,8 @@ pfil_add_hook_dispatch(netmsg_t nmsg)
 	/*
 	 * Duplicate the requested lists, install new hooks
 	 * on the duplication
 	 */
+	lwkt_gettoken(&pfil_token);
+
 	if (flags & PFIL_IN) {
 		list_in = pfil_list_alloc();
 		pfil_list_dup(ph->ph_in, list_in, NULL);
@@ -242,6 +259,8 @@ pfil_add_hook_dispatch(netmsg_t nmsg)
 	if (old_list_in != NULL)
 		pfil_list_free(old_list_in);
 	if (old_list_out != NULL)
 		pfil_list_free(old_list_out);
+
+	lwkt_reltoken(&pfil_token);
 reply:
 	lwkt_replymsg(&nmsg->base.lmsg, err);
 }
@@ -314,6 +333,8 @@ pfil_remove_hook_dispatch(netmsg_t nmsg)
 	/*
 	 * Duplicate the requested lists, but the pfil hook to
 	 * be deleted is not copied
 	 */
+	lwkt_gettoken(&pfil_token);
+
 	if (flags & PFIL_IN) {
 		KKASSERT(skip_in != NULL);
 		list_in = pfil_list_alloc();
@@ -350,6 +371,8 @@ pfil_remove_hook_dispatch(netmsg_t nmsg)
 	if (old_list_in != NULL)
 		pfil_list_free(old_list_in);
 	if (old_list_out != NULL)
 		pfil_list_free(old_list_out);
+
+	lwkt_reltoken(&pfil_token);
 reply:
 	lwkt_replymsg(&nmsg->base.lmsg, err);
 }