Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection

["van der Linden, Frank" <fllinden@amazon.com>]

A few comments on this one:

- obviously this is fairly serious, as it can let corrupted data all the way up to the application
- I am not nuts about the patch itself, the code feels a bit cluttered, but it's the least invasive way
  I could think of. Probably some refactoring is needed at some point.
- here's how to easily reproduce it:

On the sender, set up artificial packet corruption:

#!/bin/sh
tc qdisc add dev eth0 root handle 1: prio
tc qdisc add dev eth0 parent 1:3 netem corrupt 50.0%
tc filter add dev eth0 protocol ip parent 1:0 prio 3 u32 match ip dst $DSTADDR flowid 10:3


Then, run the following on the sender:

while :; do dd if=/dev/zero of=/dev/stdout bs=4096 count=4 | nc $DSTADDR 10000; sleep 1; done

..and this on the receiver:

uname -r; grep ^Tcp /proc/net/snmp; ttl=$((${SECONDS} + 300)); while [[ ${SECONDS} -lt ${ttl} ]]; do out="foo.$(date +%s)"; nc -l 10000 > "${out}"; md5=$(md5sum "${out}"|cut -d\  -f 1); echo -n "${md5}"; if [[ "${md5}" != "ce338fe6899778aacfc28414f2d9498b" ]]; then echo " corrupted"; else echo; fi; done; grep ^Tcp /proc/net/snmp

[obviously, if you change the size / content, the md5 sum has to be changed here]

This reproduces it fairly quickly (within 20 seconds) for us, on 4.14.x kernels. 4.17 kernels were verified to still have the issue.

Frank

On 6/11/18, 4:15 PM, "Frank van der Linden" <fllinden@amazon.com> wrote:

    commit 079096f103fa ("tcp/dccp: install syn_recv requests into ehash
    table") introduced an optimization for the handling of child sockets
    created for a new TCP connection.
    
    But this optimization passes any data associated with the last ACK of the
    connection handshake up the stack without verifying its checksum, because it
    calls tcp_child_process(), which in turn calls tcp_rcv_state_process()
    directly.  These lower-level processing functions do not do any checksum
    verification.
    
    Insert a tcp_checksum_complete call in the TCP_NEW_SYN_RECEIVE path to
    fix this.
    
    Signed-off-by: Frank van der Linden <fllinden@amazon.com>
    ---
     net/ipv4/tcp_ipv4.c | 8 +++++++-
     net/ipv6/tcp_ipv6.c | 8 +++++++-
     2 files changed, 14 insertions(+), 2 deletions(-)
    
    diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
    index f70586b50838..1ec4c0d4aba5 100644
    --- a/net/ipv4/tcp_ipv4.c
    +++ b/net/ipv4/tcp_ipv4.c
    @@ -1703,7 +1703,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
     			th = (const struct tcphdr *)skb->data;
     			iph = ip_hdr(skb);
     			tcp_v4_fill_cb(skb, iph, th);
    -			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
    +
    +			if (tcp_checksum_complete(skb)) {
    +				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
    +			} else {
    +				nsk = tcp_check_req(sk, skb, req, false,
    +						    &req_stolen);
    +			}
     		}
     		if (!nsk) {
     			reqsk_put(req);
    diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
    index 6d664d83cd16..a12b694d3d1e 100644
    --- a/net/ipv6/tcp_ipv6.c
    +++ b/net/ipv6/tcp_ipv6.c
    @@ -1486,7 +1486,13 @@ static int tcp_v6_rcv(struct sk_buff *skb)
     			th = (const struct tcphdr *)skb->data;
     			hdr = ipv6_hdr(skb);
     			tcp_v6_fill_cb(skb, hdr, th);
    -			nsk = tcp_check_req(sk, skb, req, false, &req_stolen);
    +
    +			if (tcp_checksum_complete(skb)) {
    +				__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
    +			} else {
    +				nsk = tcp_check_req(sk, skb, req, false,
    +						    &req_stolen);
    +			}
     		}
     		if (!nsk) {
     			reqsk_put(req);
    -- 
    2.14.4