CVE-2023-52556: Insufficient Information (NVD-CWE-noinfo)
value: In OpenBSD 7.4 before errata 009, a race condition between pf(4)'s processing of packets and expiration of packet states may cause a kernel panic.

CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-03-01T17:15Z
Repository: https://github.com/openbsd/src
Language: C
Files changed: sys/net/pf.c
Commit date: 2023-12-01 10:28:32 +0000
Commit message: Prevent race between pf_test() and pf_purge_expired_states().
Packets (callers to pf_test()) must alter pf_state::timeout
under protection of pf_state::mtx. We also have to make sure
the packet does not update pf_state::timeout when ::timeout
reaches PFTM_UNLINKED.

The first report came from Johan Huldtgren, but he is not
the single user who has noticed "st->timeout == PFTM_UNLINKED"
assert violation.

OK bluhm@

Diff: diff --git a/sys/net/pf.c b/sys/net/pf.c
index 9984c8aae9d59..c4cd86a351406 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,6 +1,6 @@
-/*	$OpenBSD: pf.c,v 1.1188 2023/10/10 16:26:06 bluhm Exp $ */
+/*	$OpenBSD: pf.c,v 1.1189 2023/12/01 10:28:32 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
  * Copyright (c) 2002 - 2013 Henning Brauer <henning@openbsd.org>
  * All rights reserved.
@@ -467,10 +467,19 @@ pf_state_list_remove(struct pf_state_list *pfs, struct pf_state *st)
 	mtx_leave(&pfs->pfs_mtx);
 
 	pf_state_unref(st); /* list no longer references the state */
 }
 
+void
+pf_update_state_timeout(struct pf_state *st, int to)
+{
+	mtx_enter(&st->mtx);
+	if (st->timeout != PFTM_UNLINKED)
+		st->timeout = to;
+	mtx_leave(&st->mtx);
+}
+
 int
 pf_src_connlimit(struct pf_state **stp)
 {
 	int			 bad = 0;
 	struct pf_src_node	*sn;
@@ -547,11 +556,11 @@ pf_src_connlimit(struct pf_state **stp)
 				    ((*stp)->direction == PF_IN &&
 				    PF_AEQ(&sn->addr, &sk->addr[0], sk->af))) &&
 				    ((*stp)->rule.ptr->flush &
 				    PF_FLUSH_GLOBAL ||
 				    (*stp)->rule.ptr == st->rule.ptr)) {
-					st->timeout = PFTM_PURGE;
+					pf_update_state_timeout(st, PFTM_PURGE);
 					pf_set_protostate(st, PF_PEER_BOTH,
 					    TCPS_CLOSED);
 					killed++;
 				}
 			}
@@ -561,11 +570,11 @@ pf_src_connlimit(struct pf_state **stp)
 		if (pf_status.debug >= LOG_NOTICE)
 			addlog("\n");
 	}
 
 	/* kill this state */
-	(*stp)->timeout = PFTM_PURGE;
+	pf_update_state_timeout(*stp, PFTM_PURGE);
 	pf_set_protostate(*stp, PF_PEER_BOTH, TCPS_CLOSED);
 	return (1);
 }
 
 int
@@ -1756,14 +1765,17 @@ pf_src_tree_remove_state(struct pf_state *st)
 void
 pf_remove_state(struct pf_state *st)
 {
 	PF_ASSERT_LOCKED();
 
-	if (st->timeout == PFTM_UNLINKED)
+	mtx_enter(&st->mtx);
+	if (st->timeout == PFTM_UNLINKED) {
+		mtx_leave(&st->mtx);
 		return;
-
+	}
 	st->timeout = PFTM_UNLINKED;
+	mtx_leave(&st->mtx);
 
 	/* handle load balancing related tasks */
 	pf_postprocess_addr(st);
 
 	if (st->src.state == PF_TCPS_PROXY_DST) {
@@ -1814,11 +1826,12 @@ pf_remove_divert_state(struct pf_state_key *sk)
 				 */
 				if (sist->src.state < TCPS_FIN_WAIT_2 ||
 				    sist->dst.state < TCPS_FIN_WAIT_2) {
 					pf_set_protostate(sist, PF_PEER_BOTH,
 					    TCPS_TIME_WAIT);
-					sist->timeout = PFTM_TCP_CLOSED;
+					pf_update_state_timeout(sist,
+					    PFTM_TCP_CLOSED);
 					sist->expire = getuptime();
 				}
 				sist->state_flags |= PFSTATE_INP_UNLINKED;
 			} else
 				pf_remove_state(sist);
@@ -5034,22 +5047,22 @@ pf_tcp_track_full(struct pf_pdesc *pd, struct pf_state **stp, u_short *reason,
 
 		/* update expire time */
 		(*stp)->expire = getuptime();
 		if (src->state >= TCPS_FIN_WAIT_2 &&
 		    dst->state >= TCPS_FIN_WAIT_2)
-			(*stp)->timeout = PFTM_TCP_CLOSED;
+			pf_update_state_timeout(*stp, PFTM_TCP_CLOSED);
 		else if (src->state >= TCPS_CLOSING &&
 		    dst->state >= TCPS_CLOSING)
-			(*stp)->timeout = PFTM_TCP_FIN_WAIT;
+			pf_update_state_timeout(*stp, PFTM_TCP_FIN_WAIT);
 		else if (src->state < TCPS_ESTABLISHED ||
 		    dst->state < TCPS_ESTABLISHED)
-			(*stp)->timeout = PFTM_TCP_OPENING;
+			pf_update_state_timeout(*stp, PFTM_TCP_OPENING);
 		else if (src->state >= TCPS_CLOSING ||
 		    dst->state >= TCPS_CLOSING)
-			(*stp)->timeout = PFTM_TCP_CLOSING;
+			pf_update_state_timeout(*stp, PFTM_TCP_CLOSING);
 		else
-			(*stp)->timeout = PFTM_TCP_ESTABLISHED;
+			pf_update_state_timeout(*stp, PFTM_TCP_ESTABLISHED);
 
 		/* Fall through to PASS packet */
 	} else if ((dst->state < TCPS_SYN_SENT ||
 		dst->state >= TCPS_FIN_WAIT_2 ||
 		src->state >= TCPS_FIN_WAIT_2) &&
@@ -5227,22 +5240,22 @@ pf_tcp_track_sloppy(struct pf_pdesc *pd, struct pf_state **stp,
 
 	/* update expire time */
 	(*stp)->expire = getuptime();
 	if (src->state >= TCPS_FIN_WAIT_2 &&
 	    dst->state >= TCPS_FIN_WAIT_2)
-		(*stp)->timeout = PFTM_TCP_CLOSED;
+		pf_update_state_timeout(*stp, PFTM_TCP_CLOSED);
 	else if (src->state >= TCPS_CLOSING &&
 	    dst->state >= TCPS_CLOSING)
-		(*stp)->timeout = PFTM_TCP_FIN_WAIT;
+		pf_update_state_timeout(*stp, PFTM_TCP_FIN_WAIT);
 	else if (src->state < TCPS_ESTABLISHED ||
 	    dst->state < TCPS_ESTABLISHED)
-		(*stp)->timeout = PFTM_TCP_OPENING;
+		pf_update_state_timeout(*stp, PFTM_TCP_OPENING);
 	else if (src->state >= TCPS_CLOSING ||
 	    dst->state >= TCPS_CLOSING)
-		(*stp)->timeout = PFTM_TCP_CLOSING;
+		pf_update_state_timeout(*stp, PFTM_TCP_CLOSING);
 	else
-		(*stp)->timeout = PFTM_TCP_ESTABLISHED;
+		pf_update_state_timeout(*stp, PFTM_TCP_ESTABLISHED);
 
 	return (PF_PASS);
 }
 
 static __inline int
@@ -5375,11 +5388,11 @@ pf_test_state(struct pf_pdesc *pd, struct pf_state **stp, u_short *reason)
 					pf_print_state(*stp);
 					pf_print_flags(pd->hdr.tcp.th_flags);
 					addlog("\n");
 				}
 				/* XXX make sure it's the same direction ?? */
-				(*stp)->timeout = PFTM_PURGE;
+				pf_update_state_timeout(*stp, PFTM_PURGE);
 				pf_state_unref(*stp);
 				*stp = NULL;
 				pf_mbuf_link_inpcb(pd->m, inp);
 				return (PF_DROP);
 			} else if (dst->state >= TCPS_ESTABLISHED &&
@@ -5415,13 +5428,13 @@ pf_test_state(struct pf_pdesc *pd, struct pf_state **stp, u_short *reason)
 
 		/* update expire time */
 		(*stp)->expire = getuptime();
 		if (src->state == PFUDPS_MULTIPLE &&
 		    dst->state == PFUDPS_MULTIPLE)
-			(*stp)->timeout = PFTM_UDP_MULTIPLE;
+			pf_update_state_timeout(*stp, PFTM_UDP_MULTIPLE);
 		else
-			(*stp)->timeout = PFTM_UDP_SINGLE;
+			pf_update_state_timeout(*stp, PFTM_UDP_SINGLE);
 		break;
 	default:
 		/* update states */
 		if (src->state < PFOTHERS_SINGLE)
 			pf_set_protostate(*stp, psrc, PFOTHERS_SINGLE);
@@ -5430,13 +5443,13 @@ pf_test_state(struct pf_pdesc *pd, struct pf_state **stp, u_short *reason)
 
 		/* update expire time */
 		(*stp)->expire = getuptime();
 		if (src->state == PFOTHERS_MULTIPLE &&
 		    dst->state == PFOTHERS_MULTIPLE)
-			(*stp)->timeout = PFTM_OTHER_MULTIPLE;
+			pf_update_state_timeout(*stp, PFTM_OTHER_MULTIPLE);
 		else
-			(*stp)->timeout = PFTM_OTHER_SINGLE;
+			pf_update_state_timeout(*stp, PFTM_OTHER_SINGLE);
 		break;
 	}
 
 	/* translate source/destination address, if necessary */
 	if ((*stp)->key[PF_SK_WIRE] != (*stp)->key[PF_SK_STACK]) {
@@ -5583,11 +5596,11 @@ pf_test_state_icmp(struct pf_pdesc *pd, struct pf_state **stp,
 			    virtual_type, icmp_dir, &iidx, 1, 0);
 		if (ret >= 0)
 			return (ret);
 
 		(*stp)->expire = getuptime();
-		(*stp)->timeout = PFTM_ICMP_ERROR_REPLY;
+		pf_update_state_timeout(*stp, PFTM_ICMP_ERROR_REPLY);
 
 		/* translate source/destination address, if necessary */
 		if ((*stp)->key[PF_SK_WIRE] != (*stp)->key[PF_SK_STACK]) {
 			struct pf_state_key	*nk;
 			int			 afto, sidx, didx;

Vulnerable code: /*	$OpenBSD: pf.c,v 1.1188 2023/10/10 16:26:06 bluhm Exp $ */
					st->timeout = PFTM_PURGE;
	(*stp)->timeout = PFTM_PURGE;
	if (st->timeout == PFTM_UNLINKED)

					sist->timeout = PFTM_TCP_CLOSED;
			(*stp)->timeout = PFTM_TCP_CLOSED;
			(*stp)->timeout = PFTM_TCP_FIN_WAIT;
			(*stp)->timeout = PFTM_TCP_OPENING;
			(*stp)->timeout = PFTM_TCP_CLOSING;
			(*stp)->timeout = PFTM_TCP_ESTABLISHED;
		(*stp)->timeout = PFTM_TCP_CLOSED;
		(*stp)->timeout = PFTM_TCP_FIN_WAIT;
		(*stp)->timeout = PFTM_TCP_OPENING;
		(*stp)->timeout = PFTM_TCP_CLOSING;
		(*stp)->timeout = PFTM_TCP_ESTABLISHED;
				(*stp)->timeout = PFTM_PURGE;
			(*stp)->timeout = PFTM_UDP_MULTIPLE;
			(*stp)->timeout = PFTM_UDP_SINGLE;
			(*stp)->timeout = PFTM_OTHER_MULTIPLE;
			(*stp)->timeout = PFTM_OTHER_SINGLE;
		(*stp)->timeout = PFTM_ICMP_ERROR_REPLY;
Fixed code: /*	$OpenBSD: pf.c,v 1.1189 2023/12/01 10:28:32 sashan Exp $ */
void
pf_update_state_timeout(struct pf_state *st, int to)
{
	mtx_enter(&st->mtx);
	if (st->timeout != PFTM_UNLINKED)
		st->timeout = to;
	mtx_leave(&st->mtx);
}

					pf_update_state_timeout(st, PFTM_PURGE);
	pf_update_state_timeout(*stp, PFTM_PURGE);
	mtx_enter(&st->mtx);
	if (st->timeout == PFTM_UNLINKED) {
		mtx_leave(&st->mtx);
	}
	mtx_leave(&st->mtx);
					pf_update_state_timeout(sist,
					    PFTM_TCP_CLOSED);
			pf_update_state_timeout(*stp, PFTM_TCP_CLOSED);
			pf_update_state_timeout(*stp, PFTM_TCP_FIN_WAIT);
			pf_update_state_timeout(*stp, PFTM_TCP_OPENING);
			pf_update_state_timeout(*stp, PFTM_TCP_CLOSING);
			pf_update_state_timeout(*stp, PFTM_TCP_ESTABLISHED);
		pf_update_state_timeout(*stp, PFTM_TCP_CLOSED);
		pf_update_state_timeout(*stp, PFTM_TCP_FIN_WAIT);
		pf_update_state_timeout(*stp, PFTM_TCP_OPENING);
		pf_update_state_timeout(*stp, PFTM_TCP_CLOSING);
		pf_update_state_timeout(*stp, PFTM_TCP_ESTABLISHED);
				pf_update_state_timeout(*stp, PFTM_PURGE);
			pf_update_state_timeout(*stp, PFTM_UDP_MULTIPLE);
			pf_update_state_timeout(*stp, PFTM_UDP_SINGLE);
			pf_update_state_timeout(*stp, PFTM_OTHER_MULTIPLE);
			pf_update_state_timeout(*stp, PFTM_OTHER_SINGLE);
		pf_update_state_timeout(*stp, PFTM_ICMP_ERROR_REPLY);