| Message ID | tencent_E860EA86EF0ECC0079FA6D3C2D496D30940A@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | jfs: fix uaf in jfs_syncpt | expand |
On Tue, Feb 20, 2024 at 11:55:18AM +0800, Edward Adam Davis wrote: > During the execution of the jfs lazy commit, the jfs file system was unmounted, > causing the sbi and jfs log objects to be released, triggering this issue. > The solution is to add mutex to synchronize jfs lazy commit and jfs unmount > operations. Why is that the solution? LAZY_LOCK with IN_LAZYCOMMIT is supposed to cover this. Please be more verbose in your commit messages. Describe what is going wrong and why; that will allow people to understand why this is the correct solution to the problem.
On Tue, 20 Feb 2024 04:07:09 +0000, Matthew Wilcox wrote: > > During the execution of the jfs lazy commit, the jfs file system was unmounted, > > causing the sbi and jfs log objects to be released, triggering this issue. > > The solution is to add mutex to synchronize jfs lazy commit and jfs unmount > > operations. > > Why is that the solution? LAZY_LOCK with IN_LAZYCOMMIT is supposed to LAZY_LOCK not cover jfs umount. > cover this. Please be more verbose in your commit messages. Describe > what is going wrong and why; that will allow people to understand why > this is the correct solution to the problem. [Syz reported] BUG: KASAN: slab-use-after-free in __mutex_waiter_is_first kernel/locking/mutex.c:197 [inline] BUG: KASAN: slab-use-after-free in __mutex_lock_common kernel/locking/mutex.c:686 [inline] BUG: KASAN: slab-use-after-free in __mutex_lock+0x8f4/0x9d0 kernel/locking/mutex.c:752 Read of size 8 at addr ffff8880272d2908 by task jfsCommit/131 CPU: 3 PID: 131 Comm: jfsCommit Not tainted 6.8.0-rc4-syzkaller-00388-gced590523156 #0 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106 print_address_description mm/kasan/report.c:377 [inline] print_report+0xc4/0x620 mm/kasan/report.c:488 kasan_report+0xda/0x110 mm/kasan/report.c:601 __mutex_waiter_is_first kernel/locking/mutex.c:197 [inline] __mutex_lock_common kernel/locking/mutex.c:686 [inline] __mutex_lock+0x8f4/0x9d0 kernel/locking/mutex.c:752 jfs_syncpt+0x2a/0xa0 fs/jfs/jfs_logmgr.c:1039 txEnd+0x30d/0x5a0 fs/jfs/jfs_txnmgr.c:549 txLazyCommit fs/jfs/jfs_txnmgr.c:2684 [inline] jfs_lazycommit+0x77d/0xb20 fs/jfs/jfs_txnmgr.c:2733 kthread+0x2c6/0x3b0 kernel/kthread.c:388 ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:242 Freed by task 5177: kasan_save_stack+0x33/0x60 mm/kasan/common.c:47 kasan_save_track+0x14/0x30 mm/kasan/common.c:68 kasan_save_free_info+0x3f/0x60 mm/kasan/generic.c:640 poison_slab_object mm/kasan/common.c:241 [inline] __kasan_slab_free+0x121/0x1c0 mm/kasan/common.c:257 kasan_slab_free include/linux/kasan.h:184 [inline] slab_free_hook mm/slub.c:2121 [inline] slab_free mm/slub.c:4299 [inline] kfree+0x124/0x370 mm/slub.c:4409 lmLogClose+0x585/0x710 fs/jfs/jfs_logmgr.c:1461 jfs_umount+0x2f0/0x440 fs/jfs/jfs_umount.c:114 jfs_put_super+0x88/0x1d0 fs/jfs/super.c:194 [Analyze] This issue occurs due to task 131 executing jfs lazy commit and task 5177 executing jfs put super (which will release objects such as sbi and jfs log). The solution is to use mutex to sort the two tasks and determine whether the log and sbi objects are valid before using them. This way, regardless of who executes the two tasks first, the latter can determine whether the log and sbi objects are valid or invalid, thus avoiding the current problem.
diff --git a/fs/jfs/jfs_incore.h b/fs/jfs/jfs_incore.h index dd4264aa9bed..15955dd86bfd 100644 --- a/fs/jfs/jfs_incore.h +++ b/fs/jfs/jfs_incore.h @@ -197,6 +197,7 @@ struct jfs_sb_info { kgid_t gid; /* gid to override on-disk gid */ uint umask; /* umask to override on-disk umask */ uint minblks_trim; /* minimum blocks, for online trim */ + struct mutex log_mutex; }; /* jfs_sb_info commit_state */ diff --git a/fs/jfs/jfs_txnmgr.c b/fs/jfs/jfs_txnmgr.c index be17e3c43582..eb60862dc61b 100644 --- a/fs/jfs/jfs_txnmgr.c +++ b/fs/jfs/jfs_txnmgr.c @@ -2665,6 +2665,9 @@ static void txLazyCommit(struct tblock * tblk) log = (struct jfs_log *) JFS_SBI(tblk->sb)->log; + if (!log) + return; + spin_lock_irq(&log->gclock); // LOGGC_LOCK tblk->flag |= tblkGC_COMMITTED; @@ -2730,10 +2733,12 @@ int jfs_lazycommit(void *arg) list_del(&tblk->cqueue); LAZY_UNLOCK(flags); + mutex_lock(&sbi->log_mutex); txLazyCommit(tblk); + sbi->commit_state &= ~IN_LAZYCOMMIT; + mutex_unlock(&sbi->log_mutex); LAZY_LOCK(flags); - sbi->commit_state &= ~IN_LAZYCOMMIT; /* * Don't continue in the for loop. (We can't * anyway, it's unsafe!) We want to go back to diff --git a/fs/jfs/jfs_umount.c b/fs/jfs/jfs_umount.c index 8ec43f53f686..04788cf3a471 100644 --- a/fs/jfs/jfs_umount.c +++ b/fs/jfs/jfs_umount.c @@ -51,6 +51,7 @@ int jfs_umount(struct super_block *sb) * * if mounted read-write and log based recovery was enabled */ + mutex_lock(&sbi->log_mutex); if ((log = sbi->log)) /* * Wait for outstanding transactions to be written to log: @@ -113,6 +114,7 @@ int jfs_umount(struct super_block *sb) */ rc = lmLogClose(sb); } + mutex_unlock(&sbi->log_mutex); jfs_info("UnMount JFS Complete: rc = %d", rc); return rc; } diff --git a/fs/jfs/super.c b/fs/jfs/super.c index 8d8e556bd610..cf291bdd094f 100644 --- a/fs/jfs/super.c +++ b/fs/jfs/super.c @@ -504,6 +504,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent) sbi->uid = INVALID_UID; sbi->gid = INVALID_GID; sbi->umask = -1; + mutex_init(&sbi->log_mutex); /* initialize the mount flag and determine the default error handler */ flag = JFS_ERR_REMOUNT_RO;
During the execution of the jfs lazy commit, the jfs file system was unmounted, causing the sbi and jfs log objects to be released, triggering this issue. The solution is to add mutex to synchronize jfs lazy commit and jfs unmount operations. Reported-and-tested-by: syzbot+c244f4a09ca85dd2ebc1@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/jfs/jfs_incore.h | 1 + fs/jfs/jfs_txnmgr.c | 7 ++++++- fs/jfs/jfs_umount.c | 2 ++ fs/jfs/super.c | 1 + 4 files changed, 10 insertions(+), 1 deletion(-)