Hi, On Fri, Sep 24, 2021 at 9:13 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Tue, Sep 07, 2021 at 08:00:26PM -0700, Andrew Morton wrote: > > 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 > > I don't know where this happened, but the leading whitespace has been > eaten at some point, making this description of the race completely > unreadable as everything appears to be done by CPU 0. The diagram is the same as that in the original patch by author, and I approved it without any discomfort because these five operations (nilfs_put_root() ~ spin_lock(); rb_erase(); spin_unlock() ~ kfree() ) are all done by CPU0. But, yeah, an example of a function call might have been written on the CPU1 side as well, for instance, a nilfs_lookup_root() call etc, to clarify the race issue the message explains.. Regards, Ryusuke Konishi
