netdev - Re: [PATCH] ipv4: set the tcp_min_rtt_wlen range from 0 to u32_max_div

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d46919ec-2e7f-1c3c-2acb-b41635f732f0@gmail.com>
Date:   Mon, 15 Apr 2019 10:53:14 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     ZhangXiaoxu <zhangxiaoxu5@...wei.com>, davem@...emloft.net,
        kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        netdev@...r.kernel.org
Cc:     Neal Cardwell <ncardwell@...gle.com>
Subject: Re: [PATCH] ipv4: set the tcp_min_rtt_wlen range from 0 to
 u32_max_div_HZ



On 04/15/2019 12:15 AM, ZhangXiaoxu wrote:
> There is a UBSAN report as below:
> UBSAN: Undefined behaviour in net/ipv4/tcp_input.c:2877:56
> signed integer overflow:
> 2147483647 * 1000 cannot be represented in type 'int'
> CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.1.0-rc4-00058-g582549e #1
> Call Trace:
>  <IRQ>
>  dump_stack+0x8c/0xba
>  ubsan_epilogue+0x11/0x60
>  handle_overflow+0x12d/0x170
>  ? ttwu_do_wakeup+0x21/0x320
>  __ubsan_handle_mul_overflow+0x12/0x20
>  tcp_ack_update_rtt+0x76c/0x780
>  tcp_clean_rtx_queue+0x499/0x14d0
>  tcp_ack+0x69e/0x1240
>  ? __wake_up_sync_key+0x2c/0x50
>  ? update_group_capacity+0x50/0x680
>  tcp_rcv_established+0x4e2/0xe10
>  tcp_v4_do_rcv+0x22b/0x420
>  tcp_v4_rcv+0xfe8/0x1190
>  ip_protocol_deliver_rcu+0x36/0x180
>  ip_local_deliver+0x15b/0x1a0
>  ip_rcv+0xac/0xd0
>  __netif_receive_skb_one_core+0x7f/0xb0
>  __netif_receive_skb+0x33/0xc0
>  netif_receive_skb_internal+0x84/0x1c0
>  napi_gro_receive+0x2a0/0x300
>  receive_buf+0x3d4/0x2350
>  ? detach_buf_split+0x159/0x390
>  virtnet_poll+0x198/0x840
>  ? reweight_entity+0x243/0x4b0
>  net_rx_action+0x25c/0x770
>  __do_softirq+0x19b/0x66d
>  irq_exit+0x1eb/0x230
>  do_IRQ+0x7a/0x150
>  common_interrupt+0xf/0xf
>  </IRQ>
> 
> It can be reproduced by:
>   echo 2147483647 > /proc/sys/net/ipv4/tcp_min_rtt_wlen
> 
> Fixes: f672258391b42 ("tcp: track min RTT using windowed min-filter")
> 
> Signed-off-by: ZhangXiaoxu <zhangxiaoxu5@...wei.com>
> ---
>  net/ipv4/sysctl_net_ipv4.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index ba0fc4b..56306bc 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -1151,7 +1151,9 @@ static struct ctl_table ipv4_net_table[] = {
>  		.data		= &init_net.ipv4.sysctl_tcp_min_rtt_wlen,
>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &u32_max_div_HZ
>  	},
>  	{
>  		.procname	= "tcp_autocorking",
> 

Thanks for the patch, but I believe we need more sensible limits,
otherwise we hide a real problem.

Please take a look at minmax_running_min() implementation.

If we allow @win to be close of 0xFFFFFFFF, then (val.t - m->s[2].t > win)
will never be true.

One day limit would be more than enough I think.