Re: [PATCH -v3] ext4: fix possible UAF when remounting r/o a mmp-protected file system
From: Jan Kara <jack@suse.cz>
Date: 2021-07-06 19:49:12
On Tue 06-07-21 13:12:08, Theodore Ts'o wrote:
After commit 618f003199c6 ("ext4: fix memory leak in
ext4_fill_super"), after the file system is remounted read-only, there
is a race where the kmmpd thread can exit, causing sbi->s_mmp_tsk to
point at freed memory, which the call to ext4_stop_mmpd() can trip
over.
Fix this by only allowing kmmpd() to exit when it is stopped via
ext4_stop_mmpd().
Link: https://lore.kernel.org/r/YONtEGojq7LcXnuC@mit.edu (local)
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>The patch looks mostly fine. Two comments below.
quoted hunk ↗ jump to hunk
@@ -242,9 +237,13 @@ static int kmmpd(void *data) mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN); mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds()); - retval = write_mmp_block(sb, bh); + return write_mmp_block(sb, bh);
I think we need to keep retval = write_mmp_block() here. Otherwise we could exit early in sb_rdonly() case and still have potential use-after-free.
-exit_thread: +wait_to_exit: + set_current_state(TASK_INTERRUPTIBLE); + while (!kthread_should_stop()) + schedule(); + set_current_state(TASK_RUNNING); return retval; }
This is more or less fine but if we get a spurious wakeup for whatever reason (which sets task to TASK_RUNNING state) we would still be potentially looping in that loop burning cpu... Honza -- Jan Kara [off-list ref] SUSE Labs, CR