[PATCH] jffs2: fix use-after-free in jffs2_garbage_collect_thread

[PATCH] jffs2: fix use-after-free in jffs2_garbage_collect_thread

[Yiqing Wu]

jffs2_do_remount_fs() decides whether to (re)start the garbage collect
thread from the requested flags:

	if (!(fc->sb_flags & SB_RDONLY))
		jffs2_start_garbage_collect_thread(c);

The flags which actually take effect, however, are computed by
reconfigure_super() as

	sb->s_flags = (sb->s_flags & ~fc->sb_flags_mask) |
		      (fc->sb_flags & fc->sb_flags_mask);

and fspick(2) creates its reconfiguration context with both sb_flags
and sb_flags_mask set to zero, i.e. "no change requested".  So after

	fd = fspick(AT_FDCWD, mnt, 0);
	fsconfig(fd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);

on a filesystem which was mounted read-only, jffs2 starts the GC
thread (because fc->sb_flags carries no SB_RDONLY) while the superblock
keeps its SB_RDONLY (because fc->sb_flags_mask is zero).  The classic
mount(2) remount path is not affected: it always passes a full
MS_RMT_MASK, so the flags it hands to the filesystem do take effect.

jffs2_kill_sb() then refuses to stop the GC thread for a read-only
superblock:

	if (c && !sb_rdonly(sb))
		jffs2_stop_garbage_collect_thread(c);
	kill_mtd_super(sb);
	kfree(c);

so at unmount the thread is neither signalled nor waited for, and
kfree(c) frees the jffs2_sb_info out from under the still-running
jffs2_gcd_mtd%d kthread.  It dereferences the freed object when it
wakes up from its 50 ms idle sleep:

  BUG: KASAN: slab-use-after-free in rt_spin_lock
  Read of size 1 at addr ffff88804f2b02c0 by task jffs2_gcd_mtd0/12364
  Call Trace:
  rt_spin_lock kernel/locking/spinlock_rt.c:56
  spin_lock include/linux/spinlock_rt.h:45 [inline]
  jffs2_garbage_collect_thread fs/jffs2/background.c:94

Fix both layers of the desynchronisation:

  - jffs2_do_remount_fs() now derives the flags which will be in
    effect after the reconfiguration the same way reconfigure_super()
    does, and only starts the GC thread if the filesystem will
    actually be writable.

  - jffs2_kill_sb() unconditionally stops and waits for the GC thread
    before freeing c.  Without a running GC thread this is a no-op,
    and the thread can no longer outlive the jffs2_sb_info, no matter
    which future path manages to start it on a superblock that stays
    read-only.

Fixes: ec10a24f10c8 ("vfs: Convert jffs2 to use the new mount API")
Reported-by: syzbot+ae2d1f6d474e1725bdc0@syzkaller.appspotmail.com
Closes: https://syzbot.org/bug?extid=ae2d1f6d474e1725bdc0
Signed-off-by: Yiqing Wu <wuyiqing@cmss.chinamobile.com>
---
 fs/jffs2/fs.c    | 13 ++++++++++++-
 fs/jffs2/super.c |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 05cf860307c7..a124f5309c2b 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -396,6 +396,7 @@ void jffs2_dirty_inode(struct inode *inode, int flags)
 int jffs2_do_remount_fs(struct super_block *sb, struct fs_context *fc)
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
+	unsigned int sb_flags;
 
 	if (c->flags & JFFS2_SB_FLAG_RO && !sb_rdonly(sb))
 		return -EROFS;
@@ -411,7 +412,17 @@ int jffs2_do_remount_fs(struct super_block *sb, struct fs_context *fc)
 		mutex_unlock(&c->alloc_sem);
 	}
 
-	if (!(fc->sb_flags & SB_RDONLY))
+	/*
+	 * Start the GC thread only if the filesystem will actually be
+	 * writable after the reconfiguration.  Note that reconfigure_super()
+	 * applies the flags as
+	 * (sb->s_flags & ~fc->sb_flags_mask) | (fc->sb_flags & fc->sb_flags_mask),
+	 * so a zero sb_flags_mask (as e.g. used by fspick()) leaves the
+	 * superblock read-only.
+	 */
+	sb_flags = (sb->s_flags & ~fc->sb_flags_mask) |
+		   (fc->sb_flags & fc->sb_flags_mask);
+	if (!(sb_flags & SB_RDONLY))
 		jffs2_start_garbage_collect_thread(c);
 
 	fc->sb_flags |= SB_NOATIME;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 81396a092ba8..c846b435a38b 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -345,7 +345,7 @@ static void jffs2_put_super (struct super_block *sb)
 static void jffs2_kill_sb(struct super_block *sb)
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
-	if (c && !sb_rdonly(sb))
+	if (c)
 		jffs2_stop_garbage_collect_thread(c);
 	kill_mtd_super(sb);
 	kfree(c);
-- 
2.56.0




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] jffs2: fix use-after-free in jffs2_garbage_collect_thread

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] jffs2: uninitialized spinlock in jffs2_kill_sb() on rp_size mount error
--

commit 8866980c81747d68f6037e251702023d6140fd9d
Author: Yiqing Wu <wuyiqing@cmss.chinamobile.com>

jffs2: fix use-after-free in jffs2_garbage_collect_thread

This commit fixes a use-after-free of struct jffs2_sb_info by computing the
effective superblock flags in jffs2_do_remount_fs() before starting the
garbage collection thread, and by unconditionally calling
jffs2_stop_garbage_collect_thread() in jffs2_kill_sb().

[ ... ]

> diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
> index 81396a092ba88..c846b435a38b6 100644
> --- a/fs/jffs2/super.c
> +++ b/fs/jffs2/super.c
> @@ -345,7 +345,7 @@ static void jffs2_put_super (struct super_block *sb)
>  static void jffs2_kill_sb(struct super_block *sb)
>  {
>  	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
> -	if (c && !sb_rdonly(sb))
> +	if (c)
>  		jffs2_stop_garbage_collect_thread(c);

[Severity: High]
Can calling jffs2_stop_garbage_collect_thread(c) unconditionally here lock an
uninitialized c->erase_completion_lock when a mount fails early in
jffs2_fill_super()?

During mount context initialization, jffs2_init_fs_context() in
fs/jffs2/super.c allocates struct jffs2_sb_info and assigns it to
fc->s_fs_info without initializing c->erase_completion_lock. When
get_tree_mtd()->mtd_get_sb()->sget_dev()->sget_fc() creates the superblock,
fc->s_fs_info is transferred to sb->s_fs_info before mtd_get_sb() calls
jffs2_fill_super(sb, fc).

In fs/jffs2/super.c:jffs2_fill_super():

	if (c->mount_opts.rp_size > c->mtd->size)
		return invalf(fc, "jffs2: Too large reserve pool specified, max is %llu KB",
			      c->mtd->size / 1024);

	/* Initialize JFFS2 superblock locks, the further initialization will
	 * be done later */
	mutex_init(&c->alloc_sem);
	mutex_init(&c->erase_free_sem);
	init_waitqueue_head(&c->erase_wait);
	init_waitqueue_head(&c->inocache_wq);
	spin_lock_init(&c->erase_completion_lock);

If a mount specifies an rp_size larger than c->mtd->size, jffs2_fill_super()
returns -EINVAL before spin_lock_init(&c->erase_completion_lock) is called.
mtd_get_sb() then calls deactivate_locked_super(sb)->jffs2_kill_sb(sb), where
c is non-NULL.

Previously, failed read-only mounts skipped jffs2_stop_garbage_collect_thread()
because sb_rdonly(sb) was true (though failed read-write mounts already hit
this path). With the !sb_rdonly(sb) check removed, any mount failing at the
rp_size check now calls jffs2_stop_garbage_collect_thread(c) in
fs/jffs2/background.c:

void jffs2_stop_garbage_collect_thread(struct jffs2_sb_info *c)
{
	int wait = 0;
	spin_lock(&c->erase_completion_lock);

Should c->erase_completion_lock be initialized earlier (for example, in
jffs2_init_fs_context() or before the rp_size check in jffs2_fill_super()) so
jffs2_kill_sb() can safely call jffs2_stop_garbage_collect_thread(c) on early
mount failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103090018.157034-1-wuyiqing@cmss.chinamobile.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2] jffs2: fix use-after-free in jffs2_garbage_collect_thread

[Yiqing Wu]

jffs2_do_remount_fs() decides whether to (re)start the garbage collect
thread from the requested flags:

	if (!(fc->sb_flags & SB_RDONLY))
		jffs2_start_garbage_collect_thread(c);

The flags which actually take effect, however, are computed by
reconfigure_super() as

	sb->s_flags = (sb->s_flags & ~fc->sb_flags_mask) |
		      (fc->sb_flags & fc->sb_flags_mask);

and fspick(2) creates its reconfiguration context with both sb_flags
and sb_flags_mask set to zero, i.e. "no change requested".  So after

	fd = fspick(AT_FDCWD, mnt, 0);
	fsconfig(fd, FSCONFIG_CMD_RECONFIGURE, NULL, NULL, 0);

on a filesystem which was mounted read-only, jffs2 starts the GC
thread (because fc->sb_flags carries no SB_RDONLY) while the superblock
keeps its SB_RDONLY (because fc->sb_flags_mask is zero).  The classic
mount(2) remount path is not affected: it always passes a full
MS_RMT_MASK, so the flags it hands to the filesystem do take effect.

jffs2_kill_sb() then refuses to stop the GC thread for a read-only
superblock:

	if (c && !sb_rdonly(sb))
		jffs2_stop_garbage_collect_thread(c);
	kill_mtd_super(sb);
	kfree(c);

so at unmount the thread is neither signalled nor waited for, and
kfree(c) frees the jffs2_sb_info out from under the still-running
jffs2_gcd_mtd%d kthread.  It dereferences the freed object when it
wakes up from its 50 ms idle sleep:

  BUG: KASAN: slab-use-after-free in rt_spin_lock
  Read of size 1 at addr ffff88804f2b02c0 by task jffs2_gcd_mtd0/12364
  Call Trace:
  rt_spin_lock kernel/locking/spinlock_rt.c:56
  spin_lock include/linux/spinlock_rt.h:45 [inline]
  jffs2_garbage_collect_thread fs/jffs2/background.c:94

Fix the desynchronisation, and harden the unmount path against it:

  - jffs2_do_remount_fs() now derives the flags which will be in
    effect after the reconfiguration the same way reconfigure_super()
    does, and only starts the GC thread if the filesystem will
    actually be writable.

  - jffs2_kill_sb() unconditionally stops and waits for the GC thread
    before freeing c.  Without a running GC thread this is a no-op,
    and the thread can no longer outlive the jffs2_sb_info, no matter
    which future path manages to start it on a superblock that stays
    read-only.

Fixes: ec10a24f10c8 ("vfs: Convert jffs2 to use the new mount API")
Reported-by: syzbot+ae2d1f6d474e1725bdc0@syzkaller.appspotmail.com
Closes: https://syzbot.org/bug?extid=ae2d1f6d474e1725bdc0
Signed-off-by: Yiqing Wu <wuyiqing@cmss.chinamobile.com>
---
v2:
  - jffs2_fill_super() initialises the superblock locks before the
    rp_size sanity check can fail the mount, so that the now
    unconditional jffs2_stop_garbage_collect_thread() does not take an
    uninitialised c->erase_completion_lock when a mount bails out
    early.  (Read-write mounts with an oversized rp_size already
    reached that path even before this change.)
  - v1: https://lore.kernel.org/all/20261103090018.157034-1-wuyiqing@cmss.chinamobile.com/T/
---
 fs/jffs2/fs.c    | 13 ++++++++++++-
 fs/jffs2/super.c | 16 ++++++++++------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 05cf860307c7..a124f5309c2b 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -396,6 +396,7 @@ void jffs2_dirty_inode(struct inode *inode, int flags)
 int jffs2_do_remount_fs(struct super_block *sb, struct fs_context *fc)
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
+	unsigned int sb_flags;
 
 	if (c->flags & JFFS2_SB_FLAG_RO && !sb_rdonly(sb))
 		return -EROFS;
@@ -411,7 +412,17 @@ int jffs2_do_remount_fs(struct super_block *sb, struct fs_context *fc)
 		mutex_unlock(&c->alloc_sem);
 	}
 
-	if (!(fc->sb_flags & SB_RDONLY))
+	/*
+	 * Start the GC thread only if the filesystem will actually be
+	 * writable after the reconfiguration.  Note that reconfigure_super()
+	 * applies the flags as
+	 * (sb->s_flags & ~fc->sb_flags_mask) | (fc->sb_flags & fc->sb_flags_mask),
+	 * so a zero sb_flags_mask (as e.g. used by fspick()) leaves the
+	 * superblock read-only.
+	 */
+	sb_flags = (sb->s_flags & ~fc->sb_flags_mask) |
+		   (fc->sb_flags & fc->sb_flags_mask);
+	if (!(sb_flags & SB_RDONLY))
 		jffs2_start_garbage_collect_thread(c);
 
 	fc->sb_flags |= SB_NOATIME;
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 81396a092ba8..9ac22b6479f7 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -267,12 +267,12 @@ static int jffs2_fill_super(struct super_block *sb, struct fs_context *fc)
 	c->mtd = sb->s_mtd;
 	c->os_priv = sb;
 
-	if (c->mount_opts.rp_size > c->mtd->size)
-		return invalf(fc, "jffs2: Too large reserve pool specified, max is %llu KB",
-			      c->mtd->size / 1024);
-
 	/* Initialize JFFS2 superblock locks, the further initialization will
-	 * be done later */
+	 * be done later.  This must precede any error return: kill_sb() stops
+	 * the GC thread even on mounts which failed before it could start,
+	 * and jffs2_stop_garbage_collect_thread() takes
+	 * c->erase_completion_lock.
+	 */
 	mutex_init(&c->alloc_sem);
 	mutex_init(&c->erase_free_sem);
 	init_waitqueue_head(&c->erase_wait);
@@ -280,6 +280,10 @@ static int jffs2_fill_super(struct super_block *sb, struct fs_context *fc)
 	spin_lock_init(&c->erase_completion_lock);
 	spin_lock_init(&c->inocache_lock);
 
+	if (c->mount_opts.rp_size > c->mtd->size)
+		return invalf(fc, "jffs2: Too large reserve pool specified, max is %llu KB",
+			      c->mtd->size / 1024);
+
 	sb->s_op = &jffs2_super_operations;
 	sb->s_export_op = &jffs2_export_ops;
 	sb->s_flags = sb->s_flags | SB_NOATIME;
@@ -345,7 +349,7 @@ static void jffs2_put_super (struct super_block *sb)
 static void jffs2_kill_sb(struct super_block *sb)
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
-	if (c && !sb_rdonly(sb))
+	if (c)
 		jffs2_stop_garbage_collect_thread(c);
 	kill_mtd_super(sb);
 	kfree(c);
-- 
2.56.0




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/