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

Groups > linux.kernel > #1580782 > unrolled thread

Re: net: use-after-free in tw_timer_handler

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: net: use-after-free in tw_timer_handler Dmitry Vyukov <dvyukov@google.com> - 2017-02-14 20:40 +0100
    Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() Dmitry Vyukov <dvyukov@google.com> - 2017-02-21 13:00 +0100
      Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() Dmitry Vyukov <dvyukov@google.com> - 2017-02-22 07:50 +0100
    Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-21 14:50 +0100
      Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() Eric Dumazet <edumazet@google.com> - 2017-02-21 15:00 +0100
        Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() David Miller <davem@davemloft.net> - 2017-02-21 19:30 +0100
    Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() David Miller <davem@davemloft.net> - 2017-02-21 19:30 +0100
      Re: [PATCH] net/dccp: fix use after free in tw_timer_handler() David Miller <davem@davemloft.net> - 2017-02-21 19:30 +0100

#1580782 — Re: net: use-after-free in tw_timer_handler

On Wed, Feb 8, 2017 at 8:32 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> >> >>> This code was changed a long time ago :
>>> >> >>>
>>> >> >>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=ed2e923945892a8372ab70d2f61d364b0b6d9054
>>> >> >>>
>>> >> >>> So I suspect a recent patch broke the logic.
>>> >> >>>
>>> >> >>> You might start a bisection :
>>> >> >>>
>>> >> >>> I would check if 4.7 and 4.8 trigger the issue you noticed.
>>> >> >>
>>> >> >>
>>> >> >> It happens with too low rate for bisecting (few times per day). I
>>> >> >> could add some additional checks into code, but I don't know what
>>> >> >> checks could be useful.
>>> >> >
>>> >> > If you can not tell if 4.7 and/or 4.8 have the problem, I am not sure
>>> >> > we are able to help.
>>> >>
>>> >>
>>> >> There are also chances that the problem is older.
>>> >>
>>> >> Looking at the code, this part of inet_twsk_purge looks fishy:
>>> >>
>>> >> 285                         if (unlikely((tw->tw_family != family) ||
>>> >> 286                                      atomic_read(&twsk_net(tw)->count))) {
>>> >>
>>> >> It uses net->count == 0 check to find the right sockets. But what if
>>> >> there are several nets with count == 0 in flight, can't there be
>>> >> several inet_twsk_purge calls running concurrently freeing each other
>>> >> sockets? If so it looks like inet_twsk_purge can call
>>> >> inet_twsk_deschedule_put twice for a socket. Namely, two calls for
>>> >> different nets discover the socket, check that net->count==0 and both
>>> >> call inet_twsk_deschedule_put. Shouldn't we just give inet_twsk_purge
>>> >> net that it needs to purge?
>>> >
>>> > Yes, atomic_read() is not a proper sync point.
>>>
>>> Do you mean that it does not include read barrier?
>>> I more mean that we can call inet_twsk_deschedule_put twice for the same socket.
>>
>> I meant that this code assumed RTNL being held.
>>
>> This might not be the case now, after some old change.
>
>
> cleanup_net releases rtnl lock right before calling these callbacks.


+Andrey, do you know somebody on your side interested in stability of
network namespace?
This use-after-free seems to be related to net namespace. For context,
full thread is here:
https://groups.google.com/forum/#!msg/syzkaller/p1tn-_Kc6l4/smuL_FMAAgAJ

[toc] | [next] | [standalone]

#1585250 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

On Tue, Feb 21, 2017 at 2:27 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> DCCP doesn't purge timewait sockets on network namespace shutdown.
> So, after net namespace destroyed we could still have an active timer
> which will trigger use after free in tw_timer_handler():
>
>     BUG: KASAN: use-after-free in tw_timer_handler+0x4a/0xa0 at addr ffff88010e0d1e10
>     Read of size 8 by task swapper/1/0
>     Call Trace:
>      __asan_load8+0x54/0x90
>      tw_timer_handler+0x4a/0xa0
>      call_timer_fn+0x127/0x480
>      expire_timers+0x1db/0x2e0
>      run_timer_softirq+0x12f/0x2a0
>      __do_softirq+0x105/0x5b4
>      irq_exit+0xdd/0xf0
>      smp_apic_timer_interrupt+0x57/0x70
>      apic_timer_interrupt+0x90/0xa0
>
>     Object at ffff88010e0d1bc0, in cache net_namespace size: 6848
>     Allocated:
>      save_stack_trace+0x1b/0x20
>      kasan_kmalloc+0xee/0x180
>      kasan_slab_alloc+0x12/0x20
>      kmem_cache_alloc+0x134/0x310
>      copy_net_ns+0x8d/0x280
>      create_new_namespaces+0x23f/0x340
>      unshare_nsproxy_namespaces+0x75/0xf0
>      SyS_unshare+0x299/0x4f0
>      entry_SYSCALL_64_fastpath+0x18/0xad
>     Freed:
>      save_stack_trace+0x1b/0x20
>      kasan_slab_free+0xae/0x180
>      kmem_cache_free+0xb4/0x350
>      net_drop_ns+0x3f/0x50
>      cleanup_net+0x3df/0x450
>      process_one_work+0x419/0xbb0
>      worker_thread+0x92/0x850
>      kthread+0x192/0x1e0
>      ret_from_fork+0x2e/0x40
>
> Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
> timewait sockets on net namespace destruction and prevent above issue.


Aha! Thanks for tracking this down!

Queued the patch on syzkaller bots, should have the verdict in several hours.



> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  net/dccp/ipv4.c | 6 ++++++
>  net/dccp/ipv6.c | 6 ++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
> index d859a5c..da7cb16 100644
> --- a/net/dccp/ipv4.c
> +++ b/net/dccp/ipv4.c
> @@ -1018,9 +1018,15 @@ static void __net_exit dccp_v4_exit_net(struct net *net)
>         inet_ctl_sock_destroy(net->dccp.v4_ctl_sk);
>  }
>
> +static void __net_exit dccp_v4_exit_batch(struct list_head *net_exit_list)
> +{
> +       inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET);
> +}
> +
>  static struct pernet_operations dccp_v4_ops = {
>         .init   = dccp_v4_init_net,
>         .exit   = dccp_v4_exit_net,
> +       .exit_batch = dccp_v4_exit_batch,
>  };
>
>  static int __init dccp_v4_init(void)
> diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> index c4e879c..f3d8f92 100644
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -1077,9 +1077,15 @@ static void __net_exit dccp_v6_exit_net(struct net *net)
>         inet_ctl_sock_destroy(net->dccp.v6_ctl_sk);
>  }
>
> +static void __net_exit dccp_v6_exit_batch(struct list_head *net_exit_list)
> +{
> +       inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET6);
> +}
> +
>  static struct pernet_operations dccp_v6_ops = {
>         .init   = dccp_v6_init_net,
>         .exit   = dccp_v6_exit_net,
> +       .exit_batch = dccp_v6_exit_batch,
>  };
>
>  static int __init dccp_v6_init(void)
> --
> 2.10.2
>

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

#1585953 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

On Tue, Feb 21, 2017 at 2:56 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Feb 21, 2017 at 2:27 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> DCCP doesn't purge timewait sockets on network namespace shutdown.
>> So, after net namespace destroyed we could still have an active timer
>> which will trigger use after free in tw_timer_handler():
>>
>>     BUG: KASAN: use-after-free in tw_timer_handler+0x4a/0xa0 at addr ffff88010e0d1e10
>>     Read of size 8 by task swapper/1/0
>>     Call Trace:
>>      __asan_load8+0x54/0x90
>>      tw_timer_handler+0x4a/0xa0
>>      call_timer_fn+0x127/0x480
>>      expire_timers+0x1db/0x2e0
>>      run_timer_softirq+0x12f/0x2a0
>>      __do_softirq+0x105/0x5b4
>>      irq_exit+0xdd/0xf0
>>      smp_apic_timer_interrupt+0x57/0x70
>>      apic_timer_interrupt+0x90/0xa0
>>
>>     Object at ffff88010e0d1bc0, in cache net_namespace size: 6848
>>     Allocated:
>>      save_stack_trace+0x1b/0x20
>>      kasan_kmalloc+0xee/0x180
>>      kasan_slab_alloc+0x12/0x20
>>      kmem_cache_alloc+0x134/0x310
>>      copy_net_ns+0x8d/0x280
>>      create_new_namespaces+0x23f/0x340
>>      unshare_nsproxy_namespaces+0x75/0xf0
>>      SyS_unshare+0x299/0x4f0
>>      entry_SYSCALL_64_fastpath+0x18/0xad
>>     Freed:
>>      save_stack_trace+0x1b/0x20
>>      kasan_slab_free+0xae/0x180
>>      kmem_cache_free+0xb4/0x350
>>      net_drop_ns+0x3f/0x50
>>      cleanup_net+0x3df/0x450
>>      process_one_work+0x419/0xbb0
>>      worker_thread+0x92/0x850
>>      kthread+0x192/0x1e0
>>      ret_from_fork+0x2e/0x40
>>
>> Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
>> timewait sockets on net namespace destruction and prevent above issue.
>
>
> Aha! Thanks for tracking this down!
>
> Queued the patch on syzkaller bots, should have the verdict in several hours.

I do not see the crash happening after applying the patch.

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

#1585345 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

Em Tue, Feb 21, 2017 at 02:27:40PM +0300, Andrey Ryabinin escreveu:
> DCCP doesn't purge timewait sockets on network namespace shutdown.
> So, after net namespace destroyed we could still have an active timer
> which will trigger use after free in tw_timer_handler():
> 
> 
> Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
> timewait sockets on net namespace destruction and prevent above issue.

Please add this, to help stable kernels to pick this up

Fixes: b099ce2602d8 ("net: Batch inet_twsk_purge")
Cc: Eric W. Biederman <ebiederm@xmission.com> 

[acme@jouet linux]$ git describe b099ce2602d8
v2.6.32-rc8-1977-gb099ce2602d8

This one added the pernet operations related to network namespaces, but
then the one above got missed.

commit 72a2d6138224298a576bcdc33d7d0004de604856
Author: Pavel Emelyanov <xemul@openvz.org>
Date:   Sun Apr 13 22:29:13 2008 -0700

    [NETNS][DCCPV4]: Add dummy per-net operations.

----------------------------------

It looks ok, so please consider adding my:

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
>  net/dccp/ipv4.c | 6 ++++++
>  net/dccp/ipv6.c | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
> index d859a5c..da7cb16 100644
> --- a/net/dccp/ipv4.c
> +++ b/net/dccp/ipv4.c
> @@ -1018,9 +1018,15 @@ static void __net_exit dccp_v4_exit_net(struct net *net)
>  	inet_ctl_sock_destroy(net->dccp.v4_ctl_sk);
>  }
>  
> +static void __net_exit dccp_v4_exit_batch(struct list_head *net_exit_list)
> +{
> +	inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET);
> +}
> +
>  static struct pernet_operations dccp_v4_ops = {
>  	.init	= dccp_v4_init_net,
>  	.exit	= dccp_v4_exit_net,
> +	.exit_batch = dccp_v4_exit_batch,
>  };
>  
>  static int __init dccp_v4_init(void)
> diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> index c4e879c..f3d8f92 100644
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -1077,9 +1077,15 @@ static void __net_exit dccp_v6_exit_net(struct net *net)
>  	inet_ctl_sock_destroy(net->dccp.v6_ctl_sk);
>  }
>  
> +static void __net_exit dccp_v6_exit_batch(struct list_head *net_exit_list)
> +{
> +	inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET6);
> +}
> +
>  static struct pernet_operations dccp_v6_ops = {
>  	.init   = dccp_v6_init_net,
>  	.exit   = dccp_v6_exit_net,
> +	.exit_batch = dccp_v6_exit_batch,
>  };
>  
>  static int __init dccp_v6_init(void)
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1585350 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

On Tue, Feb 21, 2017 at 5:43 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Feb 21, 2017 at 02:27:40PM +0300, Andrey Ryabinin escreveu:
> > DCCP doesn't purge timewait sockets on network namespace shutdown.
> > So, after net namespace destroyed we could still have an active timer
> > which will trigger use after free in tw_timer_handler():
> >
> >
> > Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
> > timewait sockets on net namespace destruction and prevent above issue.
>
> Please add this, to help stable kernels to pick this up
>
> Fixes: b099ce2602d8 ("net: Batch inet_twsk_purge")
> Cc: Eric W. Biederman <ebiederm@xmission.com>


This patch has nothing to do with this bug really.

Look at commit d315492b1a6ba29da0fa2860759505ae1b2db857
("netns : fix kernel panic in timewait socket destruction")

Back in 2008, nobody spotted that DCCP was using the same infra.

When can we get rid of DCCP in linux so that syszkaller team no longer
spend time on it ?

Thanks.

>
> [acme@jouet linux]$ git describe b099ce2602d8
> v2.6.32-rc8-1977-gb099ce2602d8
>
> This one added the pernet operations related to network namespaces, but
> then the one above got missed.
>
> commit 72a2d6138224298a576bcdc33d7d0004de604856
> Author: Pavel Emelyanov <xemul@openvz.org>
> Date:   Sun Apr 13 22:29:13 2008 -0700
>
>     [NETNS][DCCPV4]: Add dummy per-net operations.
>
> ----------------------------------
>
> It looks ok, so please consider adding my:
>
> Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> - Arnaldo
>
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > ---
> >  net/dccp/ipv4.c | 6 ++++++
> >  net/dccp/ipv6.c | 6 ++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
> > index d859a5c..da7cb16 100644
> > --- a/net/dccp/ipv4.c
> > +++ b/net/dccp/ipv4.c
> > @@ -1018,9 +1018,15 @@ static void __net_exit dccp_v4_exit_net(struct net *net)
> >       inet_ctl_sock_destroy(net->dccp.v4_ctl_sk);
> >  }
> >
> > +static void __net_exit dccp_v4_exit_batch(struct list_head *net_exit_list)
> > +{
> > +     inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET);
> > +}
> > +
> >  static struct pernet_operations dccp_v4_ops = {
> >       .init   = dccp_v4_init_net,
> >       .exit   = dccp_v4_exit_net,
> > +     .exit_batch = dccp_v4_exit_batch,
> >  };
> >
> >  static int __init dccp_v4_init(void)
> > diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
> > index c4e879c..f3d8f92 100644
> > --- a/net/dccp/ipv6.c
> > +++ b/net/dccp/ipv6.c
> > @@ -1077,9 +1077,15 @@ static void __net_exit dccp_v6_exit_net(struct net *net)
> >       inet_ctl_sock_destroy(net->dccp.v6_ctl_sk);
> >  }
> >
> > +static void __net_exit dccp_v6_exit_batch(struct list_head *net_exit_list)
> > +{
> > +     inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET6);
> > +}
> > +
> >  static struct pernet_operations dccp_v6_ops = {
> >       .init   = dccp_v6_init_net,
> >       .exit   = dccp_v6_exit_net,
> > +     .exit_batch = dccp_v6_exit_batch,
> >  };
> >
> >  static int __init dccp_v6_init(void)
> > --
> > 2.10.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe dccp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1585599 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 21 Feb 2017 05:53:13 -0800

> On Tue, Feb 21, 2017 at 5:43 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
>>
>> Em Tue, Feb 21, 2017 at 02:27:40PM +0300, Andrey Ryabinin escreveu:
>> > DCCP doesn't purge timewait sockets on network namespace shutdown.
>> > So, after net namespace destroyed we could still have an active timer
>> > which will trigger use after free in tw_timer_handler():
>> >
>> >
>> > Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
>> > timewait sockets on net namespace destruction and prevent above issue.
>>
>> Please add this, to help stable kernels to pick this up
>>
>> Fixes: b099ce2602d8 ("net: Batch inet_twsk_purge")
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
> 
> 
> This patch has nothing to do with this bug really.
> 
> Look at commit d315492b1a6ba29da0fa2860759505ae1b2db857
> ("netns : fix kernel panic in timewait socket destruction")
> 
> Back in 2008, nobody spotted that DCCP was using the same infra.

So, let me get this straight, dccp is buggy because it tried as hard as
possible to share and use common pieces of infrastructure instead of
duplicating all of said logic?

Now I've heard everything.

I know it has been a pain in the rear fixing all of these dccp bugs,
but removing it from the tree or even pushing it into staging is
simply not an option.  So we better come up with a better plan based
upon reality rather than fantasy. :-)

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

#1585596 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

From: Andrey Ryabinin <aryabinin@virtuozzo.com>
Date: Tue, 21 Feb 2017 14:27:40 +0300

> DCCP doesn't purge timewait sockets on network namespace shutdown.
> So, after net namespace destroyed we could still have an active timer
> which will trigger use after free in tw_timer_handler():
 ...
> Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
> timewait sockets on net namespace destruction and prevent above issue.
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

Applied and queued up for -stable, thanks.

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

#1585601 — Re: [PATCH] net/dccp: fix use after free in tw_timer_handler()

From: David Miller <davem@davemloft.net>
Date: Tue, 21 Feb 2017 13:23:51 -0500 (EST)

> From: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Date: Tue, 21 Feb 2017 14:27:40 +0300
> 
>> DCCP doesn't purge timewait sockets on network namespace shutdown.
>> So, after net namespace destroyed we could still have an active timer
>> which will trigger use after free in tw_timer_handler():
>  ...
>> Add .exit_batch hook to dccp_v4_ops()/dccp_v6_ops() which will purge
>> timewait sockets on net namespace destruction and prevent above issue.
>> 
>> Reported-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> 
> Applied and queued up for -stable, thanks.

Actually, this doesn't even compile.  Please fix this up and resubmit:

net/dccp/ipv4.c: In function ‘dccp_v4_exit_batch’:
net/dccp/ipv4.c:1022:34: warning: passing argument 2 of ‘inet_twsk_purge’ makes integer from pointer without a cast [-Wint-conversion]
  inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET);
                                  ^
In file included from ./include/linux/dccp.h:14:0,
                 from net/dccp/ipv4.c:13:
./include/net/inet_timewait_sock.h:118:6: note: expected ‘int’ but argument is of type ‘struct inet_timewait_death_row *’
 void inet_twsk_purge(struct inet_hashinfo *hashinfo, int family);
      ^
net/dccp/ipv4.c:1022:2: error: too many arguments to function ‘inet_twsk_purge’
  inet_twsk_purge(&dccp_hashinfo, &dccp_death_row, AF_INET);
  ^

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web