git: 1ed2bf1e0052 - main - tcp: cleanup resource handling in SYN handling
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Wed, 17 Jun 2026 13:56:33 UTC
The branch main has been updated by tuexen:
URL: https://cgit.FreeBSD.org/src/commit/?id=1ed2bf1e0052df8dd6b429fc4ddd1908005f39ef
commit 1ed2bf1e0052df8dd6b429fc4ddd1908005f39ef
Author: Michael Tuexen <tuexen@FreeBSD.org>
AuthorDate: 2026-06-17 13:46:56 +0000
Commit: Michael Tuexen <tuexen@FreeBSD.org>
CommitDate: 2026-06-17 13:46:56 +0000
tcp: cleanup resource handling in SYN handling
Handle cred, ipopts, and maclabel using the same pattern:
allocate at the beginning and set to NULL when the object is
transferred to a struct syncache. When exiting the function, free
these objects if not transferred or when transferred to the on-stack
struct syncache. This makes use of a new function syncache_release().
This fixes a use after free problem: ipopts should only be freed,
if the on-stack struct syncache is used and the pointer in this
structure still points to the allocated ipopts. If the ipopts
are moved from the struct syncache to the struct inpcb in
syncache_socket(), which is called by syncache_tfo_expand(),
the pointer in the struct syncache is set to NULL.
In a FreeBSD default setup this problem is mitigated by
1. TCP fast open support on the server side not being enabled
(the sysctl-variable net.inet.tcp.fastopen.server_enable is 0).
2. Incoming IP packet with source routing options are not being
processed by the host stack
(the sysctl-variable net.inet.ip.accept_sourceroute is 0).
Only if these two sysctl-variables are changed, a FreeBSD system
is affected, if a server actually using TCP fast open is running.
Reported by: Yuxiang Yang, Yizhou Zhao, Xuewei Feng, Qi Li, and Ke Xu from Tsinghua University using GLM5.1 from Z.ai
Reviewed by: markj, rscheff
MFC after: 3 days
Sponsored by: Netflix, Inc.
Differential Revision: https://reviews.freebsd.org/D57374
---
sys/netinet/tcp_syncache.c | 70 +++++++++++++++++++---------------------------
1 file changed, 28 insertions(+), 42 deletions(-)
diff --git a/sys/netinet/tcp_syncache.c b/sys/netinet/tcp_syncache.c
index 22f260db9805..0d8f725455b7 100644
--- a/sys/netinet/tcp_syncache.c
+++ b/sys/netinet/tcp_syncache.c
@@ -224,21 +224,25 @@ static MALLOC_DEFINE(M_SYNCACHE, "syncache", "TCP syncache");
#define SCH_UNLOCK(sch) mtx_unlock(&(sch)->sch_mtx)
#define SCH_LOCK_ASSERT(sch) mtx_assert(&(sch)->sch_mtx, MA_OWNED)
-/*
- * Requires the syncache entry to be already removed from the bucket list.
- */
static void
-syncache_free(struct syncache *sc)
+syncache_release(struct syncache *sc)
{
-
- if (sc->sc_ipopts)
+ if (sc->sc_ipopts != NULL)
(void)m_free(sc->sc_ipopts);
- if (sc->sc_cred)
+ if (sc->sc_cred != NULL)
crfree(sc->sc_cred);
#ifdef MAC
mac_syncache_destroy(&sc->sc_label);
#endif
+}
+/*
+ * Requires the syncache entry to be already removed from the bucket list.
+ */
+static void
+syncache_free(struct syncache *sc)
+{
+ syncache_release(sc);
uma_zfree(V_tcp_syncache.zone, sc);
}
@@ -1427,6 +1431,7 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
*/
KASSERT(SOLISTENING(so), ("%s: %p not listening", __func__, so));
tp = sototcpcb(so);
+ bzero(&scs, sizeof(scs));
cred = V_tcp_syncache.see_other ? NULL : crhold(so->so_cred);
#ifdef INET6
@@ -1551,14 +1556,15 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
if (tfo_cookie_valid)
INP_RUNLOCK(inp);
TCPSTAT_INC(tcps_sc_dupsyn);
- if (ipopts) {
+ if (ipopts != NULL) {
/*
* If we were remembering a previous source route,
* forget it and use the new one we've been given.
*/
- if (sc->sc_ipopts)
+ if (sc->sc_ipopts != NULL)
(void)m_free(sc->sc_ipopts);
sc->sc_ipopts = ipopts;
+ ipopts = NULL;
}
/*
* Update timestamp if present.
@@ -1575,14 +1581,6 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
sc->sc_flags &= ~SCF_ECN_MASK;
sc->sc_flags |= tcp_ecn_syncache_add(tcp_get_flags(th), iptos);
}
-#ifdef MAC
- /*
- * Since we have already unconditionally allocated label
- * storage, free it up. The syncache entry will already
- * have an initialized label we can use.
- */
- mac_syncache_destroy(&maclabel);
-#endif
TCP_PROBE5(receive, NULL, NULL, m, NULL, th);
/* Retransmit SYN|ACK and reset retransmit count. */
if ((s = tcp_log_addrs(&sc->sc_inc, th, NULL, NULL))) {
@@ -1613,10 +1611,9 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
* Skip allocating a syncache entry if we are just going to discard
* it later.
*/
- if (!locked || tfo_cookie_valid) {
- bzero(&scs, sizeof(scs));
+ if (!locked || tfo_cookie_valid)
sc = &scs;
- } else {
+ else {
sc = uma_zalloc(V_tcp_syncache.zone, M_NOWAIT | M_ZERO);
if (sc == NULL) {
/*
@@ -1633,10 +1630,9 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
}
sc = uma_zalloc(V_tcp_syncache.zone, M_NOWAIT | M_ZERO);
if (sc == NULL) {
- if (V_tcp_syncookies) {
- bzero(&scs, sizeof(scs));
+ if (V_tcp_syncookies)
sc = &scs;
- } else {
+ else {
KASSERT(locked,
("%s: bucket unexpectedly unlocked",
__func__));
@@ -1656,23 +1652,13 @@ syncache_add(struct in_conninfo *inc, struct tcpopt *to, struct tcphdr *th,
*/
#ifdef MAC
sc->sc_label = maclabel;
+ maclabel = NULL;
#endif
- /*
- * sc_cred is only used in syncache_pcblist() to list TCP endpoints in
- * TCPS_SYN_RECEIVED state when V_tcp_syncache.see_other is false.
- * Therefore, store the credentials only when needed:
- * - sc is allocated from the zone and not using the on stack instance.
- * - the sysctl variable net.inet.tcp.syncache.see_other is false.
- * The reference count is decremented when a zone allocated sc is
- * freed in syncache_free().
- */
- if (sc != &scs && !V_tcp_syncache.see_other) {
- sc->sc_cred = cred;
- cred = NULL;
- } else
- sc->sc_cred = NULL;
- sc->sc_port = port;
+ sc->sc_cred = cred;
+ cred = NULL;
sc->sc_ipopts = ipopts;
+ ipopts = NULL;
+ sc->sc_port = port;
bcopy(inc, &sc->sc_inc, sizeof(struct in_conninfo));
sc->sc_ip_tos = ip_tos;
sc->sc_ip_ttl = ip_ttl;
@@ -1819,13 +1805,13 @@ donenoprobe:
tfo_expanded:
if (cred != NULL)
crfree(cred);
- if (sc == NULL || sc == &scs) {
#ifdef MAC
+ if (maclabel != NULL)
mac_syncache_destroy(&maclabel);
#endif
- if (ipopts)
- (void)m_free(ipopts);
- }
+ if (ipopts != NULL)
+ (void)m_free(ipopts);
+ syncache_release(&scs);
return (rv);
}