DragonFlyBSD Kernel Audit
DF-0633 / fix.diff
← back to finding ↓ download raw
diff --git a/sys/net/ipfw3_basic/ip_fw3_state.c b/sys/net/ipfw3_basic/ip_fw3_state.c
--- a/sys/net/ipfw3_basic/ip_fw3_state.c
+++ b/sys/net/ipfw3_basic/ip_fw3_state.c
@@ -419,119 +419,82 @@
 }
 
 int
-ip_fw3_ctl_state_get(struct sockopt *sopt)
+ip_fw3_ctl_state_get_dispatch(netmsg_t nmsg)
 {
-	struct ipfw3_state_context *state_ctx;
-	struct ipfw3_state *s;
+	struct ipfw3_state_get_msg *m = (struct ipfw3_state_get_msg *)nmsg;
+	struct ipfw3_state_context *state_ctx = fw3_state_ctx[mycpuid];
+	struct ipfw3_state *s, *tmp;
+	struct ipfw3_ioc_state *ioc = m->cur;
+
+	/*
+	 * DF-0633: this dispatch runs on the owning CPU's netisr, so it has
+	 * exclusive access to the per-CPU state trees. Use RB_FOREACH_SAFE
+	 * for additional robustness against concurrent cleanup_dispatch.
+	 */
+#define EMIT(tree, proto) \
+	RB_FOREACH_SAFE(s, fw3_state_tree, &state_ctx->tree, tmp) { \
+		if (m->remaining <= 0) { \
+			m->overflow = 1; \
+			break; \
+		} \
+		ioc->src_addr.s_addr = ntohl(s->src_addr); \
+		ioc->dst_addr.s_addr = ntohl(s->dst_addr); \
+		ioc->src_port = ntohs(s->src_port); \
+		ioc->dst_port = ntohs(s->dst_port); \
+		ioc->cpu_id = mycpuid; \
+		ioc->rule_id = s->stub ? s->stub->rulenum : 0; \
+		ioc->proto = proto; \
+		ioc->life = s->timestamp + sysctl_var_udp_timeout - time_uptime; \
+		ioc++; \
+		m->remaining--; \
+	}
+
+	EMIT(rb_icmp_in,  IPPROTO_ICMP);
+	EMIT(rb_icmp_out, IPPROTO_ICMP);
+	EMIT(rb_tcp_in,   IPPROTO_TCP);
+	EMIT(rb_tcp_out,  IPPROTO_TCP);
+	EMIT(rb_udp_in,   IPPROTO_UDP);
+	EMIT(rb_udp_out,  IPPROTO_UDP);
+#undef EMIT
 
-	size_t sopt_size, total_len = 0;
-	struct ipfw3_ioc_state *ioc;
+	m->cur = ioc;
+	netisr_forwardmsg_all(&nmsg->base, mycpuid + 1);
+}
+
+int
+ip_fw3_ctl_state_get(struct sockopt *sopt)
+{
+	struct ipfw3_ioc_state *user_buf;
+	size_t sopt_size;
 	int ioc_rule_id;
 
 	ioc_rule_id = *((int *)(sopt->sopt_val));
 	sopt_size = sopt->sopt_valsize;
-	ioc = (struct ipfw3_ioc_state *)sopt->sopt_val;
-	/* icmp states only in CPU 0 */
-	int cpu = 0;
-
-	/* icmp states */
-	for (cpu = 0; cpu < ncpus; cpu++) {
-		state_ctx = fw3_state_ctx[cpu];
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_icmp_in) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_ICMP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_icmp_out) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_ICMP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_tcp_in) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_TCP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_tcp_out) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_TCP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_udp_in) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_UDP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-		RB_FOREACH(s, fw3_state_tree, &state_ctx->rb_udp_out) {
-			total_len += LEN_IOC_FW3_STATE;
-			if (total_len > sopt_size)
-				goto nospace;
-			ioc->src_addr.s_addr = ntohl(s->src_addr);
-			ioc->dst_addr.s_addr = ntohl(s->dst_addr);
-			ioc->src_port = ntohs(s->src_port);
-			ioc->dst_port = ntohs(s->dst_port);
-			ioc->cpu_id = cpu;
-			ioc->rule_id = s->stub->rulenum;
-			ioc->proto = IPPROTO_UDP;
-			ioc->life = s->timestamp +
-				sysctl_var_udp_timeout - time_uptime;
-			ioc++;
-		}
-	}
+	user_buf = (struct ipfw3_ioc_state *)sopt->sopt_val;
 
-	sopt->sopt_valsize = total_len;
-	return 0;
-nospace:
+	/*
+	 * DF-0633 fix: previously this function ran on netisr CPU 0 and walked
+	 * every other CPU's RB tree with plain RB_FOREACH and no
+	 * synchronization, racing concurrent RB_INSERT/RB_REMOVE on the
+	 * owning CPUs. Dispatch the per-CPU traversal via netmsg so each
+	 * tree is only touched from its owning CPU.
+	 */
+	size_t cap = sopt_size / sizeof(struct ipfw3_ioc_state);
+	struct ipfw3_state_get_msg {
+		struct netmsg_base base;
+		struct ipfw3_ioc_state *cur;
+		size_t remaining;
+		int overflow;
+	} msg;
+	netmsg_init(&msg.base, NULL, &curthread->td_msgport, 0,
+	    ip_fw3_ctl_state_get_dispatch);
+	msg.cur = user_buf;
+	msg.remaining = cap;
+	msg.overflow = 0;
+	netisr_domsg(&msg.base, 0);
+
+	size_t written = (cap - msg.remaining) * sizeof(struct ipfw3_ioc_state);
+	sopt->sopt_valsize = written;
 	return 0;
 }