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

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

[Frank van der Linden]

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 | 4 ++++
 net/ipv6/tcp_ipv6.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index f70586b..ef8cd0f 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1689,6 +1689,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
 			reqsk_put(req);
 			goto discard_it;
 		}
+		if (tcp_checksum_complete(skb)) {
+			reqsk_put(req);
+			goto csum_error;
+		}
 		if (unlikely(sk->sk_state != TCP_LISTEN)) {
 			inet_csk_reqsk_queue_drop_and_put(sk, req);
 			goto lookup;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6d664d8..5d4eb9d 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1475,6 +1475,10 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 			reqsk_put(req);
 			goto discard_it;
 		}
+		if (tcp_checksum_complete(skb)) {
+			reqsk_put(req);
+			goto csum_error;
+		}
 		if (unlikely(sk->sk_state != TCP_LISTEN)) {
 			inet_csk_reqsk_queue_drop_and_put(sk, req);
 			goto lookup;
-- 
1.8.3.1

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

[Eric Dumazet]

On 06/12/2018 04:09 PM, Frank van der Linden 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>

Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks !

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

[Balbir Singh]

On Wed, Jun 13, 2018 at 9:09 AM, 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 | 4 ++++
>  net/ipv6/tcp_ipv6.c | 4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index f70586b..ef8cd0f 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1689,6 +1689,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
>                         reqsk_put(req);
>                         goto discard_it;
>                 }
> +               if (tcp_checksum_complete(skb)) {
> +                       reqsk_put(req);
> +                       goto csum_error;
> +               }
>                 if (unlikely(sk->sk_state != TCP_LISTEN)) {
>                         inet_csk_reqsk_queue_drop_and_put(sk, req);
>                         goto lookup;
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 6d664d8..5d4eb9d 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1475,6 +1475,10 @@ static int tcp_v6_rcv(struct sk_buff *skb)
>                         reqsk_put(req);
>                         goto discard_it;
>                 }
> +               if (tcp_checksum_complete(skb)) {
> +                       reqsk_put(req);
> +                       goto csum_error;
> +               }
>                 if (unlikely(sk->sk_state != TCP_LISTEN)) {
>                         inet_csk_reqsk_queue_drop_and_put(sk, req);
>                         goto lookup;


I've tested the IPv4 variant with some changes

Tested-by: Balbir Singh <bsingharora@gmail.com>
Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Balbir

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

[David Miller]

From: Frank van der Linden <fllinden@amazon.com>
Date: Tue, 12 Jun 2018 23:09:37 +0000

> 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>

Applied and queued up for -stable.

I know you mention the bug causing commit in your commit message,
but you should also still provide a proper Fixes: tag.  I took
care of it for you this time.

Thanks.

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

[van der Linden, Frank]

On 6/14/18 5:05 PM, David Miller wrote:
> From: Frank van der Linden <fllinden@amazon.com>
> Date: Tue, 12 Jun 2018 23:09:37 +0000
>
>> 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>
> Applied and queued up for -stable.
>
> I know you mention the bug causing commit in your commit message,
> but you should also still provide a proper Fixes: tag.  I took
> care of it for you this time.
Thanks Dave, and thanks for reminding me of the Fixes: tag. Will add it
next time.

- Frank