All of lore.kernel.org
 help / color / mirror / Atom feed
* KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
@ 2017-04-02  7:43 Denys Fedoryshchenko
  2017-04-02 11:24 ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-02  7:43 UTC (permalink / raw)
  To: Linux Kernel Network Developers
  Cc: Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel

Repost, due being sleepy missed few important points.

I am searching reasons of crashes for multiple conntrack enabled 
servers, usually they point to conntrack, but i suspect use after free 
might be somewhere else,
so i tried to enable KASAN.
And seems i got something after few hours, and it looks related to all 
crashes, because on all that servers who rebooted i had MSS adjustment 
(--clamp-mss-to-pmtu or --set-mss).
Please let me know if any additional information needed.

[25181.855611] 
==================================================================
[25181.855985] BUG: KASAN: use-after-free in tcpmss_tg4+0x682/0xe9c 
[xt_TCPMSS] at addr ffff8802976000ea
[25181.856344] Read of size 1 by task swapper/1/0
[25181.856555] page:ffffea000a5d8000 count:0 mapcount:0 mapping:         
  (null) index:0x0
[25181.856909] flags: 0x1000000000000000()
[25181.857123] raw: 1000000000000000 0000000000000000 0000000000000000 
00000000ffffffff
[25181.857630] raw: ffffea000b0444a0 ffffea000a0b1f60 0000000000000000 
0000000000000000
[25181.857996] page dumped because: kasan: bad access detected
[25181.858214] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 
4.10.8-build-0133-debug #3
[25181.858571] Hardware name: HP ProLiant DL320e Gen8 v2, BIOS P80 
04/02/2015
[25181.858786] Call Trace:
[25181.859000]  <IRQ>
[25181.859215]  dump_stack+0x99/0xd4
[25181.859423]  ? _atomic_dec_and_lock+0x15d/0x15d
[25181.859644]  ? __dump_page+0x447/0x4e3
[25181.859859]  ? tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
[25181.860080]  kasan_report+0x577/0x69d
[25181.860291]  ? __ip_route_output_key_hash+0x14ce/0x1503
[25181.860512]  ? tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
[25181.860736]  __asan_report_load1_noabort+0x19/0x1b
[25181.860956]  tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
[25181.861180]  ? tcpmss_tg4_check+0x287/0x287 [xt_TCPMSS]
[25181.861407]  ? udp_mt+0x45a/0x45a [xt_tcpudp]
[25181.861634]  ? __fib_validate_source+0x46b/0xcd1
[25181.861860]  ipt_do_table+0x1432/0x1573 [ip_tables]
[25181.862088]  ? igb_msix_ring+0x2d/0x35
[25181.862318]  ? ip_tables_net_init+0x15/0x15 [ip_tables]
[25181.862537]  ? ip_route_input_slow+0xe9f/0x17e3
[25181.862759]  ? handle_irq_event_percpu+0x141/0x141
[25181.862985]  ? rt_set_nexthop+0x9a7/0x9a7
[25181.863203]  ? ip_tables_net_exit+0xe/0x15 [ip_tables]
[25181.863419]  ? tcf_action_exec+0xce/0x18c
[25181.863628]  ? iptable_mangle_net_exit+0x92/0x92 [iptable_mangle]
[25181.863856]  ? iptable_filter_net_exit+0x92/0x92 [iptable_filter]
[25181.864084]  iptable_filter_hook+0xc0/0x1c8 [iptable_filter]
[25181.864311]  nf_hook_slow+0x7d/0x121
[25181.864536]  ip_forward+0x1183/0x11c6
[25181.864752]  ? ip_forward_finish+0x168/0x168
[25181.864967]  ? ip_frag_mem+0x43/0x43
[25181.865194]  ? iptable_nat_net_exit+0x92/0x92 [iptable_nat]
[25181.865423]  ? nf_nat_ipv4_in+0xf0/0x209 [nf_nat_ipv4]
[25181.865648]  ip_rcv_finish+0xf4c/0xf5b
[25181.865861]  ip_rcv+0xb41/0xb72
[25181.866086]  ? ip_local_deliver+0x282/0x282
[25181.866308]  ? ip_local_deliver_finish+0x6e6/0x6e6
[25181.866524]  ? ip_local_deliver+0x282/0x282
[25181.866752]  __netif_receive_skb_core+0x1b27/0x21bf
[25181.866971]  ? netdev_rx_handler_register+0x1a6/0x1a6
[25181.867186]  ? enqueue_hrtimer+0x232/0x240
[25181.867401]  ? hrtimer_start_range_ns+0xd1c/0xd4b
[25181.867630]  ? __ppp_xmit_process+0x101f/0x104e [ppp_generic]
[25181.867852]  ? hrtimer_cancel+0x20/0x20
[25181.868081]  ? ppp_push+0x1402/0x1402 [ppp_generic]
[25181.868301]  ? __pskb_pull_tail+0xb0f/0xb25
[25181.868523]  ? ppp_xmit_process+0x47/0xaf [ppp_generic]
[25181.868749]  __netif_receive_skb+0x5e/0x191
[25181.868968]  process_backlog+0x295/0x573
[25181.869180]  ? __netif_receive_skb+0x191/0x191
[25181.869401]  napi_poll+0x311/0x745
[25181.869611]  ? napi_complete_done+0x3b4/0x3b4
[25181.869836]  ? __qdisc_run+0x4ec/0xb7f
[25181.870061]  ? sch_direct_xmit+0x60b/0x60b
[25181.870286]  net_rx_action+0x2e8/0x6dc
[25181.870512]  ? napi_poll+0x745/0x745
[25181.870732]  ? rps_trigger_softirq+0x181/0x1e4
[25181.870956]  ? rps_may_expire_flow+0x29b/0x29b
[25181.871184]  ? irq_work_run+0x2c/0x2e
[25181.871411]  __do_softirq+0x22b/0x5df
[25181.871629]  ? smp_call_function_single_async+0x17d/0x17d
[25181.871854]  irq_exit+0x8a/0xfe
[25181.872069]  smp_call_function_single_interrupt+0x8d/0x90
[25181.872297]  call_function_single_interrupt+0x83/0x90
[25181.872519] RIP: 0010:mwait_idle+0x15a/0x30d
[25181.872733] RSP: 0018:ffff8802d1017e78 EFLAGS: 00000246 ORIG_RAX: 
ffffffffffffff04
[25181.873091] RAX: 0000000000000000 RBX: ffff8802d1000c80 RCX: 
0000000000000000
[25181.873311] RDX: 1ffff1005a200190 RSI: 0000000000000000 RDI: 
0000000000000000
[25181.873532] RBP: ffff8802d1017e98 R08: 000000000000003f R09: 
00007f75f7fff700
[25181.873751] R10: ffff8802d1017d80 R11: ffff8802c9b00000 R12: 
0000000000000001
[25181.873971] R13: 0000000000000000 R14: ffff8802d1000c80 R15: 
dffffc0000000000
[25181.874182]  </IRQ>
[25181.874393]  arch_cpu_idle+0xf/0x11
[25181.874602]  default_idle_call+0x59/0x5c
[25181.874818]  do_idle+0x11c/0x217
[25181.875039]  cpu_startup_entry+0x1f/0x21
[25181.875258]  start_secondary+0x2cc/0x2d5
[25181.875481]  start_cpu+0x14/0x14
[25181.875696] Memory state around the buggy address:
[25181.875919]  ffff8802975fff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[25181.876275]  ffff880297600000: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[25181.876628] >ffff880297600080: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[25181.876984]                                                           
^
[25181.877203]  ffff880297600100: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[25181.877569]  ffff880297600180: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[25181.877930] 
==================================================================
[25181.878283] Disabling lock debugging due to kernel taint
[25181.878584] 
==================================================================

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02  7:43 KASAN, xt_TCPMSS finally found nasty use-after-free bug? 4.10.8 Denys Fedoryshchenko
@ 2017-04-02 11:24 ` Eric Dumazet
  2017-04-02 11:45   ` Florian Westphal
  2017-04-03 17:55   ` [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff Eric Dumazet
  0 siblings, 2 replies; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 11:24 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Linux Kernel Network Developers, Pablo Neira Ayuso,
	Patrick McHardy, Jozsef Kadlecsik, netfilter-devel, coreteam,
	linux-kernel

On Sun, 2017-04-02 at 10:43 +0300, Denys Fedoryshchenko wrote:
> Repost, due being sleepy missed few important points.
> 
> I am searching reasons of crashes for multiple conntrack enabled 
> servers, usually they point to conntrack, but i suspect use after free 
> might be somewhere else,
> so i tried to enable KASAN.
> And seems i got something after few hours, and it looks related to all 
> crashes, because on all that servers who rebooted i had MSS adjustment 
> (--clamp-mss-to-pmtu or --set-mss).
> Please let me know if any additional information needed.
> 
> [25181.855611] 
> ==================================================================
> [25181.855985] BUG: KASAN: use-after-free in tcpmss_tg4+0x682/0xe9c 
> [xt_TCPMSS] at addr ffff8802976000ea
> [25181.856344] Read of size 1 by task swapper/1/0
> [25181.856555] page:ffffea000a5d8000 count:0 mapcount:0 mapping:         
>   (null) index:0x0
> [25181.856909] flags: 0x1000000000000000()
> [25181.857123] raw: 1000000000000000 0000000000000000 0000000000000000 
> 00000000ffffffff
> [25181.857630] raw: ffffea000b0444a0 ffffea000a0b1f60 0000000000000000 
> 0000000000000000
> [25181.857996] page dumped because: kasan: bad access detected
> [25181.858214] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 
> 4.10.8-build-0133-debug #3
> [25181.858571] Hardware name: HP ProLiant DL320e Gen8 v2, BIOS P80 
> 04/02/2015
> [25181.858786] Call Trace:
> [25181.859000]  <IRQ>
> [25181.859215]  dump_stack+0x99/0xd4
> [25181.859423]  ? _atomic_dec_and_lock+0x15d/0x15d
> [25181.859644]  ? __dump_page+0x447/0x4e3
> [25181.859859]  ? tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
> [25181.860080]  kasan_report+0x577/0x69d
> [25181.860291]  ? __ip_route_output_key_hash+0x14ce/0x1503
> [25181.860512]  ? tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
> [25181.860736]  __asan_report_load1_noabort+0x19/0x1b
> [25181.860956]  tcpmss_tg4+0x682/0xe9c [xt_TCPMSS]
> [25181.861180]  ? tcpmss_tg4_check+0x287/0x287 [xt_TCPMSS]
> [25181.861407]  ? udp_mt+0x45a/0x45a [xt_tcpudp]
> [25181.861634]  ? __fib_validate_source+0x46b/0xcd1
> [25181.861860]  ipt_do_table+0x1432/0x1573 [ip_tables]
> [25181.862088]  ? igb_msix_ring+0x2d/0x35
> [25181.862318]  ? ip_tables_net_init+0x15/0x15 [ip_tables]
> [25181.862537]  ? ip_route_input_slow+0xe9f/0x17e3
> [25181.862759]  ? handle_irq_event_percpu+0x141/0x141
> [25181.862985]  ? rt_set_nexthop+0x9a7/0x9a7
> [25181.863203]  ? ip_tables_net_exit+0xe/0x15 [ip_tables]
> [25181.863419]  ? tcf_action_exec+0xce/0x18c
> [25181.863628]  ? iptable_mangle_net_exit+0x92/0x92 [iptable_mangle]
> [25181.863856]  ? iptable_filter_net_exit+0x92/0x92 [iptable_filter]
> [25181.864084]  iptable_filter_hook+0xc0/0x1c8 [iptable_filter]
> [25181.864311]  nf_hook_slow+0x7d/0x121
> [25181.864536]  ip_forward+0x1183/0x11c6
> [25181.864752]  ? ip_forward_finish+0x168/0x168
> [25181.864967]  ? ip_frag_mem+0x43/0x43
> [25181.865194]  ? iptable_nat_net_exit+0x92/0x92 [iptable_nat]
> [25181.865423]  ? nf_nat_ipv4_in+0xf0/0x209 [nf_nat_ipv4]
> [25181.865648]  ip_rcv_finish+0xf4c/0xf5b
> [25181.865861]  ip_rcv+0xb41/0xb72
> [25181.866086]  ? ip_local_deliver+0x282/0x282
> [25181.866308]  ? ip_local_deliver_finish+0x6e6/0x6e6
> [25181.866524]  ? ip_local_deliver+0x282/0x282
> [25181.866752]  __netif_receive_skb_core+0x1b27/0x21bf
> [25181.866971]  ? netdev_rx_handler_register+0x1a6/0x1a6
> [25181.867186]  ? enqueue_hrtimer+0x232/0x240
> [25181.867401]  ? hrtimer_start_range_ns+0xd1c/0xd4b
> [25181.867630]  ? __ppp_xmit_process+0x101f/0x104e [ppp_generic]
> [25181.867852]  ? hrtimer_cancel+0x20/0x20
> [25181.868081]  ? ppp_push+0x1402/0x1402 [ppp_generic]
> [25181.868301]  ? __pskb_pull_tail+0xb0f/0xb25
> [25181.868523]  ? ppp_xmit_process+0x47/0xaf [ppp_generic]
> [25181.868749]  __netif_receive_skb+0x5e/0x191
> [25181.868968]  process_backlog+0x295/0x573
> [25181.869180]  ? __netif_receive_skb+0x191/0x191
> [25181.869401]  napi_poll+0x311/0x745
> [25181.869611]  ? napi_complete_done+0x3b4/0x3b4
> [25181.869836]  ? __qdisc_run+0x4ec/0xb7f
> [25181.870061]  ? sch_direct_xmit+0x60b/0x60b
> [25181.870286]  net_rx_action+0x2e8/0x6dc
> [25181.870512]  ? napi_poll+0x745/0x745
> [25181.870732]  ? rps_trigger_softirq+0x181/0x1e4
> [25181.870956]  ? rps_may_expire_flow+0x29b/0x29b
> [25181.871184]  ? irq_work_run+0x2c/0x2e
> [25181.871411]  __do_softirq+0x22b/0x5df
> [25181.871629]  ? smp_call_function_single_async+0x17d/0x17d
> [25181.871854]  irq_exit+0x8a/0xfe
> [25181.872069]  smp_call_function_single_interrupt+0x8d/0x90
> [25181.872297]  call_function_single_interrupt+0x83/0x90
> [25181.872519] RIP: 0010:mwait_idle+0x15a/0x30d
> [25181.872733] RSP: 0018:ffff8802d1017e78 EFLAGS: 00000246 ORIG_RAX: 
> ffffffffffffff04
> [25181.873091] RAX: 0000000000000000 RBX: ffff8802d1000c80 RCX: 
> 0000000000000000
> [25181.873311] RDX: 1ffff1005a200190 RSI: 0000000000000000 RDI: 
> 0000000000000000
> [25181.873532] RBP: ffff8802d1017e98 R08: 000000000000003f R09: 
> 00007f75f7fff700
> [25181.873751] R10: ffff8802d1017d80 R11: ffff8802c9b00000 R12: 
> 0000000000000001
> [25181.873971] R13: 0000000000000000 R14: ffff8802d1000c80 R15: 
> dffffc0000000000
> [25181.874182]  </IRQ>
> [25181.874393]  arch_cpu_idle+0xf/0x11
> [25181.874602]  default_idle_call+0x59/0x5c
> [25181.874818]  do_idle+0x11c/0x217
> [25181.875039]  cpu_startup_entry+0x1f/0x21
> [25181.875258]  start_secondary+0x2cc/0x2d5
> [25181.875481]  start_cpu+0x14/0x14
> [25181.875696] Memory state around the buggy address:
> [25181.875919]  ffff8802975fff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00
> [25181.876275]  ffff880297600000: 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00
> [25181.876628] >ffff880297600080: 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00
> [25181.876984]                                                           
> ^
> [25181.877203]  ffff880297600100: 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00
> [25181.877569]  ffff880297600180: 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 00 00 00
> [25181.877930] 
> ==================================================================
> [25181.878283] Disabling lock debugging due to kernel taint
> [25181.878584] 
> ==================================================================

Hi Denys

This definitely looks bad.

Could you try :

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 27241a767f17b4b27d24095a31e5e9a2d3e29ce4..81731866c921932318555414b497e37b0649114a 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -122,7 +122,7 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 		newmss = info->mss;
 
 	opt = (u_int8_t *)tcph;
-	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
+	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
 		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
 			u_int16_t oldmss;
 

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 11:24 ` Eric Dumazet
@ 2017-04-02 11:45   ` Florian Westphal
  2017-04-02 11:51     ` Denys Fedoryshchenko
  2017-04-02 11:54     ` Eric Dumazet
  2017-04-03 17:55   ` [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff Eric Dumazet
  1 sibling, 2 replies; 18+ messages in thread
From: Florian Westphal @ 2017-04-02 11:45 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Denys Fedoryshchenko, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> -	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
>  		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
>  			u_int16_t oldmss;

maybe I am low on caffeeine but this looks fine, for tcp header with
only tcpmss this boils down to "20 <= 24 - 4" so we acccess offsets 20-23 which seems ok.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 11:45   ` Florian Westphal
@ 2017-04-02 11:51     ` Denys Fedoryshchenko
  2017-04-02 11:54     ` Eric Dumazet
  1 sibling, 0 replies; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-02 11:51 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Eric Dumazet, Linux Kernel Network Developers, Pablo Neira Ayuso,
	Patrick McHardy, Jozsef Kadlecsik, netfilter-devel, coreteam,
	linux-kernel

On 2017-04-02 14:45, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> -	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += 
>> optlen(opt, i)) {
>> +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += 
>> optlen(opt, i)) {
>>  		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
>>  			u_int16_t oldmss;
> 
> maybe I am low on caffeeine but this looks fine, for tcp header with
> only tcpmss this boils down to "20 <= 24 - 4" so we acccess offsets
> 20-23 which seems ok.
It seems some non-standard(or corrupted) packets are passing, because 
even on ~1G server it might cause corruption once per several days, 
KASAN seems need less time to trigger.

I am not aware how things working, but:
[25181.875696] Memory state around the buggy address:
[25181.875919]  ffff8802975fff80: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[25181.876275]  ffff880297600000: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[25181.876628] >ffff880297600080: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[25181.876984]
^
[25181.877203]  ffff880297600100: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[25181.877569]  ffff880297600180: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00

Why all data here is zero? I guess it should be some packet data?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 11:45   ` Florian Westphal
  2017-04-02 11:51     ` Denys Fedoryshchenko
@ 2017-04-02 11:54     ` Eric Dumazet
  2017-04-02 12:19       ` Eric Dumazet
  1 sibling, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 11:54 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Denys Fedoryshchenko, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel

On Sun, 2017-04-02 at 13:45 +0200, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > -	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> > +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> >  		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
> >  			u_int16_t oldmss;
> 
> maybe I am low on caffeeine but this looks fine, for tcp header with
> only tcpmss this boils down to "20 <= 24 - 4" so we acccess offsets 20-23 which seems ok.

I am definitely low on caffeine ;)

An issue in this function is that we might add the missing MSS option,
without checking that TCP options are already full.

But this should not cause a KASAN splat, only some malformed TCP packet

(tcph->doff would wrap)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 11:54     ` Eric Dumazet
@ 2017-04-02 12:19       ` Eric Dumazet
  2017-04-02 12:25         ` Denys Fedoryshchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 12:19 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Denys Fedoryshchenko, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel

On Sun, 2017-04-02 at 04:54 -0700, Eric Dumazet wrote:
> On Sun, 2017-04-02 at 13:45 +0200, Florian Westphal wrote:
> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > -	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> > > +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
> > >  		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
> > >  			u_int16_t oldmss;
> > 
> > maybe I am low on caffeeine but this looks fine, for tcp header with
> > only tcpmss this boils down to "20 <= 24 - 4" so we acccess offsets 20-23 which seems ok.
> 
> I am definitely low on caffeine ;)
> 
> An issue in this function is that we might add the missing MSS option,
> without checking that TCP options are already full.
> 
> But this should not cause a KASAN splat, only some malformed TCP packet
> 
> (tcph->doff would wrap)

Something like that maybe.

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index
27241a767f17b4b27d24095a31e5e9a2d3e29ce4..1465aaf0e3a15d69d105d0a50b0429b11b6439d3 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -151,7 +151,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	 */
 	if (len > tcp_hdrlen)
 		return 0;
-
+	/* tcph->doff is 4 bits wide, do not wrap its value to 0 */
+	if (tcp_hdrlen >= 15 * 4)
+		return 0;
 	/*
 	 * MSS Option not found ?! add it..
 	 */

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 12:19       ` Eric Dumazet
@ 2017-04-02 12:25         ` Denys Fedoryshchenko
  2017-04-02 12:32           ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-02 12:25 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On 2017-04-02 15:19, Eric Dumazet wrote:
> On Sun, 2017-04-02 at 04:54 -0700, Eric Dumazet wrote:
>> On Sun, 2017-04-02 at 13:45 +0200, Florian Westphal wrote:
>> > Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > > -	for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
>> > > +	for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += optlen(opt, i)) {
>> > >  		if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
>> > >  			u_int16_t oldmss;
>> >
>> > maybe I am low on caffeeine but this looks fine, for tcp header with
>> > only tcpmss this boils down to "20 <= 24 - 4" so we acccess offsets 20-23 which seems ok.
>> 
>> I am definitely low on caffeine ;)
>> 
>> An issue in this function is that we might add the missing MSS option,
>> without checking that TCP options are already full.
>> 
>> But this should not cause a KASAN splat, only some malformed TCP 
>> packet
>> 
>> (tcph->doff would wrap)
> 
> Something like that maybe.
> 
> diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
> index
> 27241a767f17b4b27d24095a31e5e9a2d3e29ce4..1465aaf0e3a15d69d105d0a50b0429b11b6439d3
> 100644
> --- a/net/netfilter/xt_TCPMSS.c
> +++ b/net/netfilter/xt_TCPMSS.c
> @@ -151,7 +151,9 @@ tcpmss_mangle_packet(struct sk_buff *skb,
>  	 */
>  	if (len > tcp_hdrlen)
>  		return 0;
> -
> +	/* tcph->doff is 4 bits wide, do not wrap its value to 0 */
> +	if (tcp_hdrlen >= 15 * 4)
> +		return 0;
>  	/*
>  	 * MSS Option not found ?! add it..
>  	 */
I will add also WARN_ON_ONCE(tcp_hdrlen >= 15 * 4) before, for 
curiosity, if this condition are triggered. Is it fine like that?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 12:25         ` Denys Fedoryshchenko
@ 2017-04-02 12:32           ` Eric Dumazet
  2017-04-02 16:52             ` Denys Fedoryshchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 12:32 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On Sun, 2017-04-02 at 15:25 +0300, Denys Fedoryshchenko wrote:
> >  	 */
> I will add also WARN_ON_ONCE(tcp_hdrlen >= 15 * 4) before, for 
> curiosity, if this condition are triggered. Is it fine like that?

Sure.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 12:32           ` Eric Dumazet
@ 2017-04-02 16:52             ` Denys Fedoryshchenko
  2017-04-02 17:14               ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-02 16:52 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On 2017-04-02 15:32, Eric Dumazet wrote:
> On Sun, 2017-04-02 at 15:25 +0300, Denys Fedoryshchenko wrote:
>> >  	 */
>> I will add also WARN_ON_ONCE(tcp_hdrlen >= 15 * 4) before, for
>> curiosity, if this condition are triggered. Is it fine like that?
> 
> Sure.

It didnt triggered WARN_ON, and with both patches here is one more 
KASAN.
What i noticed also after this KASAN, there is many others start to 
trigger in TCPMSS and locking up server by flood.
There is heavy netlink activity, it is pppoe server with lot of shapers.
I noticed there left sfq by mistake, usually i am removing it, because 
it may trigger kernel panic too (and hard to trace reason).
I will try with pfifo instead, after 6 hours.

Here is full log with others: https://nuclearcat.com/kasan.txt


[ 2033.914478] 
==================================================================
[ 2033.914855] BUG: KASAN: slab-out-of-bounds in tcpmss_tg4+0x6cc/0xee4 
[xt_TCPMSS] at addr ffff8802bfe18140
[ 2033.915218] Read of size 1 by task swapper/1/0
[ 2033.915437] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 
4.10.8-build-0136-debug #7
[ 2033.915787] Hardware name: HP ProLiant DL320e Gen8 v2, BIOS P80 
04/02/2015
[ 2033.916010] Call Trace:
[ 2033.916229]  <IRQ>
[ 2033.916449]  dump_stack+0x99/0xd4
[ 2033.916662]  ? _atomic_dec_and_lock+0x15d/0x15d
[ 2033.916886]  ? tcpmss_tg4+0x6cc/0xee4 [xt_TCPMSS]
[ 2033.917110]  kasan_object_err+0x21/0x81
[ 2033.917335]  kasan_report+0x527/0x69d
[ 2033.917557]  ? tcpmss_tg4+0x6cc/0xee4 [xt_TCPMSS]
[ 2033.917772]  __asan_report_load1_noabort+0x19/0x1b
[ 2033.917995]  tcpmss_tg4+0x6cc/0xee4 [xt_TCPMSS]
[ 2033.918222]  ? tcpmss_tg4_check+0x287/0x287 [xt_TCPMSS]
[ 2033.918451]  ? udp_mt+0x45a/0x45a [xt_tcpudp]
[ 2033.918669]  ? __fib_validate_source+0x46b/0xcd1
[ 2033.918895]  ipt_do_table+0x1432/0x1573 [ip_tables]
[ 2033.919114]  ? ip_tables_net_init+0x15/0x15 [ip_tables]
[ 2033.919338]  ? ip_route_input_slow+0xe9f/0x17e3
[ 2033.919562]  ? rt_set_nexthop+0x9a7/0x9a7
[ 2033.919790]  ? ip_tables_net_exit+0xe/0x15 [ip_tables]
[ 2033.920008]  ? tcf_action_exec+0x14a/0x18c
[ 2033.920227]  ? iptable_mangle_net_exit+0x92/0x92 [iptable_mangle]
[ 2033.920451]  ? iptable_filter_net_exit+0x92/0x92 [iptable_filter]
[ 2033.920667]  iptable_filter_hook+0xc0/0x1c8 [iptable_filter]
[ 2033.920882]  nf_hook_slow+0x7d/0x121
[ 2033.921105]  ip_forward+0x1183/0x11c6
[ 2033.921321]  ? ip_forward_finish+0x168/0x168
[ 2033.921542]  ? ip_frag_mem+0x43/0x43
[ 2033.921755]  ? iptable_nat_net_exit+0x92/0x92 [iptable_nat]
[ 2033.921981]  ? nf_nat_ipv4_in+0xf0/0x209 [nf_nat_ipv4]
[ 2033.922199]  ip_rcv_finish+0xf4c/0xf5b
[ 2033.922420]  ip_rcv+0xb41/0xb72
[ 2033.922635]  ? ip_local_deliver+0x282/0x282
[ 2033.922847]  ? ip_local_deliver_finish+0x6e6/0x6e6
[ 2033.923073]  ? ip_local_deliver+0x282/0x282
[ 2033.923291]  __netif_receive_skb_core+0x1b27/0x21bf
[ 2033.923510]  ? netdev_rx_handler_register+0x1a6/0x1a6
[ 2033.923736]  ? kasan_slab_free+0x137/0x154
[ 2033.923954]  ? save_stack_trace+0x1b/0x1d
[ 2033.924170]  ? kasan_slab_free+0xaa/0x154
[ 2033.924387]  ? net_rx_action+0x6ad/0x6dc
[ 2033.924611]  ? __do_softirq+0x22b/0x5df
[ 2033.924826]  ? irq_exit+0x8a/0xfe
[ 2033.925048]  ? do_IRQ+0x13d/0x155
[ 2033.925269]  ? common_interrupt+0x83/0x83
[ 2033.925483]  ? mwait_idle+0x15a/0x30d
[ 2033.925704]  ? napi_gro_flush+0x1d0/0x1d0
[ 2033.925928]  ? start_secondary+0x2cc/0x2d5
[ 2033.926142]  ? start_cpu+0x14/0x14
[ 2033.926354]  __netif_receive_skb+0x5e/0x191
[ 2033.926576]  process_backlog+0x295/0x573
[ 2033.926799]  ? __netif_receive_skb+0x191/0x191
[ 2033.927022]  napi_poll+0x311/0x745
[ 2033.927245]  ? napi_complete_done+0x3b4/0x3b4
[ 2033.927460]  ? igb_msix_ring+0x2d/0x35
[ 2033.927679]  net_rx_action+0x2e8/0x6dc
[ 2033.927903]  ? napi_poll+0x745/0x745
[ 2033.928133]  ? sched_clock_cpu+0x1f/0x18c
[ 2033.928360]  ? rps_trigger_softirq+0x181/0x1e4
[ 2033.928592]  ? __tick_nohz_idle_enter+0x465/0xa6d
[ 2033.928817]  ? rps_may_expire_flow+0x29b/0x29b
[ 2033.929038]  ? irq_work_run+0x2c/0x2e
[ 2033.929253]  __do_softirq+0x22b/0x5df
[ 2033.929464]  ? smp_call_function_single_async+0x17d/0x17d
[ 2033.929680]  irq_exit+0x8a/0xfe
[ 2033.929905]  smp_call_function_single_interrupt+0x8d/0x90
[ 2033.930136]  call_function_single_interrupt+0x83/0x90
[ 2033.930365] RIP: 0010:mwait_idle+0x15a/0x30d
[ 2033.930581] RSP: 0018:ffff8802d1017e78 EFLAGS: 00000246 ORIG_RAX: 
ffffffffffffff04
[ 2033.930934] RAX: 0000000000000000 RBX: ffff8802d1000c80 RCX: 
0000000000000000
[ 2033.931160] RDX: 1ffff1005a200190 RSI: 0000000000000000 RDI: 
0000000000000000
[ 2033.931383] RBP: ffff8802d1017e98 R08: ffffed00583c4fc1 R09: 
0000000000000080
[ 2033.931596] R10: ffff8802d1017d80 R11: ffffed00583c4fc1 R12: 
0000000000000001
[ 2033.931808] R13: 0000000000000000 R14: ffff8802d1000c80 R15: 
dffffc0000000000
[ 2033.932031]  </IRQ>
[ 2033.932247]  arch_cpu_idle+0xf/0x11
[ 2033.932472]  default_idle_call+0x59/0x5c
[ 2033.932686]  do_idle+0x11c/0x217
[ 2033.932906]  cpu_startup_entry+0x1f/0x21
[ 2033.933128]  start_secondary+0x2cc/0x2d5
[ 2033.933351]  start_cpu+0x14/0x14
[ 2033.933574] Object at ffff8802bfe18000, in cache kmalloc-512 size: 
512
[ 2033.933792] Allocated:
[ 2033.934004] PID = 3885
[ 2033.934213]  save_stack_trace+0x1b/0x1d
[ 2033.934424]  kasan_kmalloc.part.1+0x65/0xf1
[ 2033.934648]  kasan_kmalloc+0x81/0x8d
[ 2033.934868]  __kmalloc_node+0x18d/0x34a
[ 2033.935090]  qdisc_alloc+0x126/0x51d
[ 2033.935306]  qdisc_create+0x1a0/0xb1e
[ 2033.935531]  tc_modify_qdisc+0xc65/0xd47
[ 2033.935747]  rtnetlink_rcv_msg+0x697/0x6c8
[ 2033.935970]  netlink_rcv_skb+0x14d/0x1d6
[ 2033.936186]  rtnetlink_rcv+0x23/0x2a
[ 2033.936407]  netlink_unicast+0x40c/0x532
[ 2033.936628]  netlink_sendmsg+0xa91/0xac9
[ 2033.936845]  sock_sendmsg+0xcd/0xeb
[ 2033.937066]  ___sys_sendmsg+0x582/0x6f1
[ 2033.937290]  __sys_sendmsg+0xc2/0x130
[ 2033.937508]  SyS_sendmsg+0x12/0x1c
[ 2033.937729]  entry_SYSCALL_64_fastpath+0x17/0x98
[ 2033.937950] Freed:
[ 2033.938168] PID = 3462
[ 2033.938387]  save_stack_trace+0x1b/0x1d
[ 2033.938610]  kasan_slab_free+0xaa/0x154
[ 2033.938830]  kfree+0x18c/0x2b3
[ 2033.939054]  skb_free_head+0x92/0x97
[ 2033.939278]  skb_release_data+0x2d7/0x2f3
[ 2033.939494]  skb_release_all+0x5a/0x5d
[ 2033.939718]  __kfree_skb+0x14/0xed
[ 2033.939942]  consume_skb+0xfe/0x18c
[ 2033.940153]  skb_free_datagram+0x17/0xd5
[ 2033.940373]  netlink_recvmsg+0x733/0xb96
[ 2033.940585]  sock_recvmsg+0xd5/0xe0
[ 2033.940805]  ___sys_recvmsg+0x290/0x405
[ 2033.941025]  __sys_recvmsg+0xbf/0x12d
[ 2033.941237]  SyS_recvmsg+0x12/0x1c
[ 2033.941448]  entry_SYSCALL_64_fastpath+0x17/0x98
[ 2033.941661] Memory state around the buggy address:
[ 2033.945246]  ffff8802bfe18000: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[ 2033.945604]  ffff8802bfe18080: 00 00 00 00 00 00 00 00 00 00 00 00 00 
00 00 00
[ 2033.945965] >ffff8802bfe18100: 00 00 00 00 00 00 00 00 fc fc fc fc fc 
fc fc fc
[ 2033.946318]                                            ^
[ 2033.946535]  ffff8802bfe18180: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[ 2033.946886]  ffff8802bfe18200: fc fc fc fc fc fc fc fc fc fc fc fc fc 
fc fc fc
[ 2033.947247] 
==================================================================
[ 2033.947603] Disabling lock debugging due to kernel taint
[ 2033.947845] 
==================================================================


(gdb) list *(tcpmss_tg4+0x6cc)
0x977 is in tcpmss_tg4 (net/netfilter/xt_TCPMSS.c:131).
126		} else
127			newmss = info->mss;
128
129		opt = (u_int8_t *)tcph;
130		for (i = sizeof(struct tcphdr); i < tcp_hdrlen - TCPOLEN_MSS; i += 
optlen(opt, i)) {
131			if (opt[i] == TCPOPT_MSS && opt[i+1] == TCPOLEN_MSS) {
132				u_int16_t oldmss;
133
134				oldmss = (opt[i+2] << 8) | opt[i+3];
135

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 16:52             ` Denys Fedoryshchenko
@ 2017-04-02 17:14               ` Eric Dumazet
  2017-04-02 17:26                 ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 17:14 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On Sun, 2017-04-02 at 19:52 +0300, Denys Fedoryshchenko wrote:
> On 2017-04-02 15:32, Eric Dumazet wrote:
> > On Sun, 2017-04-02 at 15:25 +0300, Denys Fedoryshchenko wrote:
> >> >  	 */
> >> I will add also WARN_ON_ONCE(tcp_hdrlen >= 15 * 4) before, for
> >> curiosity, if this condition are triggered. Is it fine like that?
> > 
> > Sure.
> 
> It didnt triggered WARN_ON, and with both patches here is one more 
> KASAN.
> What i noticed also after this KASAN, there is many others start to 
> trigger in TCPMSS and locking up server by flood.
> There is heavy netlink activity, it is pppoe server with lot of shapers.
> I noticed there left sfq by mistake, usually i am removing it, because 
> it may trigger kernel panic too (and hard to trace reason).
> I will try with pfifo instead, after 6 hours.
> 
> Here is full log with others: https://nuclearcat.com/kasan.txt
> 


Could that be that netfilter does not abort earlier if TCP header is
completely wrong ?

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 27241a767f17b4b27d24095a31e5e9a2d3e29ce4..dd64bff38ba8074e6feb2e6e305eacb30ce4c2da 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -104,7 +104,7 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 	tcp_hdrlen = tcph->doff * 4;
 
-	if (len < tcp_hdrlen)
+	if (len < tcp_hdrlen || tcp_hdrlen < sizeof(struct tcphdr))
 		return -1;
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 17:14               ` Eric Dumazet
@ 2017-04-02 17:26                 ` Eric Dumazet
  2017-04-03  8:10                   ` Denys Fedoryshchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-02 17:26 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On Sun, 2017-04-02 at 10:14 -0700, Eric Dumazet wrote:

> Could that be that netfilter does not abort earlier if TCP header is
> completely wrong ?
> 

Yes, I wonder if this patch would be better, unless we replicate the
th->doff sanity check in all netfilter modules dissecting TCP frames.

diff --git a/net/netfilter/xt_tcpudp.c b/net/netfilter/xt_tcpudp.c
index ade024c90f4f129a7c384e9e1cbfdb8ffe73065f..8cb4eadd5ba1c20e74bc27ee52a0bc36a5b26725 100644
--- a/net/netfilter/xt_tcpudp.c
+++ b/net/netfilter/xt_tcpudp.c
@@ -103,11 +103,11 @@ static bool tcp_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (!NF_INVF(tcpinfo, XT_TCP_INV_FLAGS,
 		     (((unsigned char *)th)[13] & tcpinfo->flg_mask) == tcpinfo->flg_cmp))
 		return false;
+	if (th->doff * 4 < sizeof(_tcph)) {
+		par->hotdrop = true;
+		return false;
+	}
 	if (tcpinfo->option) {
-		if (th->doff * 4 < sizeof(_tcph)) {
-			par->hotdrop = true;
-			return false;
-		}
 		if (!tcp_find_option(tcpinfo->option, skb, par->thoff,
 				     th->doff*4 - sizeof(_tcph),
 				     tcpinfo->invflags & XT_TCP_INV_OPTION,

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-02 17:26                 ` Eric Dumazet
@ 2017-04-03  8:10                   ` Denys Fedoryshchenko
  2017-04-03 12:09                     ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-03  8:10 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On 2017-04-02 20:26, Eric Dumazet wrote:
> On Sun, 2017-04-02 at 10:14 -0700, Eric Dumazet wrote:
> 
>> Could that be that netfilter does not abort earlier if TCP header is
>> completely wrong ?
>> 
> 
> Yes, I wonder if this patch would be better, unless we replicate the
> th->doff sanity check in all netfilter modules dissecting TCP frames.
> 
> diff --git a/net/netfilter/xt_tcpudp.c b/net/netfilter/xt_tcpudp.c
> index
> ade024c90f4f129a7c384e9e1cbfdb8ffe73065f..8cb4eadd5ba1c20e74bc27ee52a0bc36a5b26725
> 100644
> --- a/net/netfilter/xt_tcpudp.c
> +++ b/net/netfilter/xt_tcpudp.c
> @@ -103,11 +103,11 @@ static bool tcp_mt(const struct sk_buff *skb,
> struct xt_action_param *par)
>  	if (!NF_INVF(tcpinfo, XT_TCP_INV_FLAGS,
>  		     (((unsigned char *)th)[13] & tcpinfo->flg_mask) == 
> tcpinfo->flg_cmp))
>  		return false;
> +	if (th->doff * 4 < sizeof(_tcph)) {
> +		par->hotdrop = true;
> +		return false;
> +	}
>  	if (tcpinfo->option) {
> -		if (th->doff * 4 < sizeof(_tcph)) {
> -			par->hotdrop = true;
> -			return false;
> -		}
>  		if (!tcp_find_option(tcpinfo->option, skb, par->thoff,
>  				     th->doff*4 - sizeof(_tcph),
>  				     tcpinfo->invflags & XT_TCP_INV_OPTION,
I modified patch a little as:
if (th->doff * 4 < sizeof(_tcph)) {
  par->hotdrop = true;
  WARN_ON_ONCE(!tcpinfo->option);
  return false;
}

And it did triggered WARN once at morning, and didn't hit KASAN. I will 
run for a while more, to see if it is ok, and then if stable, will try 
to enable SFQ again.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-03  8:10                   ` Denys Fedoryshchenko
@ 2017-04-03 12:09                     ` Eric Dumazet
  2017-04-03 12:14                       ` Denys Fedoryshchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-03 12:09 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote:

> I modified patch a little as:
> if (th->doff * 4 < sizeof(_tcph)) {
>   par->hotdrop = true;
>   WARN_ON_ONCE(!tcpinfo->option);
>   return false;
> }
> 
> And it did triggered WARN once at morning, and didn't hit KASAN. I will 
> run for a while more, to see if it is ok, and then if stable, will try 
> to enable SFQ again.

Excellent news !
We will post an official fix today, thanks a lot for this detective work
Denys.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-03 12:09                     ` Eric Dumazet
@ 2017-04-03 12:14                       ` Denys Fedoryshchenko
  2017-04-03 12:24                         ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-03 12:14 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On 2017-04-03 15:09, Eric Dumazet wrote:
> On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote:
> 
>> I modified patch a little as:
>> if (th->doff * 4 < sizeof(_tcph)) {
>>   par->hotdrop = true;
>>   WARN_ON_ONCE(!tcpinfo->option);
>>   return false;
>> }
>> 
>> And it did triggered WARN once at morning, and didn't hit KASAN. I 
>> will
>> run for a while more, to see if it is ok, and then if stable, will try
>> to enable SFQ again.
> 
> Excellent news !
> We will post an official fix today, thanks a lot for this detective 
> work
> Denys.
I am not sure it is finally fixed, maybe we need test more?
I'm doing extensive tests today with identical configuration (i had to 
run fifo, because customer cannot afford anymore outages). I've dded sfq 
now different way, and identical config i will run after 3 hours approx.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: KASAN, xt_TCPMSS  finally found nasty use-after-free bug? 4.10.8
  2017-04-03 12:14                       ` Denys Fedoryshchenko
@ 2017-04-03 12:24                         ` Eric Dumazet
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2017-04-03 12:24 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Florian Westphal, Linux Kernel Network Developers,
	Pablo Neira Ayuso, Patrick McHardy, Jozsef Kadlecsik,
	netfilter-devel, coreteam, linux-kernel, netdev-owner

On Mon, 2017-04-03 at 15:14 +0300, Denys Fedoryshchenko wrote:
> On 2017-04-03 15:09, Eric Dumazet wrote:
> > On Mon, 2017-04-03 at 11:10 +0300, Denys Fedoryshchenko wrote:
> > 
> >> I modified patch a little as:
> >> if (th->doff * 4 < sizeof(_tcph)) {
> >>   par->hotdrop = true;
> >>   WARN_ON_ONCE(!tcpinfo->option);
> >>   return false;
> >> }
> >> 
> >> And it did triggered WARN once at morning, and didn't hit KASAN. I 
> >> will
> >> run for a while more, to see if it is ok, and then if stable, will try
> >> to enable SFQ again.
> > 
> > Excellent news !
> > We will post an official fix today, thanks a lot for this detective 
> > work
> > Denys.
> I am not sure it is finally fixed, maybe we need test more?
> I'm doing extensive tests today with identical configuration (i had to 
> run fifo, because customer cannot afford anymore outages). I've dded sfq 
> now different way, and identical config i will run after 3 hours approx.

I did not say this bug fix would be the last one.

But this check is definitely needed, otherwise xt_TCPMSS can iterate
whole memory, and either crash or scramble two bytes in memory, that do
not belong to the frame at all.

If tcp_hdrlen is 0 (can happen if tcph->doff is 0)

Then :

for (i = sizeof(struct tcphdr); i <= tcp_hdrlen - TCPOLEN_MSS; i +=
optlen(opt, i)) {

Can look at 4 GB of memory, until it finds a 0x02, 0x04  sequence.

It can also loop forever in some cases, depending on memory content.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff
  2017-04-02 11:24 ` Eric Dumazet
  2017-04-02 11:45   ` Florian Westphal
@ 2017-04-03 17:55   ` Eric Dumazet
  2017-04-08 20:24     ` Pablo Neira Ayuso
  1 sibling, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2017-04-03 17:55 UTC (permalink / raw)
  To: Denys Fedoryshchenko
  Cc: Linux Kernel Network Developers, Pablo Neira Ayuso, netfilter-devel

From: Eric Dumazet <edumazet@google.com>

Denys provided an awesome KASAN report pointing to an use
after free in xt_TCPMSS

I have provided three patches to fix this issue, either in xt_TCPMSS or
in xt_tcpudp.c. It seems xt_TCPMSS patch has the smallest possible
impact.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Denys Fedoryshchenko <nuclearcat@nuclearcat.com>
---
 net/netfilter/xt_TCPMSS.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/xt_TCPMSS.c b/net/netfilter/xt_TCPMSS.c
index 27241a767f17b4b27d24095a31e5e9a2d3e29ce4..c64aca611ac5c5f81ad7c925652bbb90554763ac 100644
--- a/net/netfilter/xt_TCPMSS.c
+++ b/net/netfilter/xt_TCPMSS.c
@@ -104,7 +104,7 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	tcph = (struct tcphdr *)(skb_network_header(skb) + tcphoff);
 	tcp_hdrlen = tcph->doff * 4;
 
-	if (len < tcp_hdrlen)
+	if (len < tcp_hdrlen || tcp_hdrlen < sizeof(struct tcphdr))
 		return -1;
 
 	if (info->mss == XT_TCPMSS_CLAMP_PMTU) {
@@ -152,6 +152,10 @@ tcpmss_mangle_packet(struct sk_buff *skb,
 	if (len > tcp_hdrlen)
 		return 0;
 
+	/* tcph->doff has 4 bits, do not wrap it to 0 */
+	if (tcp_hdrlen >= 15 * 4)
+		return 0;
+
 	/*
 	 * MSS Option not found ?! add it..
 	 */

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff
  2017-04-03 17:55   ` [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff Eric Dumazet
@ 2017-04-08 20:24     ` Pablo Neira Ayuso
  2017-04-20 18:14       ` Denys Fedoryshchenko
  0 siblings, 1 reply; 18+ messages in thread
From: Pablo Neira Ayuso @ 2017-04-08 20:24 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Denys Fedoryshchenko, Linux Kernel Network Developers, netfilter-devel

On Mon, Apr 03, 2017 at 10:55:11AM -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Denys provided an awesome KASAN report pointing to an use
> after free in xt_TCPMSS
> 
> I have provided three patches to fix this issue, either in xt_TCPMSS or
> in xt_tcpudp.c. It seems xt_TCPMSS patch has the smallest possible
> impact.

Applied to nf.git, thanks!

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff
  2017-04-08 20:24     ` Pablo Neira Ayuso
@ 2017-04-20 18:14       ` Denys Fedoryshchenko
  0 siblings, 0 replies; 18+ messages in thread
From: Denys Fedoryshchenko @ 2017-04-20 18:14 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Eric Dumazet, Linux Kernel Network Developers, netfilter-devel

On 2017-04-08 23:24, Pablo Neira Ayuso wrote:
> On Mon, Apr 03, 2017 at 10:55:11AM -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>> 
>> Denys provided an awesome KASAN report pointing to an use
>> after free in xt_TCPMSS
>> 
>> I have provided three patches to fix this issue, either in xt_TCPMSS 
>> or
>> in xt_tcpudp.c. It seems xt_TCPMSS patch has the smallest possible
>> impact.
> 
> Applied to nf.git, thanks!
Any plans to queue it to stable trees?
It seems affected kernel for years.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2017-04-20 18:14 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-02  7:43 KASAN, xt_TCPMSS finally found nasty use-after-free bug? 4.10.8 Denys Fedoryshchenko
2017-04-02 11:24 ` Eric Dumazet
2017-04-02 11:45   ` Florian Westphal
2017-04-02 11:51     ` Denys Fedoryshchenko
2017-04-02 11:54     ` Eric Dumazet
2017-04-02 12:19       ` Eric Dumazet
2017-04-02 12:25         ` Denys Fedoryshchenko
2017-04-02 12:32           ` Eric Dumazet
2017-04-02 16:52             ` Denys Fedoryshchenko
2017-04-02 17:14               ` Eric Dumazet
2017-04-02 17:26                 ` Eric Dumazet
2017-04-03  8:10                   ` Denys Fedoryshchenko
2017-04-03 12:09                     ` Eric Dumazet
2017-04-03 12:14                       ` Denys Fedoryshchenko
2017-04-03 12:24                         ` Eric Dumazet
2017-04-03 17:55   ` [PATCH net] netfilter: xt_TCPMSS: add more sanity tests on tcph->doff Eric Dumazet
2017-04-08 20:24     ` Pablo Neira Ayuso
2017-04-20 18:14       ` Denys Fedoryshchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.