LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF
@ 2021-08-25  2:43 Ryusuke Konishi
  2021-08-25  2:47 ` Ryusuke Konishi
  0 siblings, 1 reply; 4+ messages in thread
From: Ryusuke Konishi @ 2021-08-25  2:43 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-nilfs, LKML

From: Zhen Lei <thunder.leizhen@huawei.com>

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")
Link: https://lkml.kernel.org/r/20210723012317.4146-1-thunder.leizhen@huawei.com
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
---
 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 @@ struct nilfs_root *
 
 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);
-- 
1.8.3.1


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

* Re: [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF
  2021-08-25  2:43 [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF Ryusuke Konishi
@ 2021-08-25  2:47 ` Ryusuke Konishi
  0 siblings, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2021-08-25  2:47 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-nilfs, LKML

Hi Andrew,

Please queue this patch additionally for the next merge window.

Thanks,
Ryusuke Konishi

On Wed, Aug 25, 2021 at 11:43 AM Ryusuke Konishi
<konishi.ryusuke@gmail.com> wrote:
>
> From: Zhen Lei <thunder.leizhen@huawei.com>
>
> 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")
> Link: https://lkml.kernel.org/r/20210723012317.4146-1-thunder.leizhen@huawei.com
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> ---
>  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 @@ struct nilfs_root *
>
>  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);
> --
> 1.8.3.1
>

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

* Re: [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF
  2021-07-23  1:23 Zhen Lei
@ 2021-07-23 13:30 ` Ryusuke Konishi
  0 siblings, 0 replies; 4+ messages in thread
From: Ryusuke Konishi @ 2021-07-23 13:30 UTC (permalink / raw)
  To: Zhen Lei; +Cc: linux-nilfs, linux-kernel

Hi,

On Fri, Jul 23, 2021 at 10:28 AM Zhen Lei <thunder.leizhen@huawei.com> 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 <thunder.leizhen@huawei.com>

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
>

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

* [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF
@ 2021-07-23  1:23 Zhen Lei
  2021-07-23 13:30 ` Ryusuke Konishi
  0 siblings, 1 reply; 4+ messages in thread
From: Zhen Lei @ 2021-07-23  1:23 UTC (permalink / raw)
  To: Ryusuke Konishi, linux-nilfs, linux-kernel; +Cc: Zhen Lei

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 <thunder.leizhen@huawei.com>
---
 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


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

end of thread, other threads:[~2021-08-25  2:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-25  2:43 [PATCH] nilfs2: use refcount_dec_and_lock() to fix potential UAF Ryusuke Konishi
2021-08-25  2:47 ` Ryusuke Konishi
  -- strict thread matches above, loose matches on Subject: below --
2021-07-23  1:23 Zhen Lei
2021-07-23 13:30 ` Ryusuke Konishi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).