Re: [PATCH v3] tcp: verify the checksum of the first data segment in a new connection
On 6/14/18 5:05 PM, David Miller wrote:
> From: Frank van der Linden
> Date: Tue, 12 Jun 2018 23:09:37 +
>
>> 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
> 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
Re: [PATCH v2] tcp: verify the checksum of the first data segment in a new connection
Ok, patch v3 sent. It was rightly pointed out to me that I shouldn't commit the mortal sin of top posting - but bear with me guys, I'll dig up my 25-year old .muttrc :-) Frank On 6/12/18, 3:03 PM, "Eric Dumazet" wrote: On 06/12/2018 02:53 PM, van der Linden, Frank wrote: > The convention seems to be to call tcp_checksum_complete after tcp_filter has a chance to deal with the packet. I wanted to preserve that. > > If that is not a concern, then I agree that this is a far better way to go. > > Frank Given that we can drop the packet earlier from : if (skb_checksum_init(skb, IPPROTO_TCP, inet_compute_pseudo)) goto csum_error; I am quite sure we really do not care of tcp_filter() being hit or not by packets with bad checksum. Thanks
Re: [PATCH v2] tcp: verify the checksum of the first data segment in a new connection
Sure, fair enough. I was assuming there might be a reason of why tcp_filter was always done after the data (not pseudo header) checksum. If there isn't (and obviously the the possible MD5 checks are done before it too), then that's definitely the right thing to do. I'll resend. Though if you have the simpler change already lined up, I'll happily refrain from sending it myself. Frank On 6/12/18, 3:03 PM, "Eric Dumazet" wrote: On 06/12/2018 02:53 PM, van der Linden, Frank wrote: > The convention seems to be to call tcp_checksum_complete after tcp_filter has a chance to deal with the packet. I wanted to preserve that. > > If that is not a concern, then I agree that this is a far better way to go. > > Frank Given that we can drop the packet earlier from : if (skb_checksum_init(skb, IPPROTO_TCP, inet_compute_pseudo)) goto csum_error; I am quite sure we really do not care of tcp_filter() being hit or not by packets with bad checksum. Thanks
Re: [PATCH v2] tcp: verify the checksum of the first data segment in a new connection
The convention seems to be to call tcp_checksum_complete after tcp_filter has a
chance to deal with the packet. I wanted to preserve that.
If that is not a concern, then I agree that this is a far better way to go.
Frank
On 6/12/18, 2:50 PM, "Eric Dumazet" wrote:
On 06/12/2018 02:41 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
This is way too complicated.
You should call tcp_checksum_complete() earlier and avoid all this mess.
IPV4 part shown here :
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index
fed3f1c6616708997f621535efe9412e4afa0a50..7b5f32aa3835b0124b0a9bd342c371df7b46f471
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1730,6 +1730,10 @@ int tcp_v4_rcv(struct sk_buff *skb)
reqsk_put(req);
goto discard_it;
}
+ if (unlikely(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;
Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection
Resubmitted. The various release/deref requirements in that path make a
straight "goto csum_error" impossible without duplicating some lines, but this
is 2nd best.
Frank
On 6/11/18, 4:43 PM, "van der Linden, Frank" wrote:
Yeah, true, it's missing INERRS in this case. I'll fix it up a bit.
Frank
On 6/11/18, 4:38 PM, "Eric Dumazet" wrote:
On 06/11/2018 04:25 PM, van der Linden, Frank wrote:
> A few comments on this one:
>
> - obviously this is fairly serious, as it can let corrupted data all
the way up to the application
Sure, although anyone relying on CRC checksum for ensuring TCP data
integrity
has big troubles ;)
I would rather have a refined version of this patch doing a "goto
csum_error"
so that we properly increment TCP_MIB_CSUMERRORS and TCP_MIB_INERRS
Thanks !
Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection
Yeah, true, it's missing INERRS in this case. I'll fix it up a bit. Frank On 6/11/18, 4:38 PM, "Eric Dumazet" wrote: On 06/11/2018 04:25 PM, van der Linden, Frank wrote: > A few comments on this one: > > - obviously this is fairly serious, as it can let corrupted data all the way up to the application Sure, although anyone relying on CRC checksum for ensuring TCP data integrity has big troubles ;) I would rather have a refined version of this patch doing a "goto csum_error" so that we properly increment TCP_MIB_CSUMERRORS and TCP_MIB_INERRS Thanks !
Re: [PATCH] tcp: verify the checksum of the first data segment in a new connection
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 1;
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 1 > "${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" 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
---
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, _stolen);
+
+ if (tcp_checksum_complete(skb)) {
+ __TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
+ } else {
+ nsk = tcp_check_req(sk, skb, req, false,
+ _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, _stolen);
+
+ if (tcp_checksum_complete(skb)) {
+ __TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
+ } else {
+ nsk = tcp_check_req(sk, skb, req, false,
+ _stolen);
+ }
}
if (!nsk) {
reqsk_put(req);
--
2.14.4
