From: Alexander Bluhm <bluhm@openbsd.org>
Subject: signed interger bug in tcp mss
To: tech@openbsd.org
Date: Sun, 10 Nov 2024 14:27:21 +0100

Hi,

In tcp_mss_adv() max(9) was used to guarantee that mss it not too
small.  Unfortunately max() uses u_int and mss could get negative
in some error conditions.

Rearrange the code to directly return in case of errors.  Also read
tcp_mssdflt only once to head towards atomic integer sysctl.

ok?

bluhm

Index: netinet/tcp_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
diff -u -p -r1.408 tcp_input.c
--- netinet/tcp_input.c	8 Nov 2024 21:40:39 -0000	1.408
+++ netinet/tcp_input.c	10 Nov 2024 13:16:01 -0000
@@ -3052,32 +3052,36 @@ tcp_newreno_partialack(struct tcpcb *tp,
 int
 tcp_mss_adv(struct mbuf *m, int af)
 {
-	int mss = 0;
-	int iphlen;
-	struct ifnet *ifp = NULL;
+	struct ifnet *ifp;
+	int iphlen, mss, mssdflt;
 
-	if (m && (m->m_flags & M_PKTHDR))
-		ifp = if_get(m->m_pkthdr.ph_ifidx);
+	mssdflt = atomic_load_int(&tcp_mssdflt);
+
+	if (m == NULL || (m->m_flags & M_PKTHDR) == 0)
+		return mssdflt;
+
+	ifp = if_get(m->m_pkthdr.ph_ifidx);
+	if (ifp == NULL)
+		return mssdflt;
 
 	switch (af) {
 	case AF_INET:
-		if (ifp != NULL)
-			mss = ifp->if_mtu;
 		iphlen = sizeof(struct ip);
 		break;
 #ifdef INET6
 	case AF_INET6:
-		if (ifp != NULL)
-			mss = ifp->if_mtu;
 		iphlen = sizeof(struct ip6_hdr);
 		break;
 #endif
 	default:
 		unhandled_af(af);
 	}
+	mss = ifp->if_mtu - iphlen - sizeof(struct tcphdr);
 	if_put(ifp);
-	mss = mss - iphlen - sizeof(struct tcphdr);
-	return (max(mss, tcp_mssdflt));
+
+	if (mss < mssdflt)
+		return mssdflt;
+	return mss;
 }
 
 /*