DragonFlyBSD Kernel Audit
DF-0706 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/net/ipfw3_basic/ip_fw3_sync.c b/sys/net/ipfw3_basic/ip_fw3_sync.c
--- a/sys/net/ipfw3_basic/ip_fw3_sync.c
+++ b/sys/net/ipfw3_basic/ip_fw3_sync.c
@@ -192,6 +192,14 @@
 	struct sockaddr *sa;
 	int error, flags, *type;
 
+	/*
+	 * DF-0706 (a): cache the socket pointer locally.  The handler
+	 * typically outlives any individual config ioctl, and the cleanup
+	 * at the bottom of this routine MUST release the same socket we
+	 * were receiving on; otherwise a concurrent ip_fw3_ctl_sync_edge_conf
+	 * that replaced fw3_sync_ctx.edge_sock would cause us to sofree()
+	 * the *new* socket and leak the original.
+	 */
 	so = fw3_sync_ctx.edge_sock;
 	flags = MSG_FBLOCKING;
 
@@ -206,13 +214,26 @@
 		if (error)
 			break;
 		m = sio.sb_mb;
+		if (m == NULL || m->m_len < sizeof(int)) {
+			/* DF-0704: drop short datagrams */
+			kprintf("ipfw3sync: ignore short datagram\n");
+			continue;
+		}
 		type = (int *)m->m_data;
 		if (*type == SYNC_TYPE_SEND_TEST) {
 			struct cmd_send_test *cmd;
+			if (m->m_len < sizeof(*cmd)) {
+				kprintf("ipfw3sync: short SEND_TEST\n");
+				continue;
+			}
 			cmd = (struct cmd_send_test *)m->m_data;
 			kprintf("test received %d\n", cmd->num);
 		} else if (*type == SYNC_TYPE_SEND_STATE) {
 			struct cmd_send_state *cmd;
+			if (m->m_len < sizeof(*cmd)) {
+				kprintf("ipfw3sync: short SEND_STATE\n");
+				continue;
+			}
 			cmd = (struct cmd_send_state *)m->m_data;
 			if (ipfw_sync_install_state_prt != NULL) {
 				(*ipfw_sync_install_state_prt)(cmd);
@@ -224,8 +245,16 @@
 			kprintf("Error ignore\n");
 		}
 	}
-	soshutdown(fw3_sync_ctx.edge_sock, SHUT_RD);
-	sofree(fw3_sync_ctx.edge_sock);
+	/*
+	 * DF-0706 (a)+(b): use the cached socket, and only drop the single
+	 * reference that soreference() added in ip_fw3_ctl_sync_edge_start.
+	 * The previous code called soshutdown+sofree on fw3_sync_ctx.edge_sock
+	 * which (i) hit the wrong socket if edge_conf replaced it while we
+	 * were running, and (ii) when combined with ip_fw3_ctl_sync_edge_stop's
+	 * soclose() dropped two refs for one soreference().
+	 */
+	soshutdown(so, SHUT_RD);
+	sofree(so);
 	kthread_exit();
 }
 
@@ -350,9 +379,25 @@
 int
 ip_fw3_ctl_sync_edge_stop(struct sockopt *sopt)
 {
+	/*
+	 * DF-0706 (b): previously called soclose(edge_sock).  The edge
+	 * socket is owned by the edge_socket_handler thread which already
+	 * drops the soreference() added by ip_fw3_ctl_sync_edge_start via
+	 * sofree() on exit.  Calling soclose() here dropped a second
+	 * reference, breaking the (single) refcount added at start.
+	 *
+	 * The correct way to stop the edge is to clear the running bit and
+	 * shut down the receive side; the handler will return from
+	 * so_pru_soreceive, perform soshutdown+sofree on the cached socket,
+	 * and exit.  The caller is expected to kthread_join(edge_td).
+	 */
 	if (fw3_sync_ctx.running & 1) {
 		fw3_sync_ctx.running &= 2;
-		soclose(fw3_sync_ctx.edge_sock, 0);
+		soshutdown(fw3_sync_ctx.edge_sock, SHUT_RD);
+		if (fw3_sync_ctx.edge_td != NULL) {
+			kthread_join(fw3_sync_ctx.edge_td);
+			fw3_sync_ctx.edge_td = NULL;
+		}
 	}
 	return 0;
 }
@@ -474,18 +519,48 @@
 			ipfw_sync_send_state_prt = ip_fw3_sync_send_state;
 			break;
 		case MOD_UNLOAD:
+			/*
+			 * DF-0706 (d): clear the exported function pointers
+			 * FIRST so no other thread can dispatch into code we
+			 * are about to unmap.  Without these NULLs the
+			 * pointers remain dangling after module unload.
+			 */
+			ipfw_sync_send_state_prt = NULL;
+			ipfw_sync_install_state_prt = NULL;
+
 			if (fw3_sync_ctx.edges != NULL) {
 				kfree(fw3_sync_ctx.edges, M_IPFW3_SYNC);
+				fw3_sync_ctx.edges = NULL;
 			}
+			/*
+			 * DF-0706 (b)+(c): shut down the receive side so the
+			 * handler thread returns from so_pru_soreceive, then
+			 * kthread_join() it before completing the unload.
+			 * The previous code called soclose() and immediately
+			 * NULLed edge_td, leaving the handler running inside
+			 * module code that was about to be unmapped.
+			 */
 			if (fw3_sync_ctx.running & 1) {
-				fw3_sync_ctx.running = 0;
-				soclose(fw3_sync_ctx.edge_sock, 0);
-				fw3_sync_ctx.edge_td = NULL;
+				fw3_sync_ctx.running &= ~1;
+				if (fw3_sync_ctx.edge_sock != NULL)
+					soshutdown(fw3_sync_ctx.edge_sock,
+						   SHUT_RD);
+				if (fw3_sync_ctx.edge_td != NULL) {
+					kthread_join(fw3_sync_ctx.edge_td);
+					fw3_sync_ctx.edge_td = NULL;
+				}
+				if (fw3_sync_ctx.edge_sock != NULL) {
+					sofree(fw3_sync_ctx.edge_sock);
+					fw3_sync_ctx.edge_sock = NULL;
+				}
 			}
 			if (fw3_sync_ctx.running & 2) {
 				int i;
+				fw3_sync_ctx.running &= ~2;
 				for (i = 0; i < fw3_sync_ctx.count; i++) {
-					soclose(fw3_sync_ctx.centre_socks[i], 0);
+					if (fw3_sync_ctx.centre_socks[i] != NULL)
+						soclose(
+						  fw3_sync_ctx.centre_socks[i], 0);
 				}
 			}
 			break;