Re: [PATCH] kernfs: fix UAF race condition in __kernfs_remove()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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