Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1687570 > unrolled thread

[PATCH] ipv6: initialize treq->txhash in cookie_v6_check()

Started byAlexander Potapenko <glider@google.com>
First post2017-07-14 19:00 +0200
Last post2017-07-17 10:00 +0200
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Alexander Potapenko <glider@google.com> - 2017-07-14 19:00 +0200
    Re: [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Neal Cardwell <ncardwell@google.com> - 2017-07-14 19:10 +0200
      Re: [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Alexander Potapenko <glider@google.com> - 2017-07-14 19:40 +0200
        Re: [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Neal Cardwell <ncardwell@google.com> - 2017-07-14 21:10 +0200
    Re: [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Eric Dumazet <eric.dumazet@gmail.com> - 2017-07-17 09:40 +0200
      Re: [PATCH] ipv6: initialize treq->txhash in cookie_v6_check() Eric Dumazet <eric.dumazet@gmail.com> - 2017-07-17 10:00 +0200

#1687570 — [PATCH] ipv6: initialize treq->txhash in cookie_v6_check()

FromAlexander Potapenko <glider@google.com>
Date2017-07-14 19:00 +0200
Subject[PATCH] ipv6: initialize treq->txhash in cookie_v6_check()
Message-ID<u3c6B-686-11@gated-at.bofh.it>
KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
which originated from the TCP request socket created in
cookie_v6_check():

 ==================================================================
 BUG: KMSAN: use of uninitialized memory in tcp_transmit_skb+0xf77/0x3ec0
 CPU: 1 PID: 2949 Comm: syz-execprog Not tainted 4.11.0-rc5+ #2931
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 TCP: request_sock_TCPv6: Possible SYN flooding on port 20028. Sending cookies.  Check SNMP counters.
 Call Trace:
  <IRQ>
  __dump_stack lib/dump_stack.c:16
  dump_stack+0x172/0x1c0 lib/dump_stack.c:52
  kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:927
  __msan_warning_32+0x61/0xb0 mm/kmsan/kmsan_instr.c:469
  skb_set_hash_from_sk ./include/net/sock.h:2011
  tcp_transmit_skb+0xf77/0x3ec0 net/ipv4/tcp_output.c:983
  tcp_send_ack+0x75b/0x830 net/ipv4/tcp_output.c:3493
  tcp_delack_timer_handler+0x9a6/0xb90 net/ipv4/tcp_timer.c:284
  tcp_delack_timer+0x1b0/0x310 net/ipv4/tcp_timer.c:309
  call_timer_fn+0x240/0x520 kernel/time/timer.c:1268
  expire_timers kernel/time/timer.c:1307
  __run_timers+0xc13/0xf10 kernel/time/timer.c:1601
  run_timer_softirq+0x36/0xa0 kernel/time/timer.c:1614
  __do_softirq+0x485/0x942 kernel/softirq.c:284
  invoke_softirq kernel/softirq.c:364
  irq_exit+0x1fa/0x230 kernel/softirq.c:405
  exiting_irq+0xe/0x10 ./arch/x86/include/asm/apic.h:657
  smp_apic_timer_interrupt+0x5a/0x80 arch/x86/kernel/apic/apic.c:966
  apic_timer_interrupt+0x86/0x90 arch/x86/entry/entry_64.S:489
 RIP: 0010:native_restore_fl ./arch/x86/include/asm/irqflags.h:36
 RIP: 0010:arch_local_irq_restore ./arch/x86/include/asm/irqflags.h:77
 RIP: 0010:__msan_poison_alloca+0xed/0x120 mm/kmsan/kmsan_instr.c:440
 RSP: 0018:ffff880024917cd8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff10
 RAX: 0000000000000246 RBX: ffff8800224c0000 RCX: 0000000000000005
 RDX: 0000000000000004 RSI: ffff880000000000 RDI: ffffea0000b6d770
 RBP: ffff880024917d58 R08: 0000000000000dd8 R09: 0000000000000004
 R10: 0000160000000000 R11: 0000000000000000 R12: ffffffff85abf810
 R13: ffff880024917dd8 R14: 0000000000000010 R15: ffffffff81cabde4
  </IRQ>
  poll_select_copy_remaining+0xac/0x6b0 fs/select.c:293
  SYSC_select+0x4b4/0x4e0 fs/select.c:653
  SyS_select+0x76/0xa0 fs/select.c:634
  entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204
 RIP: 0033:0x4597e7
 RSP: 002b:000000c420037ee0 EFLAGS: 00000246 ORIG_RAX: 0000000000000017
 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00000000004597e7
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: 000000c420037ef0 R08: 000000c420037ee0 R09: 0000000000000059
 R10: 0000000000000000 R11: 0000000000000246 R12: 000000000042dc20
 R13: 00000000000000f3 R14: 0000000000000030 R15: 0000000000000003
 chained origin:
  save_stack_trace+0x37/0x40 arch/x86/kernel/stacktrace.c:59
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302
  kmsan_save_stack mm/kmsan/kmsan.c:317
  kmsan_internal_chain_origin+0x12a/0x1f0 mm/kmsan/kmsan.c:547
  __msan_store_shadow_origin_4+0xac/0x110 mm/kmsan/kmsan_instr.c:259
  tcp_create_openreq_child+0x709/0x1ae0 net/ipv4/tcp_minisocks.c:472
  tcp_v6_syn_recv_sock+0x7eb/0x2a30 net/ipv6/tcp_ipv6.c:1103
  tcp_get_cookie_sock+0x136/0x5f0 net/ipv4/syncookies.c:212
  cookie_v6_check+0x17a9/0x1b50 net/ipv6/syncookies.c:245
  tcp_v6_cookie_check net/ipv6/tcp_ipv6.c:989
  tcp_v6_do_rcv+0xdd8/0x1c60 net/ipv6/tcp_ipv6.c:1298
  tcp_v6_rcv+0x41a3/0x4f00 net/ipv6/tcp_ipv6.c:1487
  ip6_input_finish+0x82f/0x1ee0 net/ipv6/ip6_input.c:279
  NF_HOOK ./include/linux/netfilter.h:257
  ip6_input+0x239/0x290 net/ipv6/ip6_input.c:322
  dst_input ./include/net/dst.h:492
  ip6_rcv_finish net/ipv6/ip6_input.c:69
  NF_HOOK ./include/linux/netfilter.h:257
  ipv6_rcv+0x1dbd/0x22e0 net/ipv6/ip6_input.c:203
  __netif_receive_skb_core+0x2f6f/0x3a20 net/core/dev.c:4208
  __netif_receive_skb net/core/dev.c:4246
  process_backlog+0x667/0xba0 net/core/dev.c:4866
  napi_poll net/core/dev.c:5268
  net_rx_action+0xc95/0x1590 net/core/dev.c:5333
  __do_softirq+0x485/0x942 kernel/softirq.c:284
 origin:
  save_stack_trace+0x37/0x40 arch/x86/kernel/stacktrace.c:59
  kmsan_save_stack_with_flags mm/kmsan/kmsan.c:302
  kmsan_internal_poison_shadow+0xb1/0x1a0 mm/kmsan/kmsan.c:198
  kmsan_kmalloc+0x7f/0xe0 mm/kmsan/kmsan.c:337
  kmem_cache_alloc+0x1c2/0x1e0 mm/slub.c:2766
  reqsk_alloc ./include/net/request_sock.h:87
  inet_reqsk_alloc+0xa4/0x5b0 net/ipv4/tcp_input.c:6200
  cookie_v6_check+0x4f4/0x1b50 net/ipv6/syncookies.c:169
  tcp_v6_cookie_check net/ipv6/tcp_ipv6.c:989
  tcp_v6_do_rcv+0xdd8/0x1c60 net/ipv6/tcp_ipv6.c:1298
  tcp_v6_rcv+0x41a3/0x4f00 net/ipv6/tcp_ipv6.c:1487
  ip6_input_finish+0x82f/0x1ee0 net/ipv6/ip6_input.c:279
  NF_HOOK ./include/linux/netfilter.h:257
  ip6_input+0x239/0x290 net/ipv6/ip6_input.c:322
  dst_input ./include/net/dst.h:492
  ip6_rcv_finish net/ipv6/ip6_input.c:69
  NF_HOOK ./include/linux/netfilter.h:257
  ipv6_rcv+0x1dbd/0x22e0 net/ipv6/ip6_input.c:203
  __netif_receive_skb_core+0x2f6f/0x3a20 net/core/dev.c:4208
  __netif_receive_skb net/core/dev.c:4246
  process_backlog+0x667/0xba0 net/core/dev.c:4866
  napi_poll net/core/dev.c:5268
  net_rx_action+0xc95/0x1590 net/core/dev.c:5333
  __do_softirq+0x485/0x942 kernel/softirq.c:284
 ==================================================================

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 net/ipv6/syncookies.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 7b75b0620730..b4b354502c6e 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	treq->rcv_isn = ntohl(th->seq) - 1;
 	treq->snt_isn = cookie;
 	treq->ts_off = 0;
+	treq->txhash = 0;
 
 	/*
 	 * We need to lookup the dst_entry to get the correct window size.
-- 
2.13.2.932.g7449e964c-goog

[toc] | [next] | [standalone]

#1687571

FromNeal Cardwell <ncardwell@google.com>
Date2017-07-14 19:10 +0200
Message-ID<u3cgi-6qU-13@gated-at.bofh.it>
In reply to#1687570
On Fri, Jul 14, 2017 at 12:54 PM, Alexander Potapenko <glider@google.com> wrote:
> KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
> which originated from the TCP request socket created in
> cookie_v6_check():
...
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>         treq->rcv_isn = ntohl(th->seq) - 1;
>         treq->snt_isn = cookie;
>         treq->ts_off = 0;
> +       treq->txhash = 0;
>
>         /*
>          * We need to lookup the dst_entry to get the correct window size.

I would have thought that the same fix is needed in the corresponding
line in cookie_v4_check() in net/ipv4/syncookies.c? (I do not see
txhash being initialized for the IPv4 side.) If it's not needed for
some reason, then it would be worth a comment in the commit
description to explain why not.

thanks,
neal

[toc] | [prev] | [next] | [standalone]

#1687586

FromAlexander Potapenko <glider@google.com>
Date2017-07-14 19:40 +0200
Message-ID<u3cJl-6Ez-35@gated-at.bofh.it>
In reply to#1687571
On Fri, Jul 14, 2017 at 7:04 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Fri, Jul 14, 2017 at 12:54 PM, Alexander Potapenko <glider@google.com> wrote:
>> KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
>> which originated from the TCP request socket created in
>> cookie_v6_check():
> ...
>> --- a/net/ipv6/syncookies.c
>> +++ b/net/ipv6/syncookies.c
>> @@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>>         treq->rcv_isn = ntohl(th->seq) - 1;
>>         treq->snt_isn = cookie;
>>         treq->ts_off = 0;
>> +       treq->txhash = 0;
>>
>>         /*
>>          * We need to lookup the dst_entry to get the correct window size.
>
> I would have thought that the same fix is needed in the corresponding
> line in cookie_v4_check() in net/ipv4/syncookies.c? (I do not see
> txhash being initialized for the IPv4 side.) If it's not needed for
> some reason, then it would be worth a comment in the commit
> description to explain why not.
Most certainly it is needed. I haven't seen reports for that in the
wild and couldn't forge a repro triggering the bug in IPv4, but I'll
give it another shot.
> thanks,
> neal



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

[toc] | [prev] | [next] | [standalone]

#1687609

FromNeal Cardwell <ncardwell@google.com>
Date2017-07-14 21:10 +0200
Message-ID<u3e8p-7II-7@gated-at.bofh.it>
In reply to#1687586
On Fri, Jul 14, 2017 at 1:35 PM, Alexander Potapenko <glider@google.com> wrote:
> On Fri, Jul 14, 2017 at 7:04 PM, Neal Cardwell <ncardwell@google.com> wrote:
>> On Fri, Jul 14, 2017 at 12:54 PM, Alexander Potapenko <glider@google.com> wrote:
>>> KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
>>> which originated from the TCP request socket created in
>>> cookie_v6_check():
>> ...
>>> --- a/net/ipv6/syncookies.c
>>> +++ b/net/ipv6/syncookies.c
>>> @@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>>>         treq->rcv_isn = ntohl(th->seq) - 1;
>>>         treq->snt_isn = cookie;
>>>         treq->ts_off = 0;
>>> +       treq->txhash = 0;
>>>
>>>         /*
>>>          * We need to lookup the dst_entry to get the correct window size.
>>
>> I would have thought that the same fix is needed in the corresponding
>> line in cookie_v4_check() in net/ipv4/syncookies.c? (I do not see
>> txhash being initialized for the IPv4 side.) If it's not needed for
>> some reason, then it would be worth a comment in the commit
>> description to explain why not.
> Most certainly it is needed. I haven't seen reports for that in the
> wild and couldn't forge a repro triggering the bug in IPv4, but I'll
> give it another shot.

If you force syncookies to be used, with:

  sysctl -q net.ipv4.tcp_syncookies=2

then every passive IPv4 TCP connection that is accepted by a TCP
server app should be using that uninitialized memory, I would think
(and thus should be liable to fire the KMSAN use of uninitialized
memory warning).

Does that work?

neal

[toc] | [prev] | [next] | [standalone]

#1688728

FromEric Dumazet <eric.dumazet@gmail.com>
Date2017-07-17 09:40 +0200
Message-ID<u48Nk-232-31@gated-at.bofh.it>
In reply to#1687570
On Fri, 2017-07-14 at 18:54 +0200, Alexander Potapenko wrote:
> KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
> which originated from the TCP request socket created in
> cookie_v6_check():

>  ==================================================================
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  net/ipv6/syncookies.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index 7b75b0620730..b4b354502c6e 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
>  	treq->rcv_isn = ntohl(th->seq) - 1;
>  	treq->snt_isn = cookie;
>  	treq->ts_off = 0;
> +	treq->txhash = 0;
>  
>  	/*
>  	 * We need to lookup the dst_entry to get the correct window size.

Please use net_tx_rndhash() instead of 0, thanks.

( And same fix is needed for IPv4, as Neal mentioned already )

[toc] | [prev] | [next] | [standalone]

#1688736

FromEric Dumazet <eric.dumazet@gmail.com>
Date2017-07-17 10:00 +0200
Message-ID<u496G-29K-13@gated-at.bofh.it>
In reply to#1688728
On Mon, 2017-07-17 at 00:39 -0700, Eric Dumazet wrote:
> On Fri, 2017-07-14 at 18:54 +0200, Alexander Potapenko wrote:
> > KMSAN reported use of uninitialized memory in skb_set_hash_from_sk(),
> > which originated from the TCP request socket created in
> > cookie_v6_check():
> 
> >  ==================================================================
> > 
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  net/ipv6/syncookies.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> > index 7b75b0620730..b4b354502c6e 100644
> > --- a/net/ipv6/syncookies.c
> > +++ b/net/ipv6/syncookies.c
> > @@ -216,6 +216,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
> >  	treq->rcv_isn = ntohl(th->seq) - 1;
> >  	treq->snt_isn = cookie;
> >  	treq->ts_off = 0;
> > +	treq->txhash = 0;
> >  
> >  	/*
> >  	 * We need to lookup the dst_entry to get the correct window size.
> 
> Please use net_tx_rndhash() instead of 0, thanks.
> 
> ( And same fix is needed for IPv4, as Neal mentioned already )

Also please add the following tag.

Fixes: 58d607d3e52f ("tcp: provide skb->hash to synack packets")

Thanks again !

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web