Module Name:    src
Committed By:   maxv
Date:           Sat Apr  7 09:06:27 UTC 2018

Modified Files:
        src/sys/net/npf: npf_inet.c

Log Message:
Rewrite npf_fetch_tcpopts:

 * Instead of doing several nbuf_advance/nbuf_ensure_contig and
   playing with gotos, fetch the TCP options only once, and iterate over
   the (safe) area. The code is similar to tcp_dooptions.

 * When handling TCPOPT_MAXSEG and TCPOPT_WINDOW, ensure the length is
   the one we're expecting. If it isn't, then skip the option. This
   wasn't done before, and not doing it allowed a packet to bypass the
   max-mss clamping procedure. Discussed on tech-net@.


To generate a diff of this commit:
cvs rdiff -u -r1.48 -r1.49 src/sys/net/npf/npf_inet.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/net/npf/npf_inet.c
diff -u src/sys/net/npf/npf_inet.c:1.48 src/sys/net/npf/npf_inet.c:1.49
--- src/sys/net/npf/npf_inet.c:1.48	Fri Apr  6 14:50:55 2018
+++ src/sys/net/npf/npf_inet.c	Sat Apr  7 09:06:26 2018
@@ -1,4 +1,4 @@
-/*	$NetBSD: npf_inet.c,v 1.48 2018/04/06 14:50:55 maxv Exp $	*/
+/*	$NetBSD: npf_inet.c,v 1.49 2018/04/07 09:06:26 maxv Exp $	*/
 
 /*-
  * Copyright (c) 2009-2014 The NetBSD Foundation, Inc.
@@ -40,7 +40,7 @@
 
 #ifdef _KERNEL
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.48 2018/04/06 14:50:55 maxv Exp $");
+__KERNEL_RCSID(0, "$NetBSD: npf_inet.c,v 1.49 2018/04/07 09:06:26 maxv Exp $");
 
 #include <sys/param.h>
 #include <sys/types.h>
@@ -229,9 +229,9 @@ npf_fetch_tcpopts(npf_cache_t *npc, uint
 {
 	nbuf_t *nbuf = npc->npc_nbuf;
 	const struct tcphdr *th = npc->npc_l4.tcp;
-	int topts_len, step;
+	int cnt, optlen = 0;
 	bool setmss = false;
-	uint8_t *nptr;
+	uint8_t *cp, opt;
 	uint8_t val;
 	bool ok;
 
@@ -239,81 +239,64 @@ npf_fetch_tcpopts(npf_cache_t *npc, uint
 	KASSERT(npf_iscached(npc, NPC_TCP));
 
 	/* Determine if there are any TCP options, get their length. */
-	topts_len = (th->th_off << 2) - sizeof(struct tcphdr);
-	if (topts_len <= 0) {
+	cnt = (th->th_off << 2) - sizeof(struct tcphdr);
+	if (cnt <= 0) {
 		/* No options. */
 		return false;
 	}
-	KASSERT(topts_len <= MAX_TCPOPTLEN);
+	KASSERT(cnt <= MAX_TCPOPTLEN);
 
 	/* Determine if we want to set or get the mss. */
 	if (mss) {
 		setmss = (*mss != 0);
 	}
 
-	/* First step: IP and TCP header up to options. */
-	step = npc->npc_hlen + sizeof(struct tcphdr);
+	/* Fetch all the options at once. */
 	nbuf_reset(nbuf);
-next:
-	if ((nptr = nbuf_advance(nbuf, step, 1)) == NULL) {
+	const int step = npc->npc_hlen + sizeof(struct tcphdr);
+	if ((cp = nbuf_advance(nbuf, step, cnt)) == NULL) {
 		ok = false;
 		goto done;
 	}
-	val = *nptr;
 
-	switch (val) {
-	case TCPOPT_EOL:
-		/* Done. */
-		ok = true;
-		goto done;
-	case TCPOPT_NOP:
-		topts_len--;
-		step = 1;
-		break;
-	case TCPOPT_MAXSEG:
-		if ((nptr = nbuf_ensure_contig(nbuf, TCPOLEN_MAXSEG)) == NULL) {
-			ok = false;
-			goto done;
-		}
-		if (mss) {
-			if (setmss) {
-				memcpy(nptr + 2, mss, sizeof(uint16_t));
-			} else {
-				memcpy(mss, nptr + 2, sizeof(uint16_t));
+	/* Scan the options. */
+	for (; cnt > 0; cnt -= optlen, cp += optlen) {
+		opt = cp[0];
+		if (opt == TCPOPT_EOL)
+			break;
+		if (opt == TCPOPT_NOP)
+			optlen = 1;
+		else {
+			if (cnt < 2)
+				break;
+			optlen = cp[1];
+			if (optlen < 2 || optlen > cnt)
+				break;
+		}
+
+		switch (opt) {
+		case TCPOPT_MAXSEG:
+			if (optlen != TCPOLEN_MAXSEG)
+				continue;
+			if (mss) {
+				if (setmss) {
+					memcpy(cp + 2, mss, sizeof(uint16_t));
+				} else {
+					memcpy(mss, cp + 2, sizeof(uint16_t));
+				}
 			}
+			break;
+		case TCPOPT_WINDOW:
+			if (optlen != TCPOLEN_MAXSEG)
+				continue;
+			val = *(cp + 2);
+			*wscale = (val > TCP_MAX_WINSHIFT) ? TCP_MAX_WINSHIFT : val;
+			break;
+		default:
+			break;
 		}
-		topts_len -= TCPOLEN_MAXSEG;
-		step = TCPOLEN_MAXSEG;
-		break;
-	case TCPOPT_WINDOW:
-		/* TCP Window Scaling (RFC 1323). */
-		if ((nptr = nbuf_ensure_contig(nbuf, TCPOLEN_WINDOW)) == NULL) {
-			ok = false;
-			goto done;
-		}
-		val = *(nptr + 2);
-		*wscale = (val > TCP_MAX_WINSHIFT) ? TCP_MAX_WINSHIFT : val;
-		topts_len -= TCPOLEN_WINDOW;
-		step = TCPOLEN_WINDOW;
-		break;
-	default:
-		if ((nptr = nbuf_ensure_contig(nbuf, 2)) == NULL) {
-			ok = false;
-			goto done;
-		}
-		val = *(nptr + 1);
-		if (val < 2 || val > topts_len) {
-			ok = false;
-			goto done;
-		}
-		topts_len -= val;
-		step = val;
-	}
-
-	/* Any options left? */
-	if (__predict_true(topts_len > 0)) {
-		goto next;
 	}
+
 	ok = true;
 done:
 	if (nbuf_flag_p(nbuf, NBUF_DATAREF_RESET)) {

Reply via email to