From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ryusuke Konishi Subject: Re: [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF Date: Fri, 23 Jul 2021 22:30:11 +0900 Message-ID: References: <20210723012317.4146-1-thunder.leizhen@huawei.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=hh2qQyV0e2ZUg+zl/VVNwTimvapqj+B4o1vSa2p3lFE=; b=S7IGk/AW41JjfZBmwuLwKk/gXcjZZJQvKRS3UGBxtrpLuHq98VbkUnb7K8Dp3oCYPb LyFucS1yf5F9ONLvvCZDVvZX+IAr2+nlIa0X9iTnsJ9elFYsgRUYenAG0Nh7q1rb/8/b 3DZ8iQzx5bdK+q/farrljjlJARX0llttk64kfnvLv0bLCVQ6pQHZg/8e7ppX9hvRY7Zq GdgxtzlPgHvYAopLdj/j0NuzJV27Zta47RL/WaDvQdWUrd9Bj3bXKGHixGR/QaIZGdnn Z2JZgXskmhhbr3bLwHogwHfp8hkmm63SPpMXTgYlBgJWC4ZKQg3KCmsFfG0kMbV/YCNk W+zA== In-Reply-To: <20210723012317.4146-1-thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Zhen Lei Cc: linux-nilfs , linux-kernel Hi, On Fri, Jul 23, 2021 at 10:28 AM Zhen Lei wrote: > > When the refcount is decreased to 0, the resource reclamation branch is > entered. Before CPU0 reaches the race point (1), CPU1 may obtain the > spinlock and traverse the rbtree to find 'root', see nilfs_lookup_root(). > Although CPU1 will call refcount_inc() to increase the refcount, it is > obviously too late. CPU0 will release 'root' directly, CPU1 then accesses > 'root' and triggers UAF. > > Use refcount_dec_and_lock() to ensure that both the operations of decrease > refcount to 0 and link deletion are lock protected eliminates this risk. > > CPU0 CPU1 > nilfs_put_root(): > <-------- (1) > spin_lock(&nilfs->ns_cptree_lock); > rb_erase(&root->rb_node, &nilfs->ns_cptree); > spin_unlock(&nilfs->ns_cptree_lock); > > kfree(root); > <-------- use-after-free > > ======================================================================== > refcount_t: underflow; use-after-free. > WARNING: CPU: 2 PID: 9476 at lib/refcount.c:28 \ > refcount_warn_saturate+0x1cf/0x210 lib/refcount.c:28 > Modules linked in: > CPU: 2 PID: 9476 Comm: syz-executor.0 Not tainted 5.10.45-rc1+ #3 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), ... > RIP: 0010:refcount_warn_saturate+0x1cf/0x210 lib/refcount.c:28 > ... ... > Call Trace: > __refcount_sub_and_test include/linux/refcount.h:283 [inline] > __refcount_dec_and_test include/linux/refcount.h:315 [inline] > refcount_dec_and_test include/linux/refcount.h:333 [inline] > nilfs_put_root+0xc1/0xd0 fs/nilfs2/the_nilfs.c:795 > nilfs_segctor_destroy fs/nilfs2/segment.c:2749 [inline] > nilfs_detach_log_writer+0x3fa/0x570 fs/nilfs2/segment.c:2812 > nilfs_put_super+0x2f/0xf0 fs/nilfs2/super.c:467 > generic_shutdown_super+0xcd/0x1f0 fs/super.c:464 > kill_block_super+0x4a/0x90 fs/super.c:1446 > deactivate_locked_super+0x6a/0xb0 fs/super.c:335 > deactivate_super+0x85/0x90 fs/super.c:366 > cleanup_mnt+0x277/0x2e0 fs/namespace.c:1118 > __cleanup_mnt+0x15/0x20 fs/namespace.c:1125 > task_work_run+0x8e/0x110 kernel/task_work.c:151 > tracehook_notify_resume include/linux/tracehook.h:188 [inline] > exit_to_user_mode_loop kernel/entry/common.c:164 [inline] > exit_to_user_mode_prepare+0x13c/0x170 kernel/entry/common.c:191 > syscall_exit_to_user_mode+0x16/0x30 kernel/entry/common.c:266 > do_syscall_64+0x45/0x80 arch/x86/entry/common.c:56 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > There is no reproduction program, and the above is only theoretical > analysis. > > Fixes: ba65ae4729bf ("nilfs2: add checkpoint tree to nilfs object") > Signed-off-by: Zhen Lei Thank you for sending this patch. I understand the purpose of the correction. I feel we need more review for the life cycle management of nilfs_root structure related to kobject. Anyway, I would like to apply this patch itself. Thanks, Ryusuke Konishi > --- > fs/nilfs2/the_nilfs.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c > index 8b7b01a380ce..c8bfc01da5d7 100644 > --- a/fs/nilfs2/the_nilfs.c > +++ b/fs/nilfs2/the_nilfs.c > @@ -792,14 +792,13 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno) > > void nilfs_put_root(struct nilfs_root *root) > { > - if (refcount_dec_and_test(&root->count)) { > - struct the_nilfs *nilfs = root->nilfs; > + struct the_nilfs *nilfs = root->nilfs; > > - nilfs_sysfs_delete_snapshot_group(root); > - > - spin_lock(&nilfs->ns_cptree_lock); > + if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) { > rb_erase(&root->rb_node, &nilfs->ns_cptree); > spin_unlock(&nilfs->ns_cptree_lock); > + > + nilfs_sysfs_delete_snapshot_group(root); > iput(root->ifile); > > kfree(root); > -- > 2.25.1 >