[PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[Adam Langley]

When MD5 signatures are turned on we can end up with syntactically invalid
packets with a header length < 20 bytes. This is because tcp_header_size
overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
SACK option.

Since we can't fit any SACK blocks in the final 8 bytes of options space, and
the MD5 signature is more important, we disable including SACK, or even
advertising it, when MD5 is enabled.

Signed-off-by: Adam Langley <agl@imperialviolet.org>

---
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..ced7241 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -358,7 +358,7 @@ static void tcp_build_and_update_options(__be32 *ptr, struct tcp_sock *tp,
 		*ptr++ = htonl(tstamp);
 		*ptr++ = htonl(tp->rx_opt.ts_recent);
 	}
-	if (tp->rx_opt.eff_sacks) {
+	if (!md5_hash && tp->rx_opt.eff_sacks) {
 		struct tcp_sack_block *sp = tp->rx_opt.dsack ? tp->duplicate_sack : tp->selective_acks;
 		int this_sack;
 
@@ -470,12 +470,12 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 			    gfp_t gfp_mask)
 {
 	const struct inet_connection_sock *icsk = inet_csk(sk);
+	struct tcp_md5sig_key *md5 = NULL;
 	struct inet_sock *inet;
 	struct tcp_sock *tp;
 	struct tcp_skb_cb *tcb;
 	int tcp_header_size;
 #ifdef CONFIG_TCP_MD5SIG
-	struct tcp_md5sig_key *md5;
 	__u8 *md5_hash_location;
 #endif
 	struct tcphdr *th;
@@ -504,13 +504,25 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	tcb = TCP_SKB_CB(skb);
 	tcp_header_size = tp->tcp_header_len;
 
+	if (unlikely(tcb->flags & TCPCB_FLAG_SYN))
+		tcp_header_size = sizeof(struct tcphdr) + TCPOLEN_MSS;
+
+#ifdef CONFIG_TCP_MD5SIG
+	/*
+	 * Are we doing MD5 on this segment? If so - make
+	 * room for it.
+	 */
+	md5 = tp->af_specific->md5_lookup(sk, sk);
+	if (md5)
+		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
+#endif
+
 #define SYSCTL_FLAG_TSTAMPS	0x1
 #define SYSCTL_FLAG_WSCALE	0x2
 #define SYSCTL_FLAG_SACK	0x4
 
 	sysctl_flags = 0;
 	if (unlikely(tcb->flags & TCPCB_FLAG_SYN)) {
-		tcp_header_size = sizeof(struct tcphdr) + TCPOLEN_MSS;
 		if (sysctl_tcp_timestamps) {
 			tcp_header_size += TCPOLEN_TSTAMP_ALIGNED;
 			sysctl_flags |= SYSCTL_FLAG_TSTAMPS;
@@ -519,12 +531,15 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 			tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
 			sysctl_flags |= SYSCTL_FLAG_WSCALE;
 		}
-		if (sysctl_tcp_sack) {
+		/* We don't include SACK options if we are going to
+		 * include an MD5 signature because they can't fit
+		 * in the options space */
+		if (sysctl_tcp_sack && !md5) {
 			sysctl_flags |= SYSCTL_FLAG_SACK;
 			if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
 				tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
 		}
-	} else if (unlikely(tp->rx_opt.eff_sacks)) {
+	} else if (unlikely(tp->rx_opt.eff_sacks && !md5)) {
 		/* A SACK is 2 pad bytes, a 2 byte header, plus
 		 * 2 32-bit sequence numbers for each SACK block.
 		 */
@@ -536,16 +551,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	if (tcp_packets_in_flight(tp) == 0)
 		tcp_ca_event(sk, CA_EVENT_TX_START);
 
-#ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * Are we doing MD5 on this segment? If so - make
-	 * room for it.
-	 */
-	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 	skb_set_owner_w(skb, sk);
@@ -974,6 +979,7 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 	u32 mss_now;
 	u16 xmit_size_goal;
 	int doing_tso = 0;
+	int md5_signing = 0;
 
 	mss_now = tp->mss_cache;
 
@@ -986,15 +992,17 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 			mss_now = tcp_sync_mss(sk, mtu);
 	}
 
-	if (tp->rx_opt.eff_sacks)
-		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
-			    (tp->rx_opt.eff_sacks * TCPOLEN_SACK_PERBLOCK));
-
 #ifdef CONFIG_TCP_MD5SIG
-	if (tp->af_specific->md5_lookup(sk, sk))
+	if (tp->af_specific->md5_lookup(sk, sk)) {
+		md5_signing = 1;
 		mss_now -= TCPOLEN_MD5SIG_ALIGNED;
+	}
 #endif
 
+	if (!md5_signing && tp->rx_opt.eff_sacks)
+		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
+			    (tp->rx_opt.eff_sacks * TCPOLEN_SACK_PERBLOCK));
+
 	xmit_size_goal = mss_now;
 
 	if (doing_tso) {
@@ -2193,6 +2201,13 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 
 	skb->dst = dst_clone(dst);
 
+#ifdef CONFIG_TCP_MD5SIG
+	md5 = tcp_rsk(req)->af_specific->md5_lookup(sk, req);
+	/* SACKs don't fit in the option space with MD5 sigs */
+	if (md5)
+		ireq->sack_ok = 0;
+#endif
+
 	tcp_header_size = (sizeof(struct tcphdr) + TCPOLEN_MSS +
 			   (ireq->tstamp_ok ? TCPOLEN_TSTAMP_ALIGNED : 0) +
 			   (ireq->wscale_ok ? TCPOLEN_WSCALE_ALIGNED : 0) +
@@ -2201,7 +2216,6 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Are we doing MD5 on this segment? If so - make room for it */
-	md5 = tcp_rsk(req)->af_specific->md5_lookup(sk, req);
 	if (md5)
 		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
 #endif

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Tue, 3 Jun 2008 14:52:00 -0700

> When MD5 signatures are turned on we can end up with syntactically invalid
> packets with a header length < 20 bytes. This is because tcp_header_size
> overflows with 12 bytes of timestamp, 20 bytes of signature and > 8 bytes of
> SACK option.
> 
> Since we can't fit any SACK blocks in the final 8 bytes of options space, and
> the MD5 signature is more important, we disable including SACK, or even
> advertising it, when MD5 is enabled.
> 
> Signed-off-by: Adam Langley <agl@imperialviolet.org>

If this is the case, I think we need to simply prioritize at option
negotiation time and leave it at that.  The data paths don't need
to be modified.

The problematic case is when all 3 of tstamps, wscale, and md5 are
enabled.  To be honest, tstamps are the least valuable of the 3.  The
only way to accurately fill in gaps is to have at least some SACK
information, it is much harder to guestimate than RTTs.

So, actually, we'd need to add logic to limit the amount of SACK
blocks we report when MD5 is present, in the output packet building
code.

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[Adam Langley]

On Tue, Jun 17, 2008 at 5:07 PM, David Miller <davem@davemloft.net> wrote:
> The problematic case is when all 3 of tstamps, wscale, and md5 are
> enabled.  To be honest, tstamps are the least valuable of the 3.  The
> only way to accurately fill in gaps is to have at least some SACK
> information, it is much harder to guestimate than RTTs.

So, have a population of Linux hosts out there which will send corrupt
packets when MD5+SACK is enabled. So we want to fix that problem all
at once, hopefully.

How's this:

If we receive a SYN packet with MD5 + SACK + TS was assume that it's
from an older kernel and reply with MD5 + TS. Not including SACK means
that it won't send us corrupt packets and since we couldn't previously
do SACK with these packets anyway, we're not loosing anything.

However, by default we send MD5 + SACK. If we are talking to an old
kernel, that means that they are sending corrupt packets if they have
> 2 SACK blocks (the current logic limits them to 4 I believe).
Hopefully this is pretty rare (and, again, better than we are
currently doing). It also means that we have MD5 + SACK between new
hosts, which is optimal.

I was actually writing a patch to implement
http://tools.ietf.org/html/draft-eddy-tcp-loo-03 (Long Options), which
involved pulling all the options logic into a single function for SYN
and one more for normal packets (rather than the duplication which we
have now). I'll probably send a draft patch for comments later today
(PST).

Pulling all the options logic together makes it a lot easier to make
these changes (once we've worked the bugs out of the change
initially). Possibly I should split it from the long options stuff and
make it a separate patch. It already solves this MD5+SACK bug and
other where MD5 signatures are accounted for twice in the MSS
calculations.

Cheers,


AGL

-- 
Adam Langley agl@imperialviolet.org http://www.imperialviolet.org

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Tue, 17 Jun 2008 17:45:52 -0700

> How's this:
> 
> If we receive a SYN packet with MD5 + SACK + TS was assume that it's
> from an older kernel and reply with MD5 + TS. Not including SACK means
> that it won't send us corrupt packets and since we couldn't previously
> do SACK with these packets anyway, we're not loosing anything.

We should reject invalid packets, even those created by
Linus, regardless of the ramifications of such.

If we drop such frames, things will reset and a timeout
based retransmission will occur.

I don't see any value in trying to recognize these
invalid frames.  We should instead just fix the part
of Linux that emits the bogus packets to begin with.

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[Bill Fink]

On Tue, 17 Jun 2008, Adam Langley wrote:

> On Tue, Jun 17, 2008 at 5:07 PM, David Miller <davem@davemloft.net> wrote:
> > The problematic case is when all 3 of tstamps, wscale, and md5 are
> > enabled.  To be honest, tstamps are the least valuable of the 3.  The
> > only way to accurately fill in gaps is to have at least some SACK
> > information, it is much harder to guestimate than RTTs.
> 
> So, have a population of Linux hosts out there which will send corrupt
> packets when MD5+SACK is enabled. So we want to fix that problem all
> at once, hopefully.
> 
> How's this:
> 
> If we receive a SYN packet with MD5 + SACK + TS was assume that it's
> from an older kernel and reply with MD5 + TS. Not including SACK means
> that it won't send us corrupt packets and since we couldn't previously
> do SACK with these packets anyway, we're not loosing anything.
> 
> However, by default we send MD5 + SACK. If we are talking to an old
> kernel, that means that they are sending corrupt packets if they have
> > 2 SACK blocks (the current logic limits them to 4 I believe).
> Hopefully this is pretty rare (and, again, better than we are
> currently doing). It also means that we have MD5 + SACK between new
> hosts, which is optimal.

I am not particularly familiar with the TCP MD5 option, but in googling
around I found the following at the end of Section 2.2 Transmission,
of RFC 4808, "TCP-MD5 Key Change", dated March 2007:

    "Note that there is an ambiguity when an acknowledgment is received
    for a segment transmitted with two different keys.  The TCP Timestamp
    option [RFC1323] can be used for disambiguation."

This would seem to imply, that at least in some scenarios, it would be
advisable to have TS enabled in conjunction with MD5.

						-Bill

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[David Miller]

From: Bill Fink <billfink@mindspring.com>
Date: Wed, 18 Jun 2008 02:34:46 -0400

> I am not particularly familiar with the TCP MD5 option, but in googling
> around I found the following at the end of Section 2.2 Transmission,
> of RFC 4808, "TCP-MD5 Key Change", dated March 2007:
> 
>     "Note that there is an ambiguity when an acknowledgment is received
>     for a segment transmitted with two different keys.  The TCP Timestamp
>     option [RFC1323] can be used for disambiguation."
> 
> This would seem to imply, that at least in some scenarios, it would be
> advisable to have TS enabled in conjunction with MD5.

That does change the situation a bit, yes.

It's pretty easy, depending upon the timestamp resolution, to
end up with the original transmit and the retransmit having
the same timetsamp value.  So this suggestion in the RFC is
I think not a complete solution.

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[Adam Langley]

On Wed, Jun 18, 2008 at 1:52 AM, David Miller <davem@davemloft.net> wrote:
> It's pretty easy, depending upon the timestamp resolution, to
> end up with the original transmit and the retransmit having
> the same timetsamp value.  So this suggestion in the RFC is
> I think not a complete solution.

I happen to have just (unintentionally) simulated this, i.e. a
situation where MD5 signed packets with SACKs were dropped by the
receiving host. The transmitting host keeps sending packets with SACKs
and they keep getting dropped: the connection stalled.

AGL

-- 
Adam Langley agl@imperialviolet.org http://www.imperialviolet.org

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Wed, 18 Jun 2008 12:39:03 -0700

> On Wed, Jun 18, 2008 at 1:52 AM, David Miller <davem@davemloft.net> wrote:
> > It's pretty easy, depending upon the timestamp resolution, to
> > end up with the original transmit and the retransmit having
> > the same timetsamp value.  So this suggestion in the RFC is
> > I think not a complete solution.
> 
> I happen to have just (unintentionally) simulated this, i.e. a
> situation where MD5 signed packets with SACKs were dropped by the
> receiving host. The transmitting host keeps sending packets with SACKs
> and they keep getting dropped: the connection stalled.

The retransmit timeout timer has to fire eventually, and when
that happens we should clear all of our SACK state.

I'm not denying your observations, I'm just stating what ought
to be happening.

Comments requested: Long options and MD5 options

[Adam Langley]

> The retransmit timeout timer has to fire eventually, and when
> that happens we should clear all of our SACK state.

Ok, that's good at least. There were other variables in my test that could have
screwed that up.

Attached is a patch which pulls the options logic together into a single
function of SYN/SYNACK and one for established. At the moment, it chooses
SACK over timestamps for MD5 connections, but it's very easy to change. (And
that's the point of this patch - the logic isn't spread out)

Not signed off, because I want to do more testing, and to sleep on it first,
but pretty close to final.

 include/net/tcp.h     |    3 
 net/ipv4/tcp_output.c |  401 +++++++++++++++++++++++++++----------------------
 2 files changed, 222 insertions(+), 182 deletions(-)

---

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 633147c..35037d5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -154,6 +154,9 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 					 * timestamps. It must be less than
 					 * minimal timewait lifetime.
 					 */
+
+#define TCP_MAX_OPTION_SPACE	40	/* Max bytes of options */
+
 /*
  *	TCP option
  */
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..7aba4f4 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -347,112 +347,163 @@ static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
 	TCP_SKB_CB(skb)->end_seq = seq;
 }
 
-static void tcp_build_and_update_options(__be32 *ptr, struct tcp_sock *tp,
-					 __u32 tstamp, __u8 **md5_hash)
-{
+/* Build, or calculate the size of the TCP options for a SYN or SYNACK.
+ *   ptr: (maybe NULL) Location to write the options
+ *   synack: if non-zero, this is a SYNACK, otherwise a SYN.
+ *   md5_hash: If non-NULL, space is left for a MD5 signature and its location
+ *     is written here
+ *   prev_written: the return value of calling this function, immediately
+ *     prior. First time round, pass 0 here. */
+static unsigned tcp_build_syn_options(__be32 *ptr,
+				      char synack, int mss, int ts, int sack,
+				      int offer_wscale, int wscale,
+				      __u32 tstamp, __u32 ts_recent,
+				      __u8 **md5_hash, unsigned prev_written) {
+	/* No SACK blocks can fit in a packet with timestamps and MD5
+	 * signatures. Older Linux kernels have a bug where they try anyway
+	 * and corrupt the packets. We want to assuage this, so if we saw a SYN
+	 * packet with MD5 + SACK + TS, we assume it's an old kernel and reply
+	 * with MD5 + TS. However, SACK is more important than TS so we send
+	 * SYNs with MD5 + SACK. */
+	const char doing_sack = sack && (!synack || !(md5_hash && sack && ts));
+	const char doing_ts = ts && !(md5_hash && doing_sack);
+
+	unsigned written = 0;
+	__be32 temp;
+	const unsigned n = !ptr ? 0 : 1;
+	BUG_ON(prev_written == 0 && ptr);
+	if (!ptr)
+		ptr = &temp;
+
+	if (md5_hash) {
+		*ptr = htonl((TCPOPT_NOP << 24) |
+			     (TCPOPT_NOP << 16) |
+			     (TCPOPT_MD5SIG << 8) |
+			     TCPOLEN_MD5SIG);
+		ptr += n;
+		*md5_hash = (__u8 *) ptr;
+		ptr += n * 4;
+		written += 4 + 16;
+	}
+
+	*ptr = htonl((TCPOPT_MSS << 24) |
+		     (TCPOLEN_MSS << 16) |
+		     mss);
+	ptr += n;
+	written += 4;
+
+	if (doing_ts) {
+		if (doing_sack) {
+			*ptr = htonl((TCPOPT_SACK_PERM << 24) |
+				(TCPOLEN_SACK_PERM << 16) |
+				(TCPOPT_TIMESTAMP << 8) |
+				TCPOLEN_TIMESTAMP);
+			ptr += n;
+		} else {
+			*ptr = htonl((TCPOPT_NOP << 24) |
+				(TCPOPT_NOP << 16) |
+				(TCPOPT_TIMESTAMP << 8) |
+				TCPOLEN_TIMESTAMP);
+			ptr += n;
+		}
+
+		*ptr = htonl(tstamp);
+		ptr += n;
+		*ptr = htonl(ts_recent);
+		ptr += n;
+		written += 4 * 3;
+	} else if (doing_sack) {
+		*ptr = htonl((TCPOPT_NOP << 24) |
+			     (TCPOPT_NOP << 16) |
+			     (TCPOPT_SACK_PERM << 8) |
+			     (TCPOLEN_SACK_PERM));
+		ptr += n;
+		written += 4;
+	}
+	if (offer_wscale) {
+		*ptr = htonl((TCPOPT_NOP << 24) |
+			     (TCPOPT_WINDOW << 16) |
+			     (TCPOLEN_WINDOW << 8) |
+			     wscale);
+		ptr += n;
+		written += 4;
+	}
+
+	return written;
+}
+
+/* Build, or calculate the size of the TCP options.
+ *   ptr: (maybe NULL) Location to write the options
+ *   md5_hash: If non-NULL, space is left for a MD5 signature and its location
+ *     is written here
+ *   prev_written: the return value of calling this function, immediately
+ *     prior. First time round, pass 0 here. */
+static unsigned tcp_build_options(__be32 *ptr, const struct sock *sk,
+				  __u32 tstamp, __u8 **md5_hash,
+				  unsigned prev_written) {
+	struct tcp_sock *tp = tcp_sk(sk);
+	const unsigned sack_bytes = TCPOLEN_SACK_BASE_ALIGNED +
+	  (tp->rx_opt.eff_sacks * TCPOLEN_SACK_PERBLOCK);
+	unsigned written = 0;
+	__be32 temp;
+	const unsigned n = !ptr ? 0 : 1;
+	BUG_ON(prev_written == 0 && ptr);
+	if (!ptr)
+		ptr = &temp;
+
+	if (md5_hash) {
+		*ptr = htonl((TCPOPT_NOP << 24) |
+			     (TCPOPT_NOP << 16) |
+			     (TCPOPT_MD5SIG << 8) |
+			     TCPOLEN_MD5SIG);
+		ptr += n;
+		*md5_hash = (__u8 *) ptr;
+		ptr += n * 4;
+		written += 4 + 16;
+	}
+
 	if (tp->rx_opt.tstamp_ok) {
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_TIMESTAMP << 8) |
-			       TCPOLEN_TIMESTAMP);
-		*ptr++ = htonl(tstamp);
-		*ptr++ = htonl(tp->rx_opt.ts_recent);
+		*ptr = htonl((TCPOPT_NOP << 24) |
+			     (TCPOPT_NOP << 16) |
+			     (TCPOPT_TIMESTAMP << 8) |
+			     TCPOLEN_TIMESTAMP);
+		ptr += n;
+		*ptr = htonl(tstamp);
+		ptr += n;
+		*ptr = htonl(tp->rx_opt.ts_recent);
+		ptr += n;
+
+		written += 4 * 3;
 	}
-	if (tp->rx_opt.eff_sacks) {
-		struct tcp_sack_block *sp = tp->rx_opt.dsack ? tp->duplicate_sack : tp->selective_acks;
+
+	if (tp->rx_opt.eff_sacks &&
+	    (TCP_MAX_OPTION_SPACE - written >= sack_bytes)) {
+		const struct tcp_sack_block *sp =
+		  tp->rx_opt.dsack ? tp->duplicate_sack : tp->selective_acks;
 		int this_sack;
 
-		*ptr++ = htonl((TCPOPT_NOP  << 24) |
-			       (TCPOPT_NOP  << 16) |
-			       (TCPOPT_SACK <<  8) |
-			       (TCPOLEN_SACK_BASE + (tp->rx_opt.eff_sacks *
-						     TCPOLEN_SACK_PERBLOCK)));
+		*ptr = htonl((TCPOPT_NOP  << 24) |
+			     (TCPOPT_NOP  << 16) |
+			     (TCPOPT_SACK <<  8) |
+			     (sack_bytes - 2));
+		ptr += n;
 
 		for (this_sack = 0; this_sack < tp->rx_opt.eff_sacks; this_sack++) {
-			*ptr++ = htonl(sp[this_sack].start_seq);
-			*ptr++ = htonl(sp[this_sack].end_seq);
+			*ptr = htonl(sp[this_sack].start_seq);
+			ptr += n;
+			*ptr = htonl(sp[this_sack].end_seq);
+			ptr += n;
 		}
 
-		if (tp->rx_opt.dsack) {
+		if (prev_written && tp->rx_opt.dsack) {
 			tp->rx_opt.dsack = 0;
 			tp->rx_opt.eff_sacks--;
 		}
-	}
-#ifdef CONFIG_TCP_MD5SIG
-	if (md5_hash) {
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_MD5SIG << 8) |
-			       TCPOLEN_MD5SIG);
-		*md5_hash = (__u8 *)ptr;
-	}
-#endif
-}
 
-/* Construct a tcp options header for a SYN or SYN_ACK packet.
- * If this is every changed make sure to change the definition of
- * MAX_SYN_SIZE to match the new maximum number of options that you
- * can generate.
- *
- * Note - that with the RFC2385 TCP option, we make room for the
- * 16 byte MD5 hash. This will be filled in later, so the pointer for the
- * location to be filled is passed back up.
- */
-static void tcp_syn_build_options(__be32 *ptr, int mss, int ts, int sack,
-				  int offer_wscale, int wscale, __u32 tstamp,
-				  __u32 ts_recent, __u8 **md5_hash)
-{
-	/* We always get an MSS option.
-	 * The option bytes which will be seen in normal data
-	 * packets should timestamps be used, must be in the MSS
-	 * advertised.  But we subtract them from tp->mss_cache so
-	 * that calculations in tcp_sendmsg are simpler etc.
-	 * So account for this fact here if necessary.  If we
-	 * don't do this correctly, as a receiver we won't
-	 * recognize data packets as being full sized when we
-	 * should, and thus we won't abide by the delayed ACK
-	 * rules correctly.
-	 * SACKs don't matter, we never delay an ACK when we
-	 * have any of those going out.
-	 */
-	*ptr++ = htonl((TCPOPT_MSS << 24) | (TCPOLEN_MSS << 16) | mss);
-	if (ts) {
-		if (sack)
-			*ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
-				       (TCPOLEN_SACK_PERM << 16) |
-				       (TCPOPT_TIMESTAMP << 8) |
-				       TCPOLEN_TIMESTAMP);
-		else
-			*ptr++ = htonl((TCPOPT_NOP << 24) |
-				       (TCPOPT_NOP << 16) |
-				       (TCPOPT_TIMESTAMP << 8) |
-				       TCPOLEN_TIMESTAMP);
-		*ptr++ = htonl(tstamp);		/* TSVAL */
-		*ptr++ = htonl(ts_recent);	/* TSECR */
-	} else if (sack)
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_SACK_PERM << 8) |
-			       TCPOLEN_SACK_PERM);
-	if (offer_wscale)
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_WINDOW << 16) |
-			       (TCPOLEN_WINDOW << 8) |
-			       (wscale));
-#ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * If MD5 is enabled, then we set the option, and include the size
-	 * (always 18). The actual MD5 hash is added just before the
-	 * packet is sent.
-	 */
-	if (md5_hash) {
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_MD5SIG << 8) |
-			       TCPOLEN_MD5SIG);
-		*md5_hash = (__u8 *)ptr;
+		written += sack_bytes;
 	}
-#endif
+
+	return written;
 }
 
 /* This routine actually transmits TCP packets queued in by
@@ -473,15 +524,22 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	struct inet_sock *inet;
 	struct tcp_sock *tp;
 	struct tcp_skb_cb *tcb;
-	int tcp_header_size;
-#ifdef CONFIG_TCP_MD5SIG
-	struct tcp_md5sig_key *md5;
+	unsigned tcp_options_size, tcp_header_size;
+	struct tcp_md5sig_key *md5 = NULL;
 	__u8 *md5_hash_location;
-#endif
 	struct tcphdr *th;
-	int sysctl_flags;
 	int err;
 
+#define SYSCTL_FLAG_TSTAMPS  0x1
+#define SYSCTL_FLAG_WSCALE   0x2
+#define SYSCTL_FLAG_SACK  0x4
+
+	/* We can't have these changing during the duration of this function */
+	const int sysctl_flags =
+	  (sysctl_tcp_timestamps ? SYSCTL_FLAG_TSTAMPS : 0) |
+	  (sysctl_tcp_window_scaling ? SYSCTL_FLAG_WSCALE : 0) |
+	  (sysctl_tcp_sack ? SYSCTL_FLAG_SACK : 0);
+
 	BUG_ON(!skb || !tcp_skb_pcount(skb));
 
 	/* If congestion control is doing timestamping, we must
@@ -502,50 +560,32 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	inet = inet_sk(sk);
 	tp = tcp_sk(sk);
 	tcb = TCP_SKB_CB(skb);
-	tcp_header_size = tp->tcp_header_len;
 
-#define SYSCTL_FLAG_TSTAMPS	0x1
-#define SYSCTL_FLAG_WSCALE	0x2
-#define SYSCTL_FLAG_SACK	0x4
+#ifdef CONFIG_TCP_MD5SIG
+	md5 = tp->af_specific->md5_lookup(sk, sk);
+#endif
 
-	sysctl_flags = 0;
 	if (unlikely(tcb->flags & TCPCB_FLAG_SYN)) {
-		tcp_header_size = sizeof(struct tcphdr) + TCPOLEN_MSS;
-		if (sysctl_tcp_timestamps) {
-			tcp_header_size += TCPOLEN_TSTAMP_ALIGNED;
-			sysctl_flags |= SYSCTL_FLAG_TSTAMPS;
-		}
-		if (sysctl_tcp_window_scaling) {
-			tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
-			sysctl_flags |= SYSCTL_FLAG_WSCALE;
-		}
-		if (sysctl_tcp_sack) {
-			sysctl_flags |= SYSCTL_FLAG_SACK;
-			if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
-				tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
-		}
-	} else if (unlikely(tp->rx_opt.eff_sacks)) {
-		/* A SACK is 2 pad bytes, a 2 byte header, plus
-		 * 2 32-bit sequence numbers for each SACK block.
-		 */
-		tcp_header_size += (TCPOLEN_SACK_BASE_ALIGNED +
-				    (tp->rx_opt.eff_sacks *
-				     TCPOLEN_SACK_PERBLOCK));
+		tcp_options_size =
+		  tcp_build_syn_options(NULL, 0,
+					tcp_advertise_mss(sk),
+					sysctl_flags & SYSCTL_FLAG_TSTAMPS,
+					sysctl_flags & SYSCTL_FLAG_SACK,
+					sysctl_flags & SYSCTL_FLAG_WSCALE,
+					tp->rx_opt.rcv_wscale,
+					tcb->when,
+					tp->rx_opt.ts_recent,
+					md5 ? &md5_hash_location : NULL, 0);
+	} else {
+		tcp_options_size =
+		  tcp_build_options(NULL, sk, tcb->when,
+				    md5 ? &md5_hash_location : NULL, 0);
 	}
+	tcp_header_size = tcp_options_size + sizeof(struct tcphdr);
 
 	if (tcp_packets_in_flight(tp) == 0)
 		tcp_ca_event(sk, CA_EVENT_TX_START);
 
-#ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * Are we doing MD5 on this segment? If so - make
-	 * room for it.
-	 */
-	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 	skb_set_owner_w(skb, sk);
@@ -577,26 +617,20 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	}
 
 	if (unlikely(tcb->flags & TCPCB_FLAG_SYN)) {
-		tcp_syn_build_options((__be32 *)(th + 1),
+		tcp_build_syn_options((__be32 *)(th + 1), 0,
 				      tcp_advertise_mss(sk),
-				      (sysctl_flags & SYSCTL_FLAG_TSTAMPS),
-				      (sysctl_flags & SYSCTL_FLAG_SACK),
-				      (sysctl_flags & SYSCTL_FLAG_WSCALE),
+				      sysctl_flags & SYSCTL_FLAG_TSTAMPS,
+				      sysctl_flags & SYSCTL_FLAG_SACK,
+				      sysctl_flags & SYSCTL_FLAG_WSCALE,
 				      tp->rx_opt.rcv_wscale,
 				      tcb->when,
 				      tp->rx_opt.ts_recent,
-
-#ifdef CONFIG_TCP_MD5SIG
-				      md5 ? &md5_hash_location :
-#endif
-				      NULL);
+				      md5 ? &md5_hash_location : NULL,
+				      tcp_options_size);
 	} else {
-		tcp_build_and_update_options((__be32 *)(th + 1),
-					     tp, tcb->when,
-#ifdef CONFIG_TCP_MD5SIG
-					     md5 ? &md5_hash_location :
-#endif
-					     NULL);
+		tcp_build_options((__be32 *)(th + 1), sk, tcb->when,
+				  md5 ? &md5_hash_location : NULL,
+				  tcp_options_size);
 		TCP_ECN_send(sk, skb, tcp_header_size);
 	}
 
@@ -974,6 +1008,8 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 	u32 mss_now;
 	u16 xmit_size_goal;
 	int doing_tso = 0;
+	unsigned header_len;
+	__u8 **md5_ptr = NULL, *md5_hash;
 
 	mss_now = tp->mss_cache;
 
@@ -986,22 +1022,29 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 			mss_now = tcp_sync_mss(sk, mtu);
 	}
 
-	if (tp->rx_opt.eff_sacks)
-		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
-			    (tp->rx_opt.eff_sacks * TCPOLEN_SACK_PERBLOCK));
-
 #ifdef CONFIG_TCP_MD5SIG
 	if (tp->af_specific->md5_lookup(sk, sk))
-		mss_now -= TCPOLEN_MD5SIG_ALIGNED;
+		md5_ptr = &md5_hash;
 #endif
 
+	header_len = tcp_build_options(NULL, sk, 0, md5_ptr, 0) +
+		     sizeof(struct tcphdr);
+	/* The mss_cache is sized based on tp->tcp_header_len, which assumes
+	 * some common options. If this is an odd packet (because we have SACK
+	 * blocks etc) then our calculated header_len will be different, and
+	 * we have to adjust mss_now correspondingly */
+	if (header_len != tp->tcp_header_len) {
+		int delta = (int) header_len - tp->tcp_header_len;
+		mss_now -= delta;
+	}
+
 	xmit_size_goal = mss_now;
 
 	if (doing_tso) {
 		xmit_size_goal = ((sk->sk_gso_max_size - 1) -
 				  inet_csk(sk)->icsk_af_ops->net_header_len -
 				  inet_csk(sk)->icsk_ext_hdr_len -
-				  tp->tcp_header_len);
+				  header_len);
 
 		xmit_size_goal = tcp_bound_to_half_wnd(tp, xmit_size_goal);
 		xmit_size_goal -= (xmit_size_goal % mss_now);
@@ -2177,12 +2220,10 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	struct inet_request_sock *ireq = inet_rsk(req);
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct tcphdr *th;
-	int tcp_header_size;
+	int tcp_options_size, tcp_header_size;
 	struct sk_buff *skb;
-#ifdef CONFIG_TCP_MD5SIG
-	struct tcp_md5sig_key *md5;
+	struct tcp_md5sig_key *md5 = NULL;
 	__u8 *md5_hash_location;
-#endif
 
 	skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15, 1, GFP_ATOMIC);
 	if (skb == NULL)
@@ -2193,18 +2234,18 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 
 	skb->dst = dst_clone(dst);
 
-	tcp_header_size = (sizeof(struct tcphdr) + TCPOLEN_MSS +
-			   (ireq->tstamp_ok ? TCPOLEN_TSTAMP_ALIGNED : 0) +
-			   (ireq->wscale_ok ? TCPOLEN_WSCALE_ALIGNED : 0) +
-			   /* SACK_PERM is in the place of NOP NOP of TS */
-			   ((ireq->sack_ok && !ireq->tstamp_ok) ? TCPOLEN_SACKPERM_ALIGNED : 0));
-
 #ifdef CONFIG_TCP_MD5SIG
-	/* Are we doing MD5 on this segment? If so - make room for it */
 	md5 = tcp_rsk(req)->af_specific->md5_lookup(sk, req);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
 #endif
+
+	tcp_options_size =
+	  tcp_build_syn_options(NULL, 1, dst_metric(dst, RTAX_ADVMSS),
+				ireq->tstamp_ok, ireq->sack_ok,
+				ireq->wscale_ok, ireq->rcv_wscale,
+				TCP_SKB_CB(skb)->when, req->ts_recent,
+				md5 ? &md5_hash_location : NULL, 0);
+	tcp_header_size = tcp_options_size + sizeof(struct tcphdr);
+
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 
@@ -2244,18 +2285,14 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	else
 #endif
 	TCP_SKB_CB(skb)->when = tcp_time_stamp;
-	tcp_syn_build_options((__be32 *)(th + 1), dst_metric(dst, RTAX_ADVMSS), ireq->tstamp_ok,
-			      ireq->sack_ok, ireq->wscale_ok, ireq->rcv_wscale,
-			      TCP_SKB_CB(skb)->when,
-			      req->ts_recent,
-			      (
-#ifdef CONFIG_TCP_MD5SIG
-			       md5 ? &md5_hash_location :
-#endif
-			       NULL)
-			      );
-
-	th->doff = (tcp_header_size >> 2);
+	tcp_build_syn_options((__be32 *)(th + 1), 1, dst_metric(dst, RTAX_ADVMSS),
+			      ireq->tstamp_ok, ireq->sack_ok,
+			      ireq->wscale_ok, ireq->rcv_wscale,
+			      TCP_SKB_CB(skb)->when, req->ts_recent,
+			      md5 ? &md5_hash_location : NULL,
+			      tcp_options_size);
+
+	th->doff = tcp_header_size >> 2;
 	TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -2264,8 +2301,8 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 		tp->af_specific->calc_md5_hash(md5_hash_location,
 					       md5,
 					       NULL, dst, req,
-					       tcp_hdr(skb), sk->sk_protocol,
-					       skb->len);
+					       tcp_hdr(skb),
+					       sk->sk_protocol, skb->len);
 	}
 #endif
 

Re: Comments requested: Long options and MD5 options

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Wed, 18 Jun 2008 16:24:33 -0700

> > The retransmit timeout timer has to fire eventually, and when
> > that happens we should clear all of our SACK state.
> 
> Ok, that's good at least. There were other variables in my test that could have
> screwed that up.
> 
> Attached is a patch which pulls the options logic together into a single
> function of SYN/SYNACK and one for established. At the moment, it chooses
> SACK over timestamps for MD5 connections, but it's very easy to change. (And
> that's the point of this patch - the logic isn't spread out)
> 
> Not signed off, because I want to do more testing, and to sleep on it first,
> but pretty close to final.

Thanks for doing this work, I'll have a closer look at this
patch later this evening.

[PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[Adam Langley]

> Not signed off, because I want to do more testing, and to sleep on it first,
> but pretty close to final.

After thinking about it for a while, I decided that it might be better another
way. This involves a few more lines, but I think that it's probably cleaner.

Rather than a functions which are called twice, first to calculate the size and
then to write the actual bytes, we split into a functions which fill out a
tcp_out_options struct and have a single function to write all combinations of
options from that struct.

Just to be clear; this is an alternative from the previous patch, not another
patch on top.

(Also, I wish that I could get diff to output bigger chunks. This diff is
pretty hard to read because of the many small chunks that diff finds.)

Again, not signed off - works reasonably, but needs more testing.


 include/net/tcp.h     |    2
 net/ipv4/tcp_output.c |  414 +++++++++++++++++++++++++++----------------------
 2 files changed, 231 insertions(+), 185 deletions(-)

---

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 633147c..9164d2f 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -50,6 +50,7 @@ extern atomic_t tcp_orphan_count;
 extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 
 #define MAX_TCP_HEADER	(128 + MAX_HEADER)
+#define MAX_TCP_OPTION_SPACE 40
 
 /* 
  * Never offer a window over 32767 without using window scaling. Some
@@ -185,6 +186,7 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 #define TCPOLEN_SACK_BASE_ALIGNED	4
 #define TCPOLEN_SACK_PERBLOCK		8
 #define TCPOLEN_MD5SIG_ALIGNED		20
+#define TCPOLEN_MSS_ALIGNED		4
 
 /* Flags in tp->nonagle */
 #define TCP_NAGLE_OFF		1	/* Nagle's algo is disabled */
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index e399bde..ddac5bd 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -82,9 +82,9 @@ static void tcp_event_new_data_sent(struct sock *sk, struct sk_buff *skb)
 /* SND.NXT, if window was not shrunk.
  * If window has been shrunk, what should we make? It is not clear at all.
  * Using SND.UNA we will fail to open window, SND.NXT is out of window. :-(
- * Anything in between SND.UNA...SND.UNA+SND.WND also can be already
- * invalid. OK, let's make this for now:
- */
+		 * Anything in between SND.UNA...SND.UNA+SND.WND also can be already
+		 * invalid. OK, let's make this for now:
+		 */
 static inline __u32 tcp_acceptable_seq(struct sock *sk)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
@@ -296,7 +296,7 @@ static inline void TCP_ECN_send_syn(struct sock *sk, struct sk_buff *skb)
 	}
 }
 
-static __inline__ void
+	static __inline__ void
 TCP_ECN_make_synack(struct request_sock *req, struct tcphdr *th)
 {
 	if (inet_rsk(req)->ecn_ok)
@@ -347,28 +347,81 @@ static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
 	TCP_SKB_CB(skb)->end_seq = seq;
 }
 
-static void tcp_build_and_update_options(__be32 *ptr, struct tcp_sock *tp,
-					 __u32 tstamp, __u8 **md5_hash)
-{
-	if (tp->rx_opt.tstamp_ok) {
+#define OPTION_SACK_ADVERTISE	(1 << 0)
+#define OPTION_TS		(1 << 1)
+#define OPTION_MD5		(1 << 2)
+
+struct tcp_out_options {
+	unsigned options;		/* bit field of OPTION_* */
+	unsigned mss;			/* 0 to disable */
+	unsigned ws;			/* window scale, 0 to disable */
+	__u32 tsval, tsecr;		/* need to include OPTION_TS */
+	unsigned num_sack_blocks;	/* number of SACK blocks to include */
+};
+
+static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
+			      const struct tcp_out_options *opts,
+			      __u8 **md5_hash) {
+	if (unlikely(OPTION_MD5 & opts->options)) {
 		*ptr++ = htonl((TCPOPT_NOP << 24) |
 			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_TIMESTAMP << 8) |
-			       TCPOLEN_TIMESTAMP);
-		*ptr++ = htonl(tstamp);
-		*ptr++ = htonl(tp->rx_opt.ts_recent);
+			       (TCPOPT_MD5SIG << 8) |
+			       TCPOLEN_MD5SIG);
+		*md5_hash = (__u8 *)ptr;
+		ptr += 4;
+	} else {
+		*md5_hash = NULL;
 	}
-	if (tp->rx_opt.eff_sacks) {
-		struct tcp_sack_block *sp = tp->rx_opt.dsack ? tp->duplicate_sack : tp->selective_acks;
+
+	if (likely(OPTION_TS & opts->options)) {
+		if (unlikely(OPTION_SACK_ADVERTISE & opts->options)) {
+			*ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
+				       (TCPOLEN_SACK_PERM << 16) |
+				       (TCPOPT_TIMESTAMP << 8) |
+				       TCPOLEN_TIMESTAMP);
+		} else {
+			*ptr++ = htonl((TCPOPT_NOP << 24) |
+				       (TCPOPT_NOP << 16) |
+				       (TCPOPT_TIMESTAMP << 8) |
+				       TCPOLEN_TIMESTAMP);
+		}
+		*ptr++ = htonl(opts->tsval);
+		*ptr++ = htonl(opts->tsecr);
+	}
+
+	if (unlikely(opts->mss)) {
+		*ptr++ = htonl((TCPOPT_MSS << 24) |
+			       (TCPOLEN_MSS << 16) |
+			       opts->mss);
+	}
+
+	if (unlikely(OPTION_SACK_ADVERTISE & opts->options &&
+		     !(OPTION_TS & opts->options))) {
+		*ptr++ = htonl((TCPOPT_NOP << 24) |
+			       (TCPOPT_NOP << 16) |
+			       (TCPOPT_SACK_PERM << 8) |
+			       TCPOLEN_SACK_PERM);
+	}
+
+	if (unlikely(opts->ws)) {
+		*ptr++ = htonl((TCPOPT_NOP << 24) |
+			       (TCPOPT_WINDOW << 16) |
+			       (TCPOLEN_WINDOW << 8) |
+			       opts->ws);
+	}
+
+	if (unlikely(opts->num_sack_blocks)) {
+		struct tcp_sack_block *sp = tp->rx_opt.dsack ?
+			tp->duplicate_sack : tp->selective_acks;
 		int this_sack;
 
 		*ptr++ = htonl((TCPOPT_NOP  << 24) |
 			       (TCPOPT_NOP  << 16) |
 			       (TCPOPT_SACK <<  8) |
-			       (TCPOLEN_SACK_BASE + (tp->rx_opt.eff_sacks *
+			       (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
 						     TCPOLEN_SACK_PERBLOCK)));
 
-		for (this_sack = 0; this_sack < tp->rx_opt.eff_sacks; this_sack++) {
+		for (this_sack = 0; this_sack < opts->num_sack_blocks; ++this_sack) {
 			*ptr++ = htonl(sp[this_sack].start_seq);
 			*ptr++ = htonl(sp[this_sack].end_seq);
 		}
@@ -378,81 +431,144 @@ static void tcp_build_and_update_options(__be32 *ptr, struct tcp_sock *tp,
 			tp->rx_opt.eff_sacks--;
 		}
 	}
+}
+
+static unsigned tcp_syn_options(struct sock *sk, struct sk_buff *skb,
+				struct tcp_out_options *opts,
+				struct tcp_md5sig_key **md5) {
+	struct tcp_sock *tp = tcp_sk(sk);
+	unsigned size = 0;
+
 #ifdef CONFIG_TCP_MD5SIG
-	if (md5_hash) {
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_MD5SIG << 8) |
-			       TCPOLEN_MD5SIG);
-		*md5_hash = (__u8 *)ptr;
+	*md5 = tp->af_specific->md5_lookup(sk, sk);
+	if (*md5) {
+		opts->options |= OPTION_MD5;
+		size += TCPOLEN_MD5SIG_ALIGNED;
 	}
+#else
+	*md5 = NULL;
 #endif
+
+	/* We always get an MSS option.  The option bytes which will be seen in
+	 * normal data packets should timestamps be used, must be in the MSS
+	 * advertised.  But we subtract them from tp->mss_cache so that
+	 * calculations in tcp_sendmsg are simpler etc.  So account for this
+	 * fact here if necessary.  If we don't do this correctly, as a
+	 * receiver we won't recognize data packets as being full sized when we
+	 * should, and thus we won't abide by the delayed ACK rules correctly.
+	 * SACKs don't matter, we never delay an ACK when we have any of those
+	 * going out.  */
+	opts->mss = tcp_advertise_mss(sk);
+	size += TCPOLEN_MSS_ALIGNED;
+
+	if (likely(sysctl_tcp_timestamps) && !*md5) {
+		opts->options |= OPTION_TS;
+		opts->tsval = TCP_SKB_CB(skb)->when;
+		opts->tsecr = tp->rx_opt.ts_recent;
+		size += TCPOLEN_TSTAMP_ALIGNED;
+	}
+	if (likely(sysctl_tcp_window_scaling)) {
+		opts->ws = tp->rx_opt.rcv_wscale;
+		size += TCPOLEN_WSCALE_ALIGNED;
+	}
+	if (likely(sysctl_tcp_sack)) {
+		opts->options |= OPTION_SACK_ADVERTISE;
+		if (unlikely(!OPTION_TS & opts->options))
+			size += TCPOLEN_SACKPERM_ALIGNED;
+	}
+
+	return size;
 }
 
-/* Construct a tcp options header for a SYN or SYN_ACK packet.
- * If this is every changed make sure to change the definition of
- * MAX_SYN_SIZE to match the new maximum number of options that you
- * can generate.
- *
- * Note - that with the RFC2385 TCP option, we make room for the
- * 16 byte MD5 hash. This will be filled in later, so the pointer for the
- * location to be filled is passed back up.
- */
-static void tcp_syn_build_options(__be32 *ptr, int mss, int ts, int sack,
-				  int offer_wscale, int wscale, __u32 tstamp,
-				  __u32 ts_recent, __u8 **md5_hash)
-{
-	/* We always get an MSS option.
-	 * The option bytes which will be seen in normal data
-	 * packets should timestamps be used, must be in the MSS
-	 * advertised.  But we subtract them from tp->mss_cache so
-	 * that calculations in tcp_sendmsg are simpler etc.
-	 * So account for this fact here if necessary.  If we
-	 * don't do this correctly, as a receiver we won't
-	 * recognize data packets as being full sized when we
-	 * should, and thus we won't abide by the delayed ACK
-	 * rules correctly.
-	 * SACKs don't matter, we never delay an ACK when we
-	 * have any of those going out.
-	 */
-	*ptr++ = htonl((TCPOPT_MSS << 24) | (TCPOLEN_MSS << 16) | mss);
-	if (ts) {
-		if (sack)
-			*ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
-				       (TCPOLEN_SACK_PERM << 16) |
-				       (TCPOPT_TIMESTAMP << 8) |
-				       TCPOLEN_TIMESTAMP);
-		else
-			*ptr++ = htonl((TCPOPT_NOP << 24) |
-				       (TCPOPT_NOP << 16) |
-				       (TCPOPT_TIMESTAMP << 8) |
-				       TCPOLEN_TIMESTAMP);
-		*ptr++ = htonl(tstamp);		/* TSVAL */
-		*ptr++ = htonl(ts_recent);	/* TSECR */
-	} else if (sack)
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_SACK_PERM << 8) |
-			       TCPOLEN_SACK_PERM);
-	if (offer_wscale)
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_WINDOW << 16) |
-			       (TCPOLEN_WINDOW << 8) |
-			       (wscale));
+static unsigned tcp_synack_options(struct sock *sk,
+				   struct request_sock *req,
+				   unsigned mss, struct sk_buff *skb,
+				   struct tcp_out_options *opts,
+				   struct tcp_md5sig_key **md5) {
+	unsigned size = 0;
+	struct inet_request_sock *ireq = inet_rsk(req);
+	char doing_sack, doing_ts;
+	
 #ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * If MD5 is enabled, then we set the option, and include the size
-	 * (always 18). The actual MD5 hash is added just before the
-	 * packet is sent.
-	 */
-	if (md5_hash) {
-		*ptr++ = htonl((TCPOPT_NOP << 24) |
-			       (TCPOPT_NOP << 16) |
-			       (TCPOPT_MD5SIG << 8) |
-			       TCPOLEN_MD5SIG);
-		*md5_hash = (__u8 *)ptr;
+	*md5 = tcp_rsk(req)->af_specific->md5_lookup(sk, req);
+	if (*md5) {
+		opts->options |= OPTION_MD5;
+		size += TCPOLEN_MD5SIG_ALIGNED;
 	}
+#else
+	*md5 = NULL;
 #endif
+
+	/* No SACK blocks can fit in a packet with timestamps and MD5
+	 * signatures. Older Linux kernels have a bug where they try anyway
+	 * and corrupt the packets. We want to assuage this, so if we saw a SYN
+	 * packet with MD5 + SACK + TS, we assume it's an old kernel and reply
+	 * with MD5 + TS. However, SACK is more important than TS so we send
+	 * SYNs with MD5 + SACK. */
+	doing_sack = ireq->sack_ok && !(*md5 && ireq->sack_ok && ireq->tstamp_ok);
+	doing_ts = ireq->tstamp_ok && !(*md5 && doing_sack);
+
+	opts->mss = mss;
+	size += TCPOLEN_MSS_ALIGNED;
+
+	if (likely(ireq->wscale_ok)) {
+		opts->ws = ireq->rcv_wscale;
+		size += TCPOLEN_WSCALE_ALIGNED;
+	}
+
+	if (likely(doing_ts)) {
+		opts->options |= OPTION_TS;
+		opts->tsval = TCP_SKB_CB(skb)->when;
+		opts->tsecr = req->ts_recent;
+		size += TCPOLEN_TSTAMP_ALIGNED;
+	}
+	if (likely(doing_sack)) {
+		opts->options |= OPTION_SACK_ADVERTISE;
+		if (unlikely(!doing_ts)) {
+			size += TCPOLEN_SACKPERM_ALIGNED;
+		}
+	}
+
+	return size;
+}
+
+static unsigned tcp_established_options(struct sock *sk, struct sk_buff *skb,
+					struct tcp_out_options *opts,
+					struct tcp_md5sig_key **md5) {
+	struct tcp_skb_cb *tcb = skb ? TCP_SKB_CB(skb) : NULL;
+	struct tcp_sock *tp = tcp_sk(sk);
+	unsigned size = 0;
+
+#ifdef CONFIG_TCP_MD5SIG
+	*md5 = tp->af_specific->md5_lookup(sk, sk);
+	if (unlikely(*md5)) {
+		opts->options |= OPTION_MD5;
+		size += TCPOLEN_MD5SIG_ALIGNED;
+	}
+#else
+	*md5 = NULL;
+#endif
+
+	if (likely(tp->rx_opt.tstamp_ok)) {
+		opts->options |= OPTION_TS;
+		opts->tsval = tcb ? tcb->when : 0;
+		opts->tsecr = tp->rx_opt.ts_recent;
+		size += TCPOLEN_TSTAMP_ALIGNED;
+	}
+
+	if (unlikely(tp->rx_opt.eff_sacks)) {
+		const unsigned remaining = MAX_TCP_OPTION_SPACE - size;
+		if (likely(remaining >= TCPOLEN_SACK_BASE + TCPOLEN_SACK_PERBLOCK)) {
+			opts->num_sack_blocks =
+				min_t(unsigned, tp->rx_opt.eff_sacks,
+				      (remaining - TCPOLEN_SACK_BASE) /
+				      TCPOLEN_SACK_PERBLOCK);
+			size += TCPOLEN_SACK_BASE +
+				opts->num_sack_blocks * TCPOLEN_SACK_PERBLOCK;
+		}
+	}
+
+	return size;
 }
 
 /* This routine actually transmits TCP packets queued in by
@@ -473,13 +589,11 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	struct inet_sock *inet;
 	struct tcp_sock *tp;
 	struct tcp_skb_cb *tcb;
-	int tcp_header_size;
-#ifdef CONFIG_TCP_MD5SIG
+	struct tcp_out_options opts;
+	unsigned tcp_options_size, tcp_header_size;
 	struct tcp_md5sig_key *md5;
 	__u8 *md5_hash_location;
-#endif
 	struct tcphdr *th;
-	int sysctl_flags;
 	int err;
 
 	BUG_ON(!skb || !tcp_skb_pcount(skb));
@@ -502,50 +616,17 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	inet = inet_sk(sk);
 	tp = tcp_sk(sk);
 	tcb = TCP_SKB_CB(skb);
-	tcp_header_size = tp->tcp_header_len;
+	memset(&opts, 0, sizeof(opts));
 
-#define SYSCTL_FLAG_TSTAMPS	0x1
-#define SYSCTL_FLAG_WSCALE	0x2
-#define SYSCTL_FLAG_SACK	0x4
-
-	sysctl_flags = 0;
-	if (unlikely(tcb->flags & TCPCB_FLAG_SYN)) {
-		tcp_header_size = sizeof(struct tcphdr) + TCPOLEN_MSS;
-		if (sysctl_tcp_timestamps) {
-			tcp_header_size += TCPOLEN_TSTAMP_ALIGNED;
-			sysctl_flags |= SYSCTL_FLAG_TSTAMPS;
-		}
-		if (sysctl_tcp_window_scaling) {
-			tcp_header_size += TCPOLEN_WSCALE_ALIGNED;
-			sysctl_flags |= SYSCTL_FLAG_WSCALE;
-		}
-		if (sysctl_tcp_sack) {
-			sysctl_flags |= SYSCTL_FLAG_SACK;
-			if (!(sysctl_flags & SYSCTL_FLAG_TSTAMPS))
-				tcp_header_size += TCPOLEN_SACKPERM_ALIGNED;
-		}
-	} else if (unlikely(tp->rx_opt.eff_sacks)) {
-		/* A SACK is 2 pad bytes, a 2 byte header, plus
-		 * 2 32-bit sequence numbers for each SACK block.
-		 */
-		tcp_header_size += (TCPOLEN_SACK_BASE_ALIGNED +
-				    (tp->rx_opt.eff_sacks *
-				     TCPOLEN_SACK_PERBLOCK));
-	}
+	if (unlikely(tcb->flags & TCPCB_FLAG_SYN))
+		tcp_options_size = tcp_syn_options(sk, skb, &opts, &md5);
+	else
+		tcp_options_size = tcp_established_options(sk, skb, &opts, &md5);
+	tcp_header_size = tcp_options_size + sizeof(struct tcphdr);
 
 	if (tcp_packets_in_flight(tp) == 0)
 		tcp_ca_event(sk, CA_EVENT_TX_START);
 
-#ifdef CONFIG_TCP_MD5SIG
-	/*
-	 * Are we doing MD5 on this segment? If so - make
-	 * room for it.
-	 */
-	md5 = tp->af_specific->md5_lookup(sk, sk);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
-
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 	skb_set_owner_w(skb, sk);
@@ -576,29 +657,9 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 		th->urg			= 1;
 	}
 
-	if (unlikely(tcb->flags & TCPCB_FLAG_SYN)) {
-		tcp_syn_build_options((__be32 *)(th + 1),
-				      tcp_advertise_mss(sk),
-				      (sysctl_flags & SYSCTL_FLAG_TSTAMPS),
-				      (sysctl_flags & SYSCTL_FLAG_SACK),
-				      (sysctl_flags & SYSCTL_FLAG_WSCALE),
-				      tp->rx_opt.rcv_wscale,
-				      tcb->when,
-				      tp->rx_opt.ts_recent,
-
-#ifdef CONFIG_TCP_MD5SIG
-				      md5 ? &md5_hash_location :
-#endif
-				      NULL);
-	} else {
-		tcp_build_and_update_options((__be32 *)(th + 1),
-					     tp, tcb->when,
-#ifdef CONFIG_TCP_MD5SIG
-					     md5 ? &md5_hash_location :
-#endif
-					     NULL);
+	tcp_options_write((__be32 *)(th + 1), tp, &opts, &md5_hash_location);
+	if (likely((tcb->flags & TCPCB_FLAG_SYN) == 0))
 		TCP_ECN_send(sk, skb, tcp_header_size);
-	}
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* Calculate the MD5 hash, as we have all we need now */
@@ -630,10 +691,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	tcp_enter_cwr(sk, 1);
 
 	return net_xmit_eval(err);
-
-#undef SYSCTL_FLAG_TSTAMPS
-#undef SYSCTL_FLAG_WSCALE
-#undef SYSCTL_FLAG_SACK
 }
 
 /* This routine just queue's the buffer
@@ -974,6 +1031,9 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 	u32 mss_now;
 	u16 xmit_size_goal;
 	int doing_tso = 0;
+	unsigned header_len;
+	struct tcp_out_options opts;
+	struct tcp_md5sig_key *md5;
 
 	mss_now = tp->mss_cache;
 
@@ -986,14 +1046,16 @@ unsigned int tcp_current_mss(struct sock *sk, int large_allowed)
 			mss_now = tcp_sync_mss(sk, mtu);
 	}
 
-	if (tp->rx_opt.eff_sacks)
-		mss_now -= (TCPOLEN_SACK_BASE_ALIGNED +
-			    (tp->rx_opt.eff_sacks * TCPOLEN_SACK_PERBLOCK));
-
-#ifdef CONFIG_TCP_MD5SIG
-	if (tp->af_specific->md5_lookup(sk, sk))
-		mss_now -= TCPOLEN_MD5SIG_ALIGNED;
-#endif
+	header_len = tcp_established_options(sk, NULL, &opts, &md5) +
+	             sizeof(struct tcphdr);
+	/* The mss_cache is sized based on tp->tcp_header_len, which assumes
+	 * some common options. If this is an odd packet (because we have SACK
+	 * blocks etc) then our calculated header_len will be different, and
+	 * we have to adjust mss_now correspondingly */
+	if (header_len != tp->tcp_header_len) {
+	        int delta = (int) header_len - tp->tcp_header_len;
+	        mss_now -= delta;
+	}
 
 	xmit_size_goal = mss_now;
 
@@ -2178,11 +2240,10 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct tcphdr *th;
 	int tcp_header_size;
+	struct tcp_out_options opts;
 	struct sk_buff *skb;
-#ifdef CONFIG_TCP_MD5SIG
 	struct tcp_md5sig_key *md5;
 	__u8 *md5_hash_location;
-#endif
 
 	skb = sock_wmalloc(sk, MAX_TCP_HEADER + 15, 1, GFP_ATOMIC);
 	if (skb == NULL)
@@ -2193,18 +2254,12 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 
 	skb->dst = dst_clone(dst);
 
-	tcp_header_size = (sizeof(struct tcphdr) + TCPOLEN_MSS +
-			   (ireq->tstamp_ok ? TCPOLEN_TSTAMP_ALIGNED : 0) +
-			   (ireq->wscale_ok ? TCPOLEN_WSCALE_ALIGNED : 0) +
-			   /* SACK_PERM is in the place of NOP NOP of TS */
-			   ((ireq->sack_ok && !ireq->tstamp_ok) ? TCPOLEN_SACKPERM_ALIGNED : 0));
+	memset(&opts, 0, sizeof(opts));
+	TCP_SKB_CB(skb)->when = tcp_time_stamp;
+	tcp_header_size = tcp_synack_options(sk, req, dst_metric(dst, RTAX_ADVMSS),
+					     skb, &opts, &md5) +
+			  sizeof(struct tcphdr);
 
-#ifdef CONFIG_TCP_MD5SIG
-	/* Are we doing MD5 on this segment? If so - make room for it */
-	md5 = tcp_rsk(req)->af_specific->md5_lookup(sk, req);
-	if (md5)
-		tcp_header_size += TCPOLEN_MD5SIG_ALIGNED;
-#endif
 	skb_push(skb, tcp_header_size);
 	skb_reset_transport_header(skb);
 
@@ -2243,18 +2298,7 @@ struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
 		TCP_SKB_CB(skb)->when = cookie_init_timestamp(req);
 	else
 #endif
-	TCP_SKB_CB(skb)->when = tcp_time_stamp;
-	tcp_syn_build_options((__be32 *)(th + 1), dst_metric(dst, RTAX_ADVMSS), ireq->tstamp_ok,
-			      ireq->sack_ok, ireq->wscale_ok, ireq->rcv_wscale,
-			      TCP_SKB_CB(skb)->when,
-			      req->ts_recent,
-			      (
-#ifdef CONFIG_TCP_MD5SIG
-			       md5 ? &md5_hash_location :
-#endif
-			       NULL)
-			      );
-
+	tcp_options_write((__be32 *)(th + 1), tp, &opts, &md5_hash_location);
 	th->doff = (tcp_header_size >> 2);
 	TCP_INC_STATS(TCP_MIB_OUTSEGS);
 

Re: [PATCH] Fix corrupt TCP packets when options space overflows with MD5SIG enabled (v2)

[David Miller]

From: "Adam Langley" <agl@imperialviolet.org>
Date: Fri, 20 Jun 2008 13:27:23 -0700

> @@ -296,7 +296,7 @@ static inline void TCP_ECN_send_syn(struct sock *sk, struct sk_buff *skb)
>  	}
>  }
>  
> -static __inline__ void
> +	static __inline__ void
>  TCP_ECN_make_synack(struct request_sock *req, struct tcphdr *th)
>  {
>  	if (inet_rsk(req)->ecn_ok)
>  

What's this new TAB for?