The branch main has been updated by tuexen: URL: https://cgit.FreeBSD.org/src/commit/?id=1ed2bf1e0052df8dd6b429fc4ddd1908005f39ef
commit 1ed2bf1e0052df8dd6b429fc4ddd1908005f39ef Author: Michael Tuexen <[email protected]> AuthorDate: 2026-06-17 13:46:56 +0000 Commit: Michael Tuexen <[email protected]> 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); }
