From 63f17add1cf6119ec8f692990df2892d86244f2f Mon Sep 17 00:00:00 2001
From: Matthew Dillon <dillon@apollo.backplane.com>
Date: Tue, 13 Nov 2018 11:12:36 -0800
Subject: [PATCH] kernel - Fix sack NULL pointer dereference

* sack_block_lookup() can get confused when the passed-in sequence
  number appears to be less than sblk_start and greater than sblk_end.
  This situation can occur when the signed integer delta test has an
  overflow due to (sblk_end - seq) overflowing the sign bit verses
  (sblk_start - seq).

  The result is that sack_block_lookup() can crash on a NULL pointer
  indirection.

* Check for the case, complain, and try to allow it.  Though I suspect
  if the case occurs at all SACK will wind up with a broken list anyway.

* I don't think this case can occur under normal conditions since TCP
  buffers do not grow to 2GB+ in size, so the crash we got was triggered
  by either an accidently malformed packet or an intentional one.
---
 sys/netinet/tcp_sack.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/sys/netinet/tcp_sack.c b/sys/netinet/tcp_sack.c
index 1a29bd1794..33927ffae3 100644
--- a/sys/netinet/tcp_sack.c
+++ b/sys/netinet/tcp_sack.c
@@ -42,6 +42,7 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/socketvar.h>
+#include <sys/resourcevar.h>
 
 #include <net/if.h>
 
@@ -98,6 +99,7 @@ tcp_sack_tcpcb_init(struct tcpcb *tp)
 static boolean_t
 sack_block_lookup(struct scoreboard *scb, tcp_seq seq, struct sackblock **sb)
 {
+	static struct krate sackkrate = { .freq = 1 };
 	struct sackblock *hint = scb->lastfound;
 	struct sackblock *cur, *last, *prev;
 
@@ -126,6 +128,21 @@ sack_block_lookup(struct scoreboard *scb, tcp_seq seq, struct sackblock **sb)
 	}
 
 	do {
+		/*
+		 * Ensure we can't crash if the list really blows up due to
+		 * delta sign wraps when comparing seq against sblk_start vs
+		 * sblk_end.
+		 */
+		if (cur == NULL) {
+			krateprintf(&sackkrate,
+				    "tcp_sack: fatal corrupt seq\n");
+			*sb = NULL;
+			return FALSE;
+		}
+
+		/*
+		 * Check completion
+		 */
 		if (SEQ_GT(cur->sblk_end, seq)) {
 			if (SEQ_GEQ(seq, cur->sblk_start)) {
 				*sb = scb->lastfound = cur;
@@ -136,6 +153,31 @@ sack_block_lookup(struct scoreboard *scb, tcp_seq seq, struct sackblock **sb)
 				return FALSE;
 			}
 		}
+
+		/*
+		 * seq is greater than sblk_end, nominally proceed to the
+		 * next block.
+		 *
+		 * It is possible for an overflow to cause the comparison
+		 * between seq an sblk_start vs sblk_end to make it appear
+		 * that seq is less than sblk_start and also greater than
+		 * sblk_end.  If we allow the case to fall through we can
+		 * end up with cur == NULL on the next loop.
+		 */
+		if (SEQ_LT(seq, cur->sblk_start)) {
+			krateprintf(&sackkrate,
+				    "tcp_sack: corrupt seq "
+				    "0x%08x vs 0x%08x-0x%08x\n",
+				    seq, cur->sblk_start, cur->sblk_end);
+			if (SEQ_GEQ(seq, cur->sblk_start)) {
+				*sb = scb->lastfound = cur;
+				return TRUE;
+			} else {
+				*sb = scb->lastfound =
+				    TAILQ_PREV(cur, sackblock_list, sblk_list);
+				return FALSE;
+			}
+		}
 		cur = TAILQ_NEXT(cur, sblk_list);
 	} while (cur != last);
 
-- 
2.41.0