From 41e3b49d2f2ddfd05ed31fbf695fa48ca43f73a9 Mon Sep 17 00:00:00 2001 From: Kristof Provost Date: Thu, 27 Mar 2025 15:21:41 +0100 Subject: [PATCH] pf: improve pf_state_key_attach() error handling If we fail to attach the stack key that means we've already attached the wire key. That means the state could be found by other cores, and given that we then free it, be used after free. Fix this by not releasing the ID hashrow lock and key locks until after we've removed the inserted key again, ensuring the state cannot be found by other cores. Reported by: markj Submitted by: glebius Reviewed by: glebius, markj MFC after: 3 weeks Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D49550 --- sys/netpfil/pf/pf.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 0cb79b42899..4db9b7a0f1f 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -1377,15 +1377,28 @@ keyattach: printf("\n"); } s->timeout = PFTM_UNLINKED; + if (idx == PF_SK_STACK) + /* + * Remove the wire key from + * the hash. Other threads + * can't be referencing it + * because we still hold the + * hash lock. + */ + pf_state_key_detach(s, + PF_SK_WIRE); PF_HASHROW_UNLOCK(ih); KEYS_UNLOCK(); - if (idx == PF_SK_WIRE) { + if (idx == PF_SK_WIRE) + /* + * We've not inserted either key. + * Free both. + */ uma_zfree(V_pf_state_key_z, skw); - if (skw != sks) - uma_zfree(V_pf_state_key_z, sks); - } else { - pf_detach_state(s); - } + if (skw != sks) + uma_zfree( + V_pf_state_key_z, + sks); return (EEXIST); /* collision! */ } }