Subject: [tcpm] Window update handling in the BSD TCP stack

From: Lawrence Stewart <[email-masked]>
Date: Thu, 14 May 2015 23:38:06 +1000

Hi all,

I've been contemplating the window update code in FreeBSD's TCP input
path (see [1] for full context):

> 2796 	/*
> 2797 	* Update window information.
> 2798 	* Don't look at window if no ACK: TAC's send garbage on first SYN.
> 2799 	*/
> 2800 	if ((thflags & TH_ACK) &&
> 2801 	    (SEQ_LT(tp->snd_wl1, th->th_seq) ||
> 2802 	    (tp->snd_wl1 == th->th_seq && (SEQ_LT(tp->snd_wl2, th->th_ack) ||
> 2803 	    (tp->snd_wl2 == th->th_ack && tiwin > tp->snd_wnd))))) {
> 2804 		/* keep track of pure window updates */
> 2805 		if (tlen == 0 &&
> 2806 		    tp->snd_wl2 == th->th_ack && tiwin > tp->snd_wnd)
> 2807 			TCPSTAT_INC(tcps_rcvwinupd);
> 2808 		tp->snd_wnd = tiwin;
> 2809 		tp->snd_wl1 = th->th_seq;
> 2810 		tp->snd_wl2 = th->th_ack;
> 2811 		if (tp->snd_wnd > tp->max_sndwnd)
> 2812 		tp->max_sndwnd = tp->snd_wnd;
> 2813 		needoutput = 1;
> 2814 	}

The code is documented essentially as above in Stevens Vol II and
reflects RFC 793 pg 72 - it has been around a long time.

I have some concerns that the logic is too restrictive in some
situations, and would like to propose the following for discussion:

>         /*
>          * Update window information if it's an ACK (TAC's send garbage on
>          * first SYN) and:
>          *   - The peer has ACKed (snd_wl2 < th_ack) or sent (snd_wl1 < th_seq)
>          *     new data OR
>          *   - A pure window update (snd_wl1 == th_ack && snd_wl2 == th_seq &&
>          *     tiwn != snd_wnd) with a new timestamp or if not doing timestamps,
>          *     we special case zero window handling to ensure we'll enter persist.
>          */
>         if ((thflags & TH_ACK) &&
>             (SEQ_LT(tp->snd_wl1, th->th_seq) ||
>             SEQ_LT(tp->snd_wl2, th->th_ack) ||
>             (tp->snd_wl1 == th->th_seq && tp->snd_wl2 == th->th_ack &&
>             tiwin != tp->snd_wnd &&
>             (tsrecentupdated || (!tiwin && !(tp->t_flags & TF_RCVD_TSTMP)))))) {
>                 /* keep track of pure window updates */
>                 if (tlen == 0 && tp->snd_wl2 == th->th_ack &&
>                     tiwin != tp->snd_wnd)
>                         TCPSTAT_INC(tcps_rcvwinupd);
>                 tp->snd_wnd = tiwin;
>                 tp->snd_wl1 = th->th_seq;
>                 tp->snd_wl2 = th->th_ack;
>                 if (tp->snd_wnd > tp->max_sndwnd)
>                         tp->max_sndwnd = tp->snd_wnd;
>                 needoutput = 1;
>         }

The tsrecentupdated variable is new, and I default it to 0 but update to
1 anywhere in tcp_do_segment() where tp->ts_recent is set to the
incoming segment's tsval.

The main motivation for the change is correct 0 window handling (the
details of which I'll be vague about as this investigation is related to
a DoS vector I'm working on a fix for).

In the spirit of being liberal with what you accept, why shouldn't we
accept pure window updates that shrink the window if we think they
reflect the most up to date information from the peer (or in the case of
no timestamps, reflect information we could act on to be safe)?

Cheers,
Lawrence

[1]
https://svnweb.freebsd.org/base/head/sys/netinet/tcp_input.c?view=markup#l2796