[src/trunk]: src/sys/net/npf Rewrite npf_fetch_tcpopts:

[maxv <maxv%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/171b5adbe7e8
branches:  trunk
changeset: 321821:171b5adbe7e8
user:      maxv <maxv%NetBSD.org@localhost>
date:      Sat Apr 07 09:06:26 2018 +0000

description:
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@.

diffstat:

 sys/net/npf/npf_inet.c |  105 ++++++++++++++++++++----------------------------
 1 files changed, 44 insertions(+), 61 deletions(-)

diffs (152 lines):

diff -r 429ab15c92b8 -r 171b5adbe7e8 sys/net/npf/npf_inet.c
--- a/sys/net/npf/npf_inet.c    Sat Apr 07 00:41:16 2018 +0000
+++ b/sys/net/npf/npf_inet.c    Sat Apr 07 09:06:26 2018 +0000
@@ -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 @@
 {
        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 @@
        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;
                }
-               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;
+
+               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;
                }
-               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)) {