lkml.org 
[lkml]   [2022]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2022-09-25 15:14    [W:0.043 / U:0.748 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site