Message-ID: <CACT4Y+YH92M_X7h21W-unhp73tXV+itxuL0JjrfEd4B4Lohjig@mail.gmail.com>
Date:   Tue, 2 Oct 2018 10:19:06 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Thomas Graf <tgraf@...g.ch>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH v2 net] inet: frags: rework rhashtable dismantle

On Tue, Oct 2, 2018 at 7:49 AM, Eric Dumazet <edumazet@...gle.com> wrote:
> syszbot found an interesting use-after-free [1] happening
> while IPv4 fragment rhashtable was destroyed at netns dismantle.
>
> While no insertions can possibly happen at the time a dismantling
> netns is destroying this rhashtable, timers can still fire and
> attempt to remove elements from this rhashtable.
>
> This is forbidden, since rhashtable_free_and_destroy() has
> no synchronization against concurrent inserts and deletes.
>
> Add a new nf->dead flag so that timers do not attempt
> a rhashtable_remove_fast() operation.
>
> [1]
> BUG: KASAN: use-after-free in __read_once_size include/linux/compiler.h:188 [inline]
> BUG: KASAN: use-after-free in rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
> Read of size 8 at addr ffff88019a4c8840 by task kworker/0:4/8279
>
> CPU: 0 PID: 8279 Comm: kworker/0:4 Not tainted 4.19.0-rc5+ #61
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events rht_deferred_worker
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
>  print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256
>  kasan_report_error mm/kasan/report.c:354 [inline]
>  kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412
>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
>  __read_once_size include/linux/compiler.h:188 [inline]
>  rhashtable_last_table+0x216/0x240 lib/rhashtable.c:217
>  rht_deferred_worker+0x157/0x1de0 lib/rhashtable.c:410
>  process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
>  worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
>  kthread+0x35a/0x420 kernel/kthread.c:246
>  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
>
> Allocated by task 5:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>  set_track mm/kasan/kasan.c:460 [inline]
>  kasan_kmalloc+0xc7/0xe0 mm/kasan/kasan.c:553
>  __do_kmalloc_node mm/slab.c:3682 [inline]
>  __kmalloc_node+0x47/0x70 mm/slab.c:3689
>  kmalloc_node include/linux/slab.h:555 [inline]
>  kvmalloc_node+0xb9/0xf0 mm/util.c:423
>  kvmalloc include/linux/mm.h:577 [inline]
>  kvzalloc include/linux/mm.h:585 [inline]
>  bucket_table_alloc+0x9a/0x4e0 lib/rhashtable.c:176
>  rhashtable_rehash_alloc+0x73/0x100 lib/rhashtable.c:353
>  rht_deferred_worker+0x278/0x1de0 lib/rhashtable.c:413
>  process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
>  worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
>  kthread+0x35a/0x420 kernel/kthread.c:246
>  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
>
> Freed by task 8283:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>  set_track mm/kasan/kasan.c:460 [inline]
>  __kasan_slab_free+0x102/0x150 mm/kasan/kasan.c:521
>  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
>  __cache_free mm/slab.c:3498 [inline]
>  kfree+0xcf/0x230 mm/slab.c:3813
>  kvfree+0x61/0x70 mm/util.c:452
>  bucket_table_free+0xda/0x250 lib/rhashtable.c:108
>  rhashtable_free_and_destroy+0x152/0x900 lib/rhashtable.c:1163
>  inet_frags_exit_net+0x3d/0x50 net/ipv4/inet_fragment.c:96
>  ipv4_frags_exit_net+0x73/0x90 net/ipv4/ip_fragment.c:914
>  ops_exit_list.isra.7+0xb0/0x160 net/core/net_namespace.c:153
>  cleanup_net+0x555/0xb10 net/core/net_namespace.c:551
>  process_one_work+0xc90/0x1b90 kernel/workqueue.c:2153
>  worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
>  kthread+0x35a/0x420 kernel/kthread.c:246
>  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
>
> The buggy address belongs to the object at ffff88019a4c8800
>  which belongs to the cache kmalloc-16384 of size 16384
> The buggy address is located 64 bytes inside of
>  16384-byte region [ffff88019a4c8800, ffff88019a4cc800)
> The buggy address belongs to the page:
> page:ffffea0006693200 count:1 mapcount:0 mapping:ffff8801da802200 index:0x0 compound_mapcount: 0
> flags: 0x2fffc0000008100(slab|head)
> raw: 02fffc0000008100 ffffea0006685608 ffffea0006617c08 ffff8801da802200
> raw: 0000000000000000 ffff88019a4c8800 0000000100000001 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
>  ffff88019a4c8700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>  ffff88019a4c8780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>>ffff88019a4c8800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                            ^
>  ffff88019a4c8880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  ffff88019a4c8900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>
> Fixes: 648700f76b03 ("inet: frags: use rhashtables for reassembly units")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: Thomas Graf <tgraf@...g.ch>
> Cc: Herbert Xu <herbert@...dor.apana.org.au>
> ---
>  include/net/inet_frag.h  |  4 +++-
>  net/ipv4/inet_fragment.c | 31 +++++++++++++++++++++----------
>  2 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/inet_frag.h b/include/net/inet_frag.h
> index 1662cbc0b46b45296a367ecbdaf03c68854fdce7..ffe5e1be40212fa63e360f3e29a56c1b2ce897ee 100644
> --- a/include/net/inet_frag.h
> +++ b/include/net/inet_frag.h
> @@ -11,7 +11,7 @@ struct netns_frags {
>         int                     timeout;
>         int                     max_dist;
>         struct inet_frags       *f;
> -
> +       bool                    dead;
>         struct rhashtable       rhashtable ____cacheline_aligned_in_smp;
>
>         /* Keep atomic mem on separate cachelines in structs that include it */
> @@ -24,11 +24,13 @@ struct netns_frags {
>   * @INET_FRAG_FIRST_IN: first fragment has arrived
>   * @INET_FRAG_LAST_IN: final fragment has arrived
>   * @INET_FRAG_COMPLETE: frag queue has been processed and is due for destruction
> + * @INET_FRAG_HASH_DEAD: inet_frag_kill() has not removed fq from rhashtable
>   */
>  enum {
>         INET_FRAG_FIRST_IN      = BIT(0),
>         INET_FRAG_LAST_IN       = BIT(1),
>         INET_FRAG_COMPLETE      = BIT(2),
> +       INET_FRAG_HASH_DEAD     = BIT(3),
>  };
>
>  struct frag_v4_compare_key {
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index bcb11f3a27c0c34115af05034a5a20f57842eb0a..887134425350cc7c8ee081d8c83a037074d96d93 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -71,28 +71,31 @@ EXPORT_SYMBOL(inet_frags_fini);
>  static void inet_frags_free_cb(void *ptr, void *arg)
>  {
>         struct inet_frag_queue *fq = ptr;
> +       int count = 0;
>
> -       /* If we can not cancel the timer, it means this frag_queue
> -        * is already disappearing, we have nothing to do.
> -        * Otherwise, we own a refcount until the end of this function.
> -        */
> -       if (!del_timer(&fq->timer))
> -               return;
> +       if (del_timer_sync(&fq->timer))
> +               count++;
>
>         spin_lock_bh(&fq->lock);
>         if (!(fq->flags & INET_FRAG_COMPLETE)) {
>                 fq->flags |= INET_FRAG_COMPLETE;
> -               refcount_dec(&fq->refcnt);
> +               count++;
> +       } else if (fq->flags & INET_FRAG_HASH_DEAD) {
> +               count++;
>         }
>         spin_unlock_bh(&fq->lock);
>
> -       inet_frag_put(fq);
> +       if (refcount_sub_and_test(count, &fq->refcnt))
> +               inet_frag_destroy(fq);
>  }
>
>  void inet_frags_exit_net(struct netns_frags *nf)
>  {
>         nf->high_thresh = 0; /* prevent creation of new frags */
>
> +       /* paired with READ_ONCE() in inet_frag_kill() */
> +       smp_store_release(&nf->dead, true);
> +
>         rhashtable_free_and_destroy(&nf->rhashtable, inet_frags_free_cb, NULL);
>  }
>  EXPORT_SYMBOL(inet_frags_exit_net);
> @@ -106,8 +109,16 @@ void inet_frag_kill(struct inet_frag_queue *fq)
>                 struct netns_frags *nf = fq->net;
>
>                 fq->flags |= INET_FRAG_COMPLETE;
> -               rhashtable_remove_fast(&nf->rhashtable, &fq->node, nf->f->rhash_params);
> -               refcount_dec(&fq->refcnt);
> +               /* This READ_ONCE() is paired with smp_store_release()
> +                * in inet_frags_exit_net().
> +                */
> +               if (!READ_ONCE(nf->dead)) {


Does inet_frag_kill() hold fq->lock? I am missing how inet_frag_kill()
and inet_frags_exit_net() are synchronized.
Since you use smp_store_release()/READ_ONCE() they seem to run in
parallel. But then isn't it possible that inet_frag_kill() reads
nf->dead == 0, then inet_frags_exit_net() sets nf->dead, and then we
have the same race on concurrent removal? Or, isn't it possible that
inet_frag_kill() reads nf->dead == 1, but does not set
INET_FRAG_HASH_DEAD yet, and then inet_frags_free_cb() misses the
INET_FRAG_HASH_DEAD flag?


> +                       rhashtable_remove_fast(&nf->rhashtable, &fq->node,
> +                                              nf->f->rhash_params);
> +                       refcount_dec(&fq->refcnt);
> +               } else {
> +                       fq->flags |= INET_FRAG_HASH_DEAD;
> +               }
>         }
>  }
>  EXPORT_SYMBOL(inet_frag_kill);
> --
> 2.19.0.605.g01d371f741-goog
>

Powered by blists - more mailing lists