Messages in this thread | | | Date | Sun, 25 Sep 2022 15:13:30 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH] kernfs: fix UAF race condition in __kernfs_remove() |
| |
On Sun, Sep 25, 2022 at 09:29:32PM +0900, Tetsuo Handa wrote: > syzbot is reporting use-after-free read at __kernfs_remove() [1], for > commit 35beab0635f3cdd4 ("kernfs: restructure removal path to fix possible > premature return") missed that we need to keep a ref on "kn" as well as > "pos". > > This race condition happens when two concurrent removers "T1" and "T2" > interfere due to kernfs_drain() temporarily dropping kernfs_rwsem. > > T1: T2: > down_write(&root->kernfs_rwsem); > do { > pos = kernfs_leftmost_descendant(kn); > kernfs_get(pos); > kernfs_drain(pos) { > up_write(&root->kernfs_rwsem); > down_write(&root->kernfs_rwsem); > do { > // Removes all children and "kn", but won't > // free T1's "pos" and "kn", for T1 has a ref > // on T1's "pos", and T1's "pos" in turn keeps > // a ref on "kn". > pos = kernfs_leftmost_descendant(kn); > kernfs_put(pos); > } while (pos != kn) // Will break. > up_write(&root->kernfs_rwsem); > down_write(&root->kernfs_rwsem); > } > // Frees "pos" because this was the last ref, and also frees "kn" > // because a ref by "pos" was gone (i.e. "kn" no longer has ref) > // via "goto repeat;" inside kernfs_put(). > kernfs_put(pos); > } while (pos != kn) // Will continue, despite "kn" already freed. > > Link: https://syzkaller.appspot.com/bug?extid=8bee3285b9e190f1509e [1] > Reported-by: syzbot+8bee3285b9e190f1509e@syzkaller.appspotmail.com > Fixes: 35beab0635f3cdd4 ("kernfs: restructure removal path to fix possible premature return") > Tested-by: syzbot+8bee3285b9e190f1509e@syzkaller.appspotmail.com > Co-developed-by: Hillf Danton <hdanton@sina.com> > Signed-off-by: Hillf Danton <hdanton@sina.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > fs/kernfs/dir.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index 1cc88ba6de90..effb461d34fa 100644 > --- a/fs/kernfs/dir.c > +++ b/fs/kernfs/dir.c > @@ -1365,6 +1365,11 @@ static void __kernfs_remove(struct kernfs_node *kn) > atomic_add(KN_DEACTIVATED_BIAS, &pos->active); > > /* deactivate and unlink the subtree node-by-node */ > + /* > + * kernfs_put(pos) will invoke kernfs_put(kn) if @pos was the last > + * reference to @kn. Make sure @kn doesn't go away underneath us. > + */ > + kernfs_get(kn); > do { > pos = kernfs_leftmost_descendant(kn); > > @@ -1406,6 +1411,7 @@ static void __kernfs_remove(struct kernfs_node *kn) > > kernfs_put(pos); > } while (pos != kn); > + kernfs_put(kn); > } > > /** > -- > 2.34.1 >
Isn't this already handled by: https://lore.kernel.org/r/20220913121723.691454-1-lk@c--e.de
that will show up in the next linux-next tree.
thanks,
greg k-h
| |