[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@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzbot.org/bug?extid=ae2d1f6d474e1725bdc0
Signed-off-by: Yiqing Wu <wuyiqing@xxxxxxxxxxxxxxxxxxxx>
---
 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