Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Christoph Paasch <christoph.paasch@gmail.com>
To: Eric Dumazet <edumazet@google.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	netdev <netdev@vger.kernel.org>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jonathan Looney <jtl@netflix.com>,
	Neal Cardwell <ncardwell@google.com>,
	Tyler Hicks <tyhicks@canonical.com>,
	Yuchung Cheng <ycheng@google.com>,
	Bruce Curtis <brucec@netflix.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Dustin Marquess <dmarquess@apple.com>
Subject: Re: [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits
Date: Mon, 17 Jun 2019 17:18:22 -0700
Message-ID: <CALMXkpYVRxgeqarp4gnmX7GqYh1sWOAt6UaRFqYBOaaNFfZ5sw@mail.gmail.com> (raw)
In-Reply-To: <20190617170354.37770-3-edumazet@google.com>

On Mon, Jun 17, 2019 at 10:05 AM Eric Dumazet <edumazet@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 <edumazet@google.com>
> Reported-by: Jonathan Looney <jtl@netflix.com>
> Acked-by: Neal Cardwell <ncardwell@google.com>
> Acked-by: Yuchung Cheng <ycheng@google.com>
> Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
> Cc: Bruce Curtis <brucec@netflix.com>
> Cc: Jonathan Lemon <jonathan.lemon@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

  parent reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-17 17:03 [PATCH net 0/4] tcp: make sack processing more robust Eric Dumazet
2019-06-17 17:03 ` [PATCH net 1/4] tcp: limit payload size of sacked skbs Eric Dumazet
2019-06-17 17:14   ` Jonathan Lemon
2019-06-17 17:03 ` [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits Eric Dumazet
2019-06-17 17:14   ` Jonathan Lemon
2019-06-18  0:18   ` Christoph Paasch [this message]
2019-06-18  2:28     ` Eric Dumazet
2019-06-18  3:19       ` Christoph Paasch
2019-06-18  3:44         ` Eric Dumazet
2019-06-18  3:53           ` Christoph Paasch
2019-06-18  4:08             ` Eric Dumazet
2019-07-10 18:23         ` Prout, Andrew - LLSC - MITLL
2019-07-10 18:28           ` Eric Dumazet
2019-07-10 18:53             ` Prout, Andrew - LLSC - MITLL
2019-07-10 19:26               ` Eric Dumazet
2019-07-11  7:28                 ` Christoph Paasch
2019-07-11  9:19                   ` Eric Dumazet
2019-07-11 18:26                     ` Michal Kubecek
2019-07-11 18:50                       ` Eric Dumazet
2019-07-11 10:18                   ` Eric Dumazet
2019-07-11 17:14                 ` Prout, Andrew - LLSC - MITLL
2019-07-11 18:28                   ` Eric Dumazet
2019-07-11 19:04                     ` Jonathan Lemon
2019-07-12  7:05                       ` Eric Dumazet
2019-07-16 15:13                   ` Prout, Andrew - LLSC - MITLL
2019-06-17 17:03 ` [PATCH net 3/4] tcp: add tcp_min_snd_mss sysctl Eric Dumazet
2019-06-17 17:15   ` Jonathan Lemon
2019-06-17 17:18   ` Tyler Hicks
2019-06-17 17:03 ` [PATCH net 4/4] tcp: enforce tcp_min_snd_mss in tcp_mtu_probing() Eric Dumazet
2019-06-17 17:16   ` Jonathan Lemon
2019-06-17 17:18   ` Tyler Hicks
2019-06-17 17:41 ` [PATCH net 0/4] tcp: make sack processing more robust David Miller
2019-08-02 19:02 [PATCH net 2/4] tcp: tcp_fragment() should apply sane memory limits Bernd
2019-08-02 19:14 ` Neal Cardwell
2019-08-02 19:58   ` Bernd
2019-08-14 14:41     ` Marcelo Ricardo Leitner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALMXkpYVRxgeqarp4gnmX7GqYh1sWOAt6UaRFqYBOaaNFfZ5sw@mail.gmail.com \
    --to=christoph.paasch@gmail.com \
    --cc=brucec@netflix.com \
    --cc=davem@davemloft.net \
    --cc=dmarquess@apple.com \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jonathan.lemon@gmail.com \
    --cc=jtl@netflix.com \
    --cc=ncardwell@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=tyhicks@canonical.com \
    --cc=ycheng@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git