git: edf6dd82e966 - main - pf: fix use-after-free from pf_find_state_all

The branch main has been updated by mjg:

URL: https://cgit.FreeBSD.org/src/commit/?id=edf6dd82e9662b72c17483de4de5575dd5acd972

commit edf6dd82e9662b72c17483de4de5575dd5acd972
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2021-11-01 13:02:43 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2021-11-01 19:59:05 +0000

    pf: fix use-after-free from pf_find_state_all
    
    state was returned without any locks nor references held
    
    Reviewed by:    kp
    Sponsored by:   Rubicon Communications, LLC ("Netgate")
---
 sys/netpfil/pf/pf.c       | 21 +++++++++++++++++++--
 sys/netpfil/pf/pf_ioctl.c | 42 +++++++++++++++++++++++-------------------
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 61eea329a7f7..d7644b47f700 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1510,6 +1510,9 @@ pf_find_state(struct pfi_kkif *kif, struct pf_state_key_cmp *key, u_int dir)
 	return (NULL);
 }
 
+/*
+ * Returns with ID hash slot locked on success.
+ */
 struct pf_kstate *
 pf_find_state_all(struct pf_state_key_cmp *key, u_int dir, int *more)
 {
@@ -1547,14 +1550,17 @@ pf_find_state_all(struct pf_state_key_cmp *key, u_int dir, int *more)
 second_run:
 	TAILQ_FOREACH(s, &sk->states[idx], key_list[idx]) {
 		if (more == NULL) {
+			PF_STATE_LOCK(s);
 			PF_HASHROW_UNLOCK(kh);
 			return (s);
 		}
 
 		if (ret)
 			(*more)++;
-		else
+		else {
 			ret = s;
+			PF_STATE_LOCK(s);
+		}
 	}
 	if (inout == 1) {
 		inout = 0;
@@ -1566,13 +1572,24 @@ second_run:
 	return (ret);
 }
 
+/*
+ * FIXME
+ * This routine is inefficient -- locks the state only to unlock immediately on
+ * return.
+ * It is racy -- after the state is unlocked nothing stops other threads from
+ * removing it.
+ */
 bool
 pf_find_state_all_exists(struct pf_state_key_cmp *key, u_int dir)
 {
 	struct pf_kstate *s;
 
 	s = pf_find_state_all(key, dir, NULL);
-	return (s != NULL);
+	if (s != NULL) {
+		PF_STATE_UNLOCK(s);
+		return (true);
+	}
+	return (false);
 }
 
 /* END state table stuff */
diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
index ee265de65b45..1cfedf19e662 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -2029,19 +2029,20 @@ pf_label_match(const struct pf_krule *rule, const char *label)
 static unsigned int
 pf_kill_matching_state(struct pf_state_key_cmp *key, int dir)
 {
-	struct pf_kstate *match;
+	struct pf_kstate *s;
 	int more = 0;
-	unsigned int killed = 0;
 
-	/* Call with unlocked hashrow */
+	s = pf_find_state_all(key, dir, &more);
+	if (s == NULL)
+		return (0);
 
-	match = pf_find_state_all(key, dir, &more);
-	if (match && !more) {
-		pf_unlink_state(match, 0);
-		killed++;
+	if (more) {
+		PF_STATE_UNLOCK(s);
+		return (0);
 	}
 
-	return (killed);
+	pf_unlink_state(s, PF_ENTER_LOCKED);
+	return (1);
 }
 
 static int
@@ -3159,18 +3160,21 @@ DIOCGETSTATESV2_full:
 			key.port[didx] = pnl->dport;
 
 			state = pf_find_state_all(&key, direction, &m);
-
-			if (m > 1)
-				error = E2BIG;	/* more than one state */
-			else if (state != NULL) {
-				/* XXXGL: not locked read */
-				sk = state->key[sidx];
-				PF_ACPY(&pnl->rsaddr, &sk->addr[sidx], sk->af);
-				pnl->rsport = sk->port[sidx];
-				PF_ACPY(&pnl->rdaddr, &sk->addr[didx], sk->af);
-				pnl->rdport = sk->port[didx];
-			} else
+			if (state == NULL) {
 				error = ENOENT;
+			} else {
+				if (m > 1) {
+					PF_STATE_UNLOCK(state);
+					error = E2BIG;	/* more than one state */
+				} else {
+					sk = state->key[sidx];
+					PF_ACPY(&pnl->rsaddr, &sk->addr[sidx], sk->af);
+					pnl->rsport = sk->port[sidx];
+					PF_ACPY(&pnl->rdaddr, &sk->addr[didx], sk->af);
+					pnl->rdport = sk->port[didx];
+					PF_STATE_UNLOCK(state);
+				}
+			}
 		}
 		break;
 	}