Hi!
> From: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> Subject: nilfs2: use refcount_dec_and_lock() to fix potential UAF
>
> 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
> There is no reproduction program, and the above is only theoretical
> analysis.
Ok, so we have a theoretical bug, and fix already on its way to
stable. But ... is it correct?
> +++ a/fs/nilfs2/the_nilfs.c
> @@ -792,14 +792,13 @@ nilfs_find_or_create_root(struct the_nil
>
> 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);
spin_lock() is deleted, but spin_unlock() is not affected. This means
unbalanced locking, right?
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Attachment:
signature.asc
Description: PGP signature
