git: 389ae8e4fabe - main - pf: Free the stack key when attaching it collides
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Wed, 28 Oct 2026 16:56:59 UTC
The branch main has been updated by rcm:
URL: https://cgit.FreeBSD.org/src/commit/?id=389ae8e4fabeaf1c655f8116b1a1402e597fa515
commit 389ae8e4fabeaf1c655f8116b1a1402e597fa515
Author: R. Christian McDonald <rcm@FreeBSD.org>
AuthorDate: 2026-10-28 16:55:07 +0000
Commit: R. Christian McDonald <rcm@FreeBSD.org>
CommitDate: 2026-10-28 16:56:13 +0000
pf: Free the stack key when attaching it collides
pf_state_key_attach() attaches the wire key first and then the stack
key, setting sks to NULL before the second pass. When the stack key
collides with an existing state, the error path removes the wire key
again but then frees sks, which is NULL by then: the stack key leaks.
pfsync hits this. A peer that gets a plain state in a bulk update
before a redirected state whose stack key is the plain state's key
fails to import the redirected state, leaks its stack key, and leaks
another one with every later update of that state.
Free sk, the key of the pass that collided (skw on the wire pass), and
add a test.
Reviewed by: kp
Fixes: 371bd29d4b22 ("pf: fix potential state key leak")
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60951
---
sys/netpfil/pf/pf.c | 11 ++--
tests/sys/netpfil/pf/pfsync.sh | 141 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 146 insertions(+), 6 deletions(-)
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index bbba67529d09..f688bf82ddbc 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1872,12 +1872,11 @@ keyattach:
PF_SK_WIRE);
PF_HASHROW_UNLOCK(ih);
KEYS_UNLOCK();
- if (idx == PF_SK_WIRE)
- /*
- * We've not inserted either key.
- * Free both.
- */
- uma_zfree(V_pf_state_key_z, skw);
+ /*
+ * sk, this pass's key, is not in the
+ * hash; sks is NULL on the stack pass.
+ */
+ uma_zfree(V_pf_state_key_z, sk);
if (skw != sks)
uma_zfree(
V_pf_state_key_z,
diff --git a/tests/sys/netpfil/pf/pfsync.sh b/tests/sys/netpfil/pf/pfsync.sh
index c3914383a627..0e1d83d21823 100644
--- a/tests/sys/netpfil/pf/pfsync.sh
+++ b/tests/sys/netpfil/pf/pfsync.sh
@@ -1742,6 +1742,146 @@ flush_by_intf_cleanup()
pfsynct_cleanup
}
+# The state key zones of all vnets, by sysctl name.
+key_zones()
+{
+ sysctl -N vm.uma | \
+ sed -n 's/^vm\.uma\.\(pf_state_keys[_0-9]*\)\.size$/\1/p'
+}
+
+# Count of the states in jail $1 from source ports 40000-40031 whose output
+# line matches $2.
+port_states()
+{
+ jexec $1 pfctl -ss | grep '198.51.100.254:400[0-3][0-9]' | \
+ grep -c -- "$2"
+}
+
+# Wait up to 10 seconds for "eval $1" to print a number of at least $2.
+wait_for()
+{
+ for i in $(seq 1 20); do
+ [ "$(eval "$1")" -ge "$2" ] && return 0
+ sleep 0.5
+ done
+ atf_fail "Timed out waiting for '$1' to reach $2 (is $(eval "$1"))"
+}
+
+# Send one UDP packet to 198.51.100.1:$1 from each of the source ports
+# 40000-40031.
+send_flows()
+{
+ python3 -c '
+import socket, sys
+for sport in range(40000, 40032):
+ s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+ s.bind(("198.51.100.254", sport))
+ s.sendto(b"x", ("198.51.100.1", int(sys.argv[1])))
+ s.close()
+' $1
+}
+
+# Number of state records jail $1 has received over pfsync.
+state_records()
+{
+ jexec $1 netstat -s -p pfsync | awk '$2 == "state" &&
+ $3 ~ /^(insert|update)s?$/ && $4 == "received" { n += $1 }
+ END { print n + 0 }'
+}
+
+atf_test_case "stack_key_collision" "cleanup"
+stack_key_collision_head()
+{
+ atf_set descr 'Failing to import a state must not leak its keys'
+ atf_set require.user root
+ atf_set require.progs python3
+ # key_zones() would see the zones of vnets other tests create.
+ atf_set is.exclusive true
+}
+
+stack_key_collision_body()
+{
+ pfsynct_init
+
+ epair_sync=$(vnet_mkepair)
+ epair_one=$(vnet_mkepair)
+ epair_two=$(vnet_mkepair)
+
+ vnet_mkjail one ${epair_one}a ${epair_sync}a
+ # Each vnet gets its own state key zone: find two's.
+ zones=$(key_zones)
+ vnet_mkjail two ${epair_two}a ${epair_sync}b
+ zone=$(key_zones | grep -vx "${zones}")
+ if [ -z "${zone}" ] || [ $(echo "${zone}" | wc -l) -ne 1 ]; then
+ atf_fail "No unique state key zone for jail two: '${zone}'"
+ fi
+
+ jexec one ifconfig ${epair_sync}a 192.0.2.1/24 up
+ jexec one ifconfig ${epair_one}a 198.51.100.1/24 up
+ jexec one ifconfig pfsync0 \
+ syncdev ${epair_sync}a \
+ up
+ jexec two ifconfig ${epair_two}a 198.51.100.2/24 up
+ jexec two ifconfig ${epair_sync}b 192.0.2.2/24 up
+
+ rdr="rdr on ${epair_one}a inet proto udp to port 9999"
+ jexec one pfctl -e
+ pft_set_rules one \
+ "set skip on ${epair_sync}a" \
+ "${rdr} -> 198.51.100.1 port 7" \
+ "pass keep state"
+ jexec two pfctl -e
+ pft_set_rules two \
+ "set skip on { ${epair_sync}b ${epair_two}a }" \
+ "pass keep state"
+
+ ifconfig ${epair_one}b 198.51.100.254/24 up
+ # Resolve one's address before sending more than ARP holds.
+ atf_check -s exit:0 -o ignore ping -c 1 198.51.100.1
+
+ # Plain states to port 7, which two gets in a bulk update.
+ send_flows 7
+ wait_for "port_states one ." 32
+ jexec two ifconfig pfsync0 \
+ syncdev ${epair_sync}b \
+ up
+ wait_for "port_states two ." 32
+
+ # two stops listening and keeps them; one forgets them.
+ jexec two ifconfig pfsync0 -syncdev down
+ jexec one pfctl -F states
+
+ # Redirected states whose stack keys are the plain states' keys.
+ send_flows 9999
+ wait_for "port_states one 9999" 32
+
+ # two gets them in a second bulk update and cannot import any of
+ # them: each one's stack key collides with a plain state. Wait for
+ # the 32 state records themselves: the bulk update's end mark can
+ # arrive before some of them.
+ before=$(state_records two)
+ jexec two ifconfig pfsync0 \
+ syncdev ${epair_sync}b \
+ up
+ wait_for "echo \$((\$(state_records two) - ${before}))" 32
+ if [ "$(port_states two 9999)" -ne 0 ]; then
+ atf_fail "two imported a colliding state"
+ fi
+
+ # With every state gone, so must be every state key.
+ jexec two ifconfig pfsync0 -syncdev down
+ jexec two pfctl -F states
+ keys=$(sysctl -n vm.uma.${zone}.stats.current)
+ if [ "${keys}" -ne 0 ]; then
+ atf_fail "${keys} state keys left on two without states"
+ fi
+}
+
+stack_key_collision_cleanup()
+{
+ pfsynct_cleanup
+}
+
atf_init_test_cases()
{
atf_add_test_case "basic"
@@ -1767,4 +1907,5 @@ atf_init_test_cases()
atf_add_test_case "altq_queues"
atf_add_test_case "rt_af"
atf_add_test_case "flush_by_intf"
+ atf_add_test_case "stack_key_collision"
}