fs/kernfs/dir.c | 6 ++++++ 1 file changed, 6 insertions(+)
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.1On 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
On 2022/09/25 22:13, Greg Kroah-Hartman wrote: > 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. Oh, I didn't know that patch. But is that patch complete, for there are three __kernfs_remove() callers?
On Sun, Sep 25, 2022 at 10:20:27PM +0900, Tetsuo Handa wrote: > On 2022/09/25 22:13, Greg Kroah-Hartman wrote: > > 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. > > Oh, I didn't know that patch. > > But is that patch complete, for there are three __kernfs_remove() callers? > syzbot seems to think it works :)
On 2022/09/25 22:40, Greg Kroah-Hartman wrote: > On Sun, Sep 25, 2022 at 10:20:27PM +0900, Tetsuo Handa wrote: >> On 2022/09/25 22:13, Greg Kroah-Hartman wrote: >>> 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. >> >> Oh, I didn't know that patch. >> >> But is that patch complete, for there are three __kernfs_remove() callers? >> > > syzbot seems to think it works :) syzbot's reproducer tested only kernfs_remove_by_name_ns() case. I'm not sure whether e.g. __kernfs_remove() from kernfs_remove() is safe.
Hi,
On Sun, Sep 25, 2022 at 10:52:56PM +0900, Tetsuo Handa wrote:
> On 2022/09/25 22:40, Greg Kroah-Hartman wrote:
> > On Sun, Sep 25, 2022 at 10:20:27PM +0900, Tetsuo Handa wrote:
> >> On 2022/09/25 22:13, Greg Kroah-Hartman wrote:
> >>> 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.
> >>
> >> Oh, I didn't know that patch.
> >>
> >> But is that patch complete, for there are three __kernfs_remove() callers?
> >>
> >
> > syzbot seems to think it works :)
>
> syzbot's reproducer tested only kernfs_remove_by_name_ns() case.
> I'm not sure whether e.g. __kernfs_remove() from kernfs_remove() is safe.
I had an older version of the patch that was rejected by Tejun Heo
on the grounds that external kernfs_remove callers must hold a reference
on their own or the race can happen even befor kernfs_remoe takes the
lock.
See https://lore.kernel.org/all/20220907200811.654034-1-lk@c--e.de/
for the details. I did convince myself that other callers of
kernfs_remove() have other means to ensure that there are no parallel
removes for the same node.
IMHO the kernfs interface's use of ref-counts is slightly unintuitive
but I think it is safe, now.
regards Christian
© 2016 - 2026 Red Hat, Inc.