On Mon, Jun 17, 2019 at 10:05 AM Eric Dumazet <eduma...@google.com> wrote:
>
> Jonathan Looney reported that a malicious peer can force a sender
> to fragment its retransmit queue into tiny skbs, inflating memory
> usage and/or overflow 32bit counters.
>
> TCP allows an application to queue up to sk_sndbuf bytes,
> so we need to give some allowance for non malicious splitting
> of retransmit queue.
>
> A new SNMP counter is added to monitor how many times TCP
> did not allow to split an skb if the allowance was exceeded.
>
> Note that this counter might increase in the case applications
> use SO_SNDBUF socket option to lower sk_sndbuf.
>
> CVE-2019-11478 : tcp_fragment, prevent fragmenting a packet when the
> socket is already using more than half the allowed space
>
> Signed-off-by: Eric Dumazet <eduma...@google.com>
> Reported-by: Jonathan Looney <j...@netflix.com>
> Acked-by: Neal Cardwell <ncardw...@google.com>
> Acked-by: Yuchung Cheng <ych...@google.com>
> Reviewed-by: Tyler Hicks <tyhi...@canonical.com>
> Cc: Bruce Curtis <bru...@netflix.com>
> Cc: Jonathan Lemon <jonathan.le...@gmail.com>
> ---
> include/uapi/linux/snmp.h | 1 +
> net/ipv4/proc.c | 1 +
> net/ipv4/tcp_output.c | 5 +++++
> 3 files changed, 7 insertions(+)
>
> diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
> index
> 86dc24a96c90ab047d5173d625450facd6c6dd79..fd42c1316d3d112ecd8a00d2b499d6f6901c5e81
> 100644
> --- a/include/uapi/linux/snmp.h
> +++ b/include/uapi/linux/snmp.h
> @@ -283,6 +283,7 @@ enum
> LINUX_MIB_TCPACKCOMPRESSED, /* TCPAckCompressed */
> LINUX_MIB_TCPZEROWINDOWDROP, /* TCPZeroWindowDrop */
> LINUX_MIB_TCPRCVQDROP, /* TCPRcvQDrop */
> + LINUX_MIB_TCPWQUEUETOOBIG, /* TCPWqueueTooBig */
> __LINUX_MIB_MAX
> };
>
> diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
> index
> 4370f4246e86dfe06a9e07cace848baeaf6cc4da..073273b751f8fcda1c9c79cd1ab566f2939b2517
> 100644
> --- a/net/ipv4/proc.c
> +++ b/net/ipv4/proc.c
> @@ -287,6 +287,7 @@ static const struct snmp_mib snmp4_net_list[] = {
> SNMP_MIB_ITEM("TCPAckCompressed", LINUX_MIB_TCPACKCOMPRESSED),
> SNMP_MIB_ITEM("TCPZeroWindowDrop", LINUX_MIB_TCPZEROWINDOWDROP),
> SNMP_MIB_ITEM("TCPRcvQDrop", LINUX_MIB_TCPRCVQDROP),
> + SNMP_MIB_ITEM("TCPWqueueTooBig", LINUX_MIB_TCPWQUEUETOOBIG),
> SNMP_MIB_SENTINEL
> };
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index
> b8e3bbb852117459d131fbb41d69ae63bd251a3e..1bb1c46b4abad100622d3f101a0a3ca0a6c8e881
> 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1296,6 +1296,11 @@ int tcp_fragment(struct sock *sk, enum tcp_queue
> tcp_queue,
> if (nsize < 0)
> nsize = 0;
>
> + if (unlikely((sk->sk_wmem_queued >> 1) > sk->sk_sndbuf)) {
> + NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
> + return -ENOMEM;
> + }
> +
Hi Eric, I now have a packetdrill test that started failing (see
below). Admittedly, a bit weird test with the SO_SNDBUF forced so low.
Nevertheless, previously this test would pass, now it stalls after the
write() because tcp_fragment() returns -ENOMEM. Your commit-message
mentions that this could trigger when one sets SO_SNDBUF low. But,
here we have a complete stall of the connection and we never recover.
I don't know if we care about this, but there it is :-)
Christoph
----
--tolerance_usecs=10000
+0.0 `ifconfig tun0 mtu 5060`
+0.0 `ethtool -K tun0 tso off` // don't send big segments > MSS
+0.015 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.0 bind(3, ..., ...) = 0
+0.0 setsockopt(3, SOL_SOCKET, SO_SNDBUF, [2000], 4) = 0
+0.0 listen(3, 1) = 0
+0 < S 0:0(0) win 24900 <mss 5020,sackOK,nop,nop,nop,wscale 7>
+0 > S. 0:0(0) ack 1 <mss 5020,sackOK,nop,nop,nop,wscale 7>
+0 < . 1:1(0) ack 1 win 257
+0 accept(3, ..., ...) = 4
+0 setsockopt(4, SOL_TCP, TCP_CORK, [1], 4) = 0
+0 write(4, ..., 9999) = 9999
+0.0 > . 1:5021(5020) ack 1
+0.01 < . 1:1(0) ack 5021 win 257
+0.206 > P. 5021:10000(4979) ack 1
+0.01 < . 1:1(0) ack 10000 win 257
