[PATCH 0/2] Fix use-after-free about sbi->s_mmp_tsk

Ye Bin (2):
  ext4: Fix use-after-free about sbi->s_mmp_tsk
  ext4: Fix potential uas-after-free about sbi->s_mmp_tsk when kmmpd
    kthread exit before set sbi->s_mmp_tsk

 fs/ext4/ext4.h  |  1 +
 fs/ext4/mmp.c   | 34 ++++++++++++++++++++++++++++------
 fs/ext4/super.c |  1 +
 3 files changed, 30 insertions(+), 6 deletions(-)

-- 
2.31.1

Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
already running and even exit as exception. Even though we set
sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
"sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
   mount                     kmmpd
     |                         |
     |-call kthread_run        |
     |                         |-kmmpd runing
     |                         |-kmmpd exit sbi->s_mmp_tsk=NULL
     |                         |
     |-kthread_run return      |
     | and set sbi->s_mmp_tsk  |
     |                         |
     |-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF

This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
NULL. We can create kthread first, and then wakeup kmmpd kthread later.

Signed-off-by: Ye Bin <redacted>
---
 fs/ext4/mmp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index fc18a8c205c7..6ec1ea182cc0 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -394,16 +394,18 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	/*
 	 * Start a kernel thread to update the MMP block periodically.
 	 */
-	EXT4_SB(sb)->s_mmp_tsk = kthread_run(kmmpd, sb, "kmmpd-%.*s",
-					     (int)sizeof(mmp->mmp_bdevname),
-					     bdevname(bh->b_bdev,
-						      mmp->mmp_bdevname));
+	EXT4_SB(sb)->s_mmp_tsk = kthread_create(kmmpd, sb, "kmmpd-%.*s",
+						(int)sizeof(mmp->mmp_bdevname),
+						bdevname(bh->b_bdev,
+							 mmp->mmp_bdevname));
+
 	if (IS_ERR(EXT4_SB(sb)->s_mmp_tsk)) {
 		EXT4_SB(sb)->s_mmp_tsk = NULL;
 		ext4_warning(sb, "Unable to create kmmpd thread for %s.",
 			     sb->s_id);
 		goto failed;
 	}
+	wake_up_process(EXT4_SB(sb)->s_mmp_tsk);
 
 	return 0;
 

-- 
2.31.1

After merge 618f003199c6("ext4: fix memory leak in ext4_fill_super") commit,
we add delay in ext4_remount after "sb->s_flags |= SB_RDONLY", then
remount filesystem with read-only kasan report following warning:
[  888.695326] ==================================================================
[  888.696566] BUG: KASAN: use-after-free in kthread_stop+0x4c/0x2f0
[  888.697599] Write of size 4 at addr ffff8883849e0020 by task mount/2013
[  888.698707]
[  888.698982] CPU: 4 PID: 2013 Comm: mount Not tainted 4.19.95-00013-ga369a6189bbf-dirty #413
[  888.700376] Hardware name: QEMU Standard PC
[  888.702587] Call Trace:
[  888.703017]  dump_stack+0x108/0x15f
[  888.703615]  print_address_description+0xa5/0x372
[  888.704420]  kasan_report.cold+0x236/0x2a8
[  888.705761]  check_memory_region+0x240/0x270
[  888.706486]  kasan_check_write+0x20/0x30
[  888.707156]  kthread_stop+0x4c/0x2f0
[  888.707776]  ext4_stop_mmpd+0x32/0x90
[  888.708262]  ext4_remount.cold+0xf6/0x116
[  888.712671]  do_remount_sb+0xff/0x460
[  888.714007]  do_mount+0xce3/0x1be0
[  888.717749]  ksys_mount+0xb2/0x150
[  888.718163]  __x64_sys_mount+0x6a/0x80
[  888.718607]  do_syscall_64+0xd9/0x1f0
[  888.719047]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

As kmmpd will exit if filesystem is read-only. Then sbi->s_mmp_tsk become wild
ptr, lead to use-after-free. As kmmpd will exit by others(call ktread_stop)
or by itself. After 618f003199c6 commit we can trigger this issue very easy.
Before this commit also exist this issue.
If kmmpd exit by itself, after merge 618f003199c6 commit there will trigger UAF
when umount filesystem.
To fix this issue, introduce sbi->s_mmp_lock to protect sbi->s_mmp_tsk. If kmmpd
exit by itself, we set sbi->s_mmp_tsk with NULL, and release mmp buffer_head.

Fixes: 618f003199c6 ("ext4: fix memory leak in ext4_fill_super")
Signed-off-by: Ye Bin <redacted>
---
 fs/ext4/ext4.h  |  1 +
 fs/ext4/mmp.c   | 24 ++++++++++++++++++++++--
 fs/ext4/super.c |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 8d3446746718..a479da37fed4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h

@@ -1489,6 +1489,7 @@ struct ext4_sb_info {
 	struct completion s_kobj_unregister;
 	struct super_block *s_sb;
 	struct buffer_head *s_mmp_bh;
+	struct mutex s_mmp_lock;
 
 	/* Journaling */
 	struct journal_s *s_journal;

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..fc18a8c205c7 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -128,8 +128,9 @@ void __dump_mmp_msg(struct super_block *sb, struct mmp_struct *mmp,
 static int kmmpd(void *data)
 {
 	struct super_block *sb = (struct super_block *) data;
-	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
-	struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	struct ext4_super_block *es = sbi->s_es;
+	struct buffer_head *bh = sbi->s_mmp_bh;
 	struct mmp_struct *mmp;
 	ext4_fsblk_t mmp_block;
 	u32 seq = 0;

@@ -245,16 +246,35 @@ static int kmmpd(void *data)
 	retval = write_mmp_block(sb, bh);
 
 exit_thread:
+	/*
+	 * Maybe s_mmp_tsk kthread is stoped by others or by itself. If exit
+	 * by itself then sbi->s_mmp_tsk will be wild ptr, so there is need
+	 * set sbi->s_mmp_tsk with NULL, and also release mmp buffer_head.
+	 */
+	while (!kthread_should_stop()) {
+		if (!mutex_trylock(&sbi->s_mmp_lock))
+			continue;
+
+		if (sbi->s_mmp_tsk) {
+			sbi->s_mmp_tsk = NULL;
+			brelse(bh);
+		}
+		mutex_unlock(&sbi->s_mmp_lock);
+		break;
+	}
+
 	return retval;
 }
 
 void ext4_stop_mmpd(struct ext4_sb_info *sbi)
 {
+	mutex_lock(&sbi->s_mmp_lock);
 	if (sbi->s_mmp_tsk) {
 		kthread_stop(sbi->s_mmp_tsk);
 		brelse(sbi->s_mmp_bh);
 		sbi->s_mmp_tsk = NULL;
 	}
+	mutex_unlock(&sbi->s_mmp_lock);
 }
 
 /*

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c3942804b57f..5bc3230553fb 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -4786,6 +4786,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	needs_recovery = (es->s_last_orphan != 0 ||
 			  ext4_has_feature_journal_needs_recovery(sb));
 
+	mutex_init(&sbi->s_mmp_lock);
 	if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb))
 		if (ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)))
 			goto failed_mount3a;

-- 
2.31.1

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/e525c0bf7b18da426bb3d3dd63830a3f85218a9e.1625244710.git.tytso@mit.edu
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---

Ye, thanks for reporting the bug!  I think this might be a better way
to address the problem, since it avoids adding a mutex just to protect
s_mmp_tsk.  What do you think?

 fs/ext4/mmp.c   | 31 +++++++++++++++++--------------
 fs/ext4/super.c |  6 +++++-
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..af461df1c1ec 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -157,6 +157,16 @@ static int kmmpd(void *data)
 	       sizeof(mmp->mmp_nodename));
 
 	while (!kthread_should_stop()) {
+		if (!(le32_to_cpu(es->s_feature_incompat) &
+		    EXT4_FEATURE_INCOMPAT_MMP)) {
+			ext4_warning(sb, "kmmpd being stopped since MMP feature"
+				     " has been disabled.");
+			goto wait_to_exit;
+		}
+		if (sb_rdonly(sb)) {
+			schedule_timeout_interruptible(HZ);
+			continue;
+		}
 		if (++seq > EXT4_MMP_SEQ_MAX)
 			seq = 1;
 

@@ -177,16 +187,6 @@ static int kmmpd(void *data)
 			failed_writes++;
 		}
 
-		if (!(le32_to_cpu(es->s_feature_incompat) &
-		    EXT4_FEATURE_INCOMPAT_MMP)) {
-			ext4_warning(sb, "kmmpd being stopped since MMP feature"
-				     " has been disabled.");
-			goto exit_thread;
-		}
-
-		if (sb_rdonly(sb))
-			break;
-
 		diff = jiffies - last_update_time;
 		if (diff < mmp_update_interval * HZ)
 			schedule_timeout_interruptible(mmp_update_interval *

@@ -207,7 +207,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
 					       retval);
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);

@@ -221,7 +221,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, EBUSY, "abort");
 				put_bh(bh_check);
 				retval = -EBUSY;
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 			put_bh(bh_check);
 		}

@@ -246,6 +246,11 @@ static int kmmpd(void *data)
 
 exit_thread:
 	return retval;
+wait_to_exit:
+	while (!kthread_should_stop())
+		schedule();
+	return retval;
+
 }
 
 void ext4_stop_mmpd(struct ext4_sb_info *sbi)

@@ -391,5 +396,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	brelse(bh);
 	return 1;
 }
-
-

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 				 */
 				ext4_mark_recovery_complete(sb, es);
 			}
-			ext4_stop_mmpd(sbi);
 		} else {
 			/* Make sure we can mount this feature set readwrite */
 			if (ext4_has_feature_readonly(sb) ||

@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
 		ext4_release_system_zone(sb);
 
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
+
 	/*
 	 * Some options can be enabled by ext4 and/or by VFS mount flag
 	 * either way we need to make sure it matches in both *flags and

@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		kfree(to_free[i]);
 #endif
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
 	kfree(orig_data);
 	return err;
 }

-- 
2.31.0

Hi Theodore,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ext4/dev]
[also build test WARNING on next-20210701]
[cannot apply to v5.13]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: mips-randconfig-r021-20210702 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9eb613b2de3163686b1a4bd1160f15ac56a4b083)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/37b4aa9eef5b3f07f803e18d4dba7aba46148f87
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
        git checkout 37b4aa9eef5b3f07f803e18d4dba7aba46148f87
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All warnings (new ones prefixed by >>):

quoted

fs/ext4/mmp.c:247:1: warning: unused label 'exit_thread' [-Wunused-label]

   exit_thread:
   ^~~~~~~~~~~~
   1 warning generated.


vim +/exit_thread +247 fs/ext4/mmp.c

c5e06d101aaf72 Johann Lombardi   2011-05-24  124  
c5e06d101aaf72 Johann Lombardi   2011-05-24  125  /*
c5e06d101aaf72 Johann Lombardi   2011-05-24  126   * kmmpd will update the MMP sequence every s_mmp_update_interval seconds
c5e06d101aaf72 Johann Lombardi   2011-05-24  127   */
c5e06d101aaf72 Johann Lombardi   2011-05-24  128  static int kmmpd(void *data)
c5e06d101aaf72 Johann Lombardi   2011-05-24  129  {
618f003199c618 Pavel Skripkin    2021-04-30  130  	struct super_block *sb = (struct super_block *) data;
c5e06d101aaf72 Johann Lombardi   2011-05-24  131  	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
618f003199c618 Pavel Skripkin    2021-04-30  132  	struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
c5e06d101aaf72 Johann Lombardi   2011-05-24  133  	struct mmp_struct *mmp;
c5e06d101aaf72 Johann Lombardi   2011-05-24  134  	ext4_fsblk_t mmp_block;
c5e06d101aaf72 Johann Lombardi   2011-05-24  135  	u32 seq = 0;
c5e06d101aaf72 Johann Lombardi   2011-05-24  136  	unsigned long failed_writes = 0;
c5e06d101aaf72 Johann Lombardi   2011-05-24  137  	int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
c5e06d101aaf72 Johann Lombardi   2011-05-24  138  	unsigned mmp_check_interval;
c5e06d101aaf72 Johann Lombardi   2011-05-24  139  	unsigned long last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  140  	unsigned long diff;
c5e06d101aaf72 Johann Lombardi   2011-05-24  141  	int retval;
c5e06d101aaf72 Johann Lombardi   2011-05-24  142  
c5e06d101aaf72 Johann Lombardi   2011-05-24  143  	mmp_block = le64_to_cpu(es->s_mmp_block);
c5e06d101aaf72 Johann Lombardi   2011-05-24  144  	mmp = (struct mmp_struct *)(bh->b_data);
af123b3718592a Arnd Bergmann     2018-07-29  145  	mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  146  	/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  147  	 * Start with the higher mmp_check_interval and reduce it if
c5e06d101aaf72 Johann Lombardi   2011-05-24  148  	 * the MMP block is being updated on time.
c5e06d101aaf72 Johann Lombardi   2011-05-24  149  	 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  150  	mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
c5e06d101aaf72 Johann Lombardi   2011-05-24  151  				 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi   2011-05-24  152  	mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
14c9ca0583eee8 Andreas Dilger    2020-01-26  153  	BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
c5e06d101aaf72 Johann Lombardi   2011-05-24  154  	bdevname(bh->b_bdev, mmp->mmp_bdevname);
c5e06d101aaf72 Johann Lombardi   2011-05-24  155  
215fc6af739d2d Nikitas Angelinas 2011-10-18  156  	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
c5e06d101aaf72 Johann Lombardi   2011-05-24  157  	       sizeof(mmp->mmp_nodename));
c5e06d101aaf72 Johann Lombardi   2011-05-24  158  
c5e06d101aaf72 Johann Lombardi   2011-05-24  159  	while (!kthread_should_stop()) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  160  		if (!(le32_to_cpu(es->s_feature_incompat) &
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  161  		    EXT4_FEATURE_INCOMPAT_MMP)) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  162  			ext4_warning(sb, "kmmpd being stopped since MMP feature"
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  163  				     " has been disabled.");
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  164  			goto wait_to_exit;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  165  		}
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  166  		if (sb_rdonly(sb)) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  167  			schedule_timeout_interruptible(HZ);
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  168  			continue;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  169  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  170  		if (++seq > EXT4_MMP_SEQ_MAX)
c5e06d101aaf72 Johann Lombardi   2011-05-24  171  			seq = 1;
c5e06d101aaf72 Johann Lombardi   2011-05-24  172  
c5e06d101aaf72 Johann Lombardi   2011-05-24  173  		mmp->mmp_seq = cpu_to_le32(seq);
af123b3718592a Arnd Bergmann     2018-07-29  174  		mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  175  		last_update_time = jiffies;
c5e06d101aaf72 Johann Lombardi   2011-05-24  176  
5c359a47e7d999 Darrick J. Wong   2012-04-29  177  		retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi   2011-05-24  178  		/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  179  		 * Don't spew too many error messages. Print one every
c5e06d101aaf72 Johann Lombardi   2011-05-24  180  		 * (s_mmp_update_interval * 60) seconds.
c5e06d101aaf72 Johann Lombardi   2011-05-24  181  		 */
bdfc230f33a9da Nikitas Angelinas 2011-10-18  182  		if (retval) {
878520ac45f9f6 Theodore Ts'o     2019-11-19  183  			if ((failed_writes % 60) == 0) {
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  184  				ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  185  					       "Error writing to MMP block");
878520ac45f9f6 Theodore Ts'o     2019-11-19  186  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  187  			failed_writes++;
c5e06d101aaf72 Johann Lombardi   2011-05-24  188  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  189  
c5e06d101aaf72 Johann Lombardi   2011-05-24  190  		diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  191  		if (diff < mmp_update_interval * HZ)
c5e06d101aaf72 Johann Lombardi   2011-05-24  192  			schedule_timeout_interruptible(mmp_update_interval *
c5e06d101aaf72 Johann Lombardi   2011-05-24  193  						       HZ - diff);
c5e06d101aaf72 Johann Lombardi   2011-05-24  194  
c5e06d101aaf72 Johann Lombardi   2011-05-24  195  		/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  196  		 * We need to make sure that more than mmp_check_interval
c5e06d101aaf72 Johann Lombardi   2011-05-24  197  		 * seconds have not passed since writing. If that has happened
c5e06d101aaf72 Johann Lombardi   2011-05-24  198  		 * we need to check if the MMP block is as we left it.
c5e06d101aaf72 Johann Lombardi   2011-05-24  199  		 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  200  		diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  201  		if (diff > mmp_check_interval * HZ) {
c5e06d101aaf72 Johann Lombardi   2011-05-24  202  			struct buffer_head *bh_check = NULL;
c5e06d101aaf72 Johann Lombardi   2011-05-24  203  			struct mmp_struct *mmp_check;
c5e06d101aaf72 Johann Lombardi   2011-05-24  204  
c5e06d101aaf72 Johann Lombardi   2011-05-24  205  			retval = read_mmp_block(sb, &bh_check, mmp_block);
c5e06d101aaf72 Johann Lombardi   2011-05-24  206  			if (retval) {
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  207  				ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  208  					       "error reading MMP data: %d",
c5e06d101aaf72 Johann Lombardi   2011-05-24  209  					       retval);
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  210  				goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi   2011-05-24  211  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  212  
c5e06d101aaf72 Johann Lombardi   2011-05-24  213  			mmp_check = (struct mmp_struct *)(bh_check->b_data);
c5e06d101aaf72 Johann Lombardi   2011-05-24  214  			if (mmp->mmp_seq != mmp_check->mmp_seq ||
c5e06d101aaf72 Johann Lombardi   2011-05-24  215  			    memcmp(mmp->mmp_nodename, mmp_check->mmp_nodename,
c5e06d101aaf72 Johann Lombardi   2011-05-24  216  				   sizeof(mmp->mmp_nodename))) {
c5e06d101aaf72 Johann Lombardi   2011-05-24  217  				dump_mmp_msg(sb, mmp_check,
c5e06d101aaf72 Johann Lombardi   2011-05-24  218  					     "Error while updating MMP info. "
c5e06d101aaf72 Johann Lombardi   2011-05-24  219  					     "The filesystem seems to have been"
c5e06d101aaf72 Johann Lombardi   2011-05-24  220  					     " multiply mounted.");
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  221  				ext4_error_err(sb, EBUSY, "abort");
0304688676bdfc vikram.jadhav07   2016-03-13  222  				put_bh(bh_check);
0304688676bdfc vikram.jadhav07   2016-03-13  223  				retval = -EBUSY;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  224  				goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi   2011-05-24  225  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  226  			put_bh(bh_check);
c5e06d101aaf72 Johann Lombardi   2011-05-24  227  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  228  
c5e06d101aaf72 Johann Lombardi   2011-05-24  229  		 /*
c5e06d101aaf72 Johann Lombardi   2011-05-24  230  		 * Adjust the mmp_check_interval depending on how much time
c5e06d101aaf72 Johann Lombardi   2011-05-24  231  		 * it took for the MMP block to be written.
c5e06d101aaf72 Johann Lombardi   2011-05-24  232  		 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  233  		mmp_check_interval = max(min(EXT4_MMP_CHECK_MULT * diff / HZ,
c5e06d101aaf72 Johann Lombardi   2011-05-24  234  					     EXT4_MMP_MAX_CHECK_INTERVAL),
c5e06d101aaf72 Johann Lombardi   2011-05-24  235  					 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi   2011-05-24  236  		mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
c5e06d101aaf72 Johann Lombardi   2011-05-24  237  	}
c5e06d101aaf72 Johann Lombardi   2011-05-24  238  
c5e06d101aaf72 Johann Lombardi   2011-05-24  239  	/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  240  	 * Unmount seems to be clean.
c5e06d101aaf72 Johann Lombardi   2011-05-24  241  	 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  242  	mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
af123b3718592a Arnd Bergmann     2018-07-29  243  	mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  244  
5c359a47e7d999 Darrick J. Wong   2012-04-29  245  	retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi   2011-05-24  246  
0304688676bdfc vikram.jadhav07   2016-03-13 @247  exit_thread:
c5e06d101aaf72 Johann Lombardi   2011-05-24  248  	return retval;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  249  wait_to_exit:
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  250  	while (!kthread_should_stop())
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  251  		schedule();
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  252  	return retval;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  253  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Hi Theodore,

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-possible-use-after-free-when-remounting-r-o-a-mmp-protected-file-system/20210703-005856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: i386-randconfig-m021-20210702 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
Reported-by: Dan Carpenter <redacted>

smatch warnings:
fs/ext4/mmp.c:252 kmmpd() error: uninitialized symbol 'retval'.

vim +/retval +252 fs/ext4/mmp.c

c5e06d101aaf72 Johann Lombardi   2011-05-24  128  static int kmmpd(void *data)
c5e06d101aaf72 Johann Lombardi   2011-05-24  129  {
618f003199c618 Pavel Skripkin    2021-04-30  130  	struct super_block *sb = (struct super_block *) data;
c5e06d101aaf72 Johann Lombardi   2011-05-24  131  	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
618f003199c618 Pavel Skripkin    2021-04-30  132  	struct buffer_head *bh = EXT4_SB(sb)->s_mmp_bh;
c5e06d101aaf72 Johann Lombardi   2011-05-24  133  	struct mmp_struct *mmp;
c5e06d101aaf72 Johann Lombardi   2011-05-24  134  	ext4_fsblk_t mmp_block;
c5e06d101aaf72 Johann Lombardi   2011-05-24  135  	u32 seq = 0;
c5e06d101aaf72 Johann Lombardi   2011-05-24  136  	unsigned long failed_writes = 0;
c5e06d101aaf72 Johann Lombardi   2011-05-24  137  	int mmp_update_interval = le16_to_cpu(es->s_mmp_update_interval);
c5e06d101aaf72 Johann Lombardi   2011-05-24  138  	unsigned mmp_check_interval;
c5e06d101aaf72 Johann Lombardi   2011-05-24  139  	unsigned long last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  140  	unsigned long diff;
c5e06d101aaf72 Johann Lombardi   2011-05-24  141  	int retval;
c5e06d101aaf72 Johann Lombardi   2011-05-24  142  
c5e06d101aaf72 Johann Lombardi   2011-05-24  143  	mmp_block = le64_to_cpu(es->s_mmp_block);
c5e06d101aaf72 Johann Lombardi   2011-05-24  144  	mmp = (struct mmp_struct *)(bh->b_data);
af123b3718592a Arnd Bergmann     2018-07-29  145  	mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  146  	/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  147  	 * Start with the higher mmp_check_interval and reduce it if
c5e06d101aaf72 Johann Lombardi   2011-05-24  148  	 * the MMP block is being updated on time.
c5e06d101aaf72 Johann Lombardi   2011-05-24  149  	 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  150  	mmp_check_interval = max(EXT4_MMP_CHECK_MULT * mmp_update_interval,
c5e06d101aaf72 Johann Lombardi   2011-05-24  151  				 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi   2011-05-24  152  	mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
14c9ca0583eee8 Andreas Dilger    2020-01-26  153  	BUILD_BUG_ON(sizeof(mmp->mmp_bdevname) < BDEVNAME_SIZE);
c5e06d101aaf72 Johann Lombardi   2011-05-24  154  	bdevname(bh->b_bdev, mmp->mmp_bdevname);
c5e06d101aaf72 Johann Lombardi   2011-05-24  155  
215fc6af739d2d Nikitas Angelinas 2011-10-18  156  	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
c5e06d101aaf72 Johann Lombardi   2011-05-24  157  	       sizeof(mmp->mmp_nodename));
c5e06d101aaf72 Johann Lombardi   2011-05-24  158  
c5e06d101aaf72 Johann Lombardi   2011-05-24  159  	while (!kthread_should_stop()) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  160  		if (!(le32_to_cpu(es->s_feature_incompat) &
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  161  		    EXT4_FEATURE_INCOMPAT_MMP)) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  162  			ext4_warning(sb, "kmmpd being stopped since MMP feature"
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  163  				     " has been disabled.");
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  164  			goto wait_to_exit;

Smatch complains about this goto.

37b4aa9eef5b3f Theodore Ts'o     2021-07-02  165  		}
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  166  		if (sb_rdonly(sb)) {
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  167  			schedule_timeout_interruptible(HZ);
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  168  			continue;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  169  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  170  		if (++seq > EXT4_MMP_SEQ_MAX)
c5e06d101aaf72 Johann Lombardi   2011-05-24  171  			seq = 1;
c5e06d101aaf72 Johann Lombardi   2011-05-24  172  
c5e06d101aaf72 Johann Lombardi   2011-05-24  173  		mmp->mmp_seq = cpu_to_le32(seq);
af123b3718592a Arnd Bergmann     2018-07-29  174  		mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  175  		last_update_time = jiffies;
c5e06d101aaf72 Johann Lombardi   2011-05-24  176  
5c359a47e7d999 Darrick J. Wong   2012-04-29  177  		retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi   2011-05-24  178  		/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  179  		 * Don't spew too many error messages. Print one every
c5e06d101aaf72 Johann Lombardi   2011-05-24  180  		 * (s_mmp_update_interval * 60) seconds.
c5e06d101aaf72 Johann Lombardi   2011-05-24  181  		 */
bdfc230f33a9da Nikitas Angelinas 2011-10-18  182  		if (retval) {
878520ac45f9f6 Theodore Ts'o     2019-11-19  183  			if ((failed_writes % 60) == 0) {
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  184  				ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  185  					       "Error writing to MMP block");
878520ac45f9f6 Theodore Ts'o     2019-11-19  186  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  187  			failed_writes++;
c5e06d101aaf72 Johann Lombardi   2011-05-24  188  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  189  
c5e06d101aaf72 Johann Lombardi   2011-05-24  190  		diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  191  		if (diff < mmp_update_interval * HZ)
c5e06d101aaf72 Johann Lombardi   2011-05-24  192  			schedule_timeout_interruptible(mmp_update_interval *
c5e06d101aaf72 Johann Lombardi   2011-05-24  193  						       HZ - diff);
c5e06d101aaf72 Johann Lombardi   2011-05-24  194  
c5e06d101aaf72 Johann Lombardi   2011-05-24  195  		/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  196  		 * We need to make sure that more than mmp_check_interval
c5e06d101aaf72 Johann Lombardi   2011-05-24  197  		 * seconds have not passed since writing. If that has happened
c5e06d101aaf72 Johann Lombardi   2011-05-24  198  		 * we need to check if the MMP block is as we left it.
c5e06d101aaf72 Johann Lombardi   2011-05-24  199  		 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  200  		diff = jiffies - last_update_time;
c5e06d101aaf72 Johann Lombardi   2011-05-24  201  		if (diff > mmp_check_interval * HZ) {
c5e06d101aaf72 Johann Lombardi   2011-05-24  202  			struct buffer_head *bh_check = NULL;
c5e06d101aaf72 Johann Lombardi   2011-05-24  203  			struct mmp_struct *mmp_check;
c5e06d101aaf72 Johann Lombardi   2011-05-24  204  
c5e06d101aaf72 Johann Lombardi   2011-05-24  205  			retval = read_mmp_block(sb, &bh_check, mmp_block);
c5e06d101aaf72 Johann Lombardi   2011-05-24  206  			if (retval) {
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  207  				ext4_error_err(sb, -retval,
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  208  					       "error reading MMP data: %d",
c5e06d101aaf72 Johann Lombardi   2011-05-24  209  					       retval);
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  210  				goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi   2011-05-24  211  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  212  
c5e06d101aaf72 Johann Lombardi   2011-05-24  213  			mmp_check = (struct mmp_struct *)(bh_check->b_data);
c5e06d101aaf72 Johann Lombardi   2011-05-24  214  			if (mmp->mmp_seq != mmp_check->mmp_seq ||
c5e06d101aaf72 Johann Lombardi   2011-05-24  215  			    memcmp(mmp->mmp_nodename, mmp_check->mmp_nodename,
c5e06d101aaf72 Johann Lombardi   2011-05-24  216  				   sizeof(mmp->mmp_nodename))) {
c5e06d101aaf72 Johann Lombardi   2011-05-24  217  				dump_mmp_msg(sb, mmp_check,
c5e06d101aaf72 Johann Lombardi   2011-05-24  218  					     "Error while updating MMP info. "
c5e06d101aaf72 Johann Lombardi   2011-05-24  219  					     "The filesystem seems to have been"
c5e06d101aaf72 Johann Lombardi   2011-05-24  220  					     " multiply mounted.");
54d3adbc29f0c7 Theodore Ts'o     2020-03-28  221  				ext4_error_err(sb, EBUSY, "abort");
0304688676bdfc vikram.jadhav07   2016-03-13  222  				put_bh(bh_check);
0304688676bdfc vikram.jadhav07   2016-03-13  223  				retval = -EBUSY;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  224  				goto wait_to_exit;
c5e06d101aaf72 Johann Lombardi   2011-05-24  225  			}
c5e06d101aaf72 Johann Lombardi   2011-05-24  226  			put_bh(bh_check);
c5e06d101aaf72 Johann Lombardi   2011-05-24  227  		}
c5e06d101aaf72 Johann Lombardi   2011-05-24  228  
c5e06d101aaf72 Johann Lombardi   2011-05-24  229  		 /*
c5e06d101aaf72 Johann Lombardi   2011-05-24  230  		 * Adjust the mmp_check_interval depending on how much time
c5e06d101aaf72 Johann Lombardi   2011-05-24  231  		 * it took for the MMP block to be written.
c5e06d101aaf72 Johann Lombardi   2011-05-24  232  		 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  233  		mmp_check_interval = max(min(EXT4_MMP_CHECK_MULT * diff / HZ,
c5e06d101aaf72 Johann Lombardi   2011-05-24  234  					     EXT4_MMP_MAX_CHECK_INTERVAL),
c5e06d101aaf72 Johann Lombardi   2011-05-24  235  					 EXT4_MMP_MIN_CHECK_INTERVAL);
c5e06d101aaf72 Johann Lombardi   2011-05-24  236  		mmp->mmp_check_interval = cpu_to_le16(mmp_check_interval);
c5e06d101aaf72 Johann Lombardi   2011-05-24  237  	}
c5e06d101aaf72 Johann Lombardi   2011-05-24  238  
c5e06d101aaf72 Johann Lombardi   2011-05-24  239  	/*
c5e06d101aaf72 Johann Lombardi   2011-05-24  240  	 * Unmount seems to be clean.
c5e06d101aaf72 Johann Lombardi   2011-05-24  241  	 */
c5e06d101aaf72 Johann Lombardi   2011-05-24  242  	mmp->mmp_seq = cpu_to_le32(EXT4_MMP_SEQ_CLEAN);
af123b3718592a Arnd Bergmann     2018-07-29  243  	mmp->mmp_time = cpu_to_le64(ktime_get_real_seconds());
c5e06d101aaf72 Johann Lombardi   2011-05-24  244  
5c359a47e7d999 Darrick J. Wong   2012-04-29  245  	retval = write_mmp_block(sb, bh);
c5e06d101aaf72 Johann Lombardi   2011-05-24  246  
0304688676bdfc vikram.jadhav07   2016-03-13  247  exit_thread:
c5e06d101aaf72 Johann Lombardi   2011-05-24  248  	return retval;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  249  wait_to_exit:
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  250  	while (!kthread_should_stop())
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  251  		schedule();
37b4aa9eef5b3f Theodore Ts'o     2021-07-02 @252  	return retval;
37b4aa9eef5b3f Theodore Ts'o     2021-07-02  253  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

On Tue 29-06-21 22:36:03, Ye Bin wrote:

Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
already running and even exit as exception. Even though we set
sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
"sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
   mount                     kmmpd
     |                         |
     |-call kthread_run        |
     |                         |-kmmpd runing
     |                         |-kmmpd exit sbi->s_mmp_tsk=NULL
     |                         |
     |-kthread_run return      |
     | and set sbi->s_mmp_tsk  |
     |                         |
     |-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF

This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
NULL. We can create kthread first, and then wakeup kmmpd kthread later.

Signed-off-by: Ye Bin <redacted>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR

On Tue 29-06-21 22:36:02, Ye Bin wrote:

Honestly, this is just ugly. I think it would be saner if ext4_stop_mmpd()
did:

void ext4_stop_mmpd(struct ext4_sb_info *sbi)
{
	struct task_struct *tsk;

	mutex_lock(&sbi->s_mmp_lock);
	tsk = sbi->s_mmp_tsk;
	if (tsk) {
		sbi->s_mmp_tsk = NULL;
		get_task_struct(tsk);
	}
     	mutex_unlock(&sbi->s_mmp_lock);
	if (tsk) {
		kthread_stop(tsk);
		put_task_struct(k);
	}
}

And you leave freeing of 'bh' to exit path from kmmpd() which can also use
normal locking scheme for zeroing s_mmp_tsk now.

That being said for this scheme spinlock is enough, you don't need a mutex
for s_mmp_lock.

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR

On Mon, Jul 05, 2021 at 01:15:48PM +0200, Jan Kara wrote:

That being said for this scheme spinlock is enough, you don't need a mutex
for s_mmp_lock.

I think we can solve this without using using either a spinlock or a
mutex, and it's a smaller and simpler patch as a result.  (This is the
-v2 version of this patch, which removes an unused label compared to
the earlier version.)

From 22ebc97aac75e27a5fd11acdb2bc3030d1da58d1 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Fri, 2 Jul 2021 12:45:02 -0400
Subject: [PATCH] ext4: fix possible UAF when remounting r/o a mmp-protected file system

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/e525c0bf7b18da426bb3d3dd63830a3f85218a9e.1625244710.git.tytso@mit.edu
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/mmp.c   | 33 +++++++++++++++++----------------
 fs/ext4/super.c |  6 +++++-
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..1e95cee3d8b7 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -157,6 +157,17 @@ static int kmmpd(void *data)
 	       sizeof(mmp->mmp_nodename));
 
 	while (!kthread_should_stop()) {
+		if (!(le32_to_cpu(es->s_feature_incompat) &
+		    EXT4_FEATURE_INCOMPAT_MMP)) {
+			ext4_warning(sb, "kmmpd being stopped since MMP feature"
+				     " has been disabled.");
+			goto wait_to_exit;
+		}
+		if (sb_rdonly(sb)) {
+			if (!kthread_should_stop())
+				schedule_timeout_interruptible(HZ);
+			continue;
+		}
 		if (++seq > EXT4_MMP_SEQ_MAX)
 			seq = 1;
 

@@ -177,16 +188,6 @@ static int kmmpd(void *data)
 			failed_writes++;
 		}
 
-		if (!(le32_to_cpu(es->s_feature_incompat) &
-		    EXT4_FEATURE_INCOMPAT_MMP)) {
-			ext4_warning(sb, "kmmpd being stopped since MMP feature"
-				     " has been disabled.");
-			goto exit_thread;
-		}
-
-		if (sb_rdonly(sb))
-			break;
-
 		diff = jiffies - last_update_time;
 		if (diff < mmp_update_interval * HZ)
 			schedule_timeout_interruptible(mmp_update_interval *

@@ -207,7 +208,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
 					       retval);
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);

@@ -221,7 +222,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, EBUSY, "abort");
 				put_bh(bh_check);
 				retval = -EBUSY;
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 			put_bh(bh_check);
 		}

@@ -242,9 +243,11 @@ 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);
 
-exit_thread:
+wait_to_exit:
+	while (!kthread_should_stop())
+		schedule();
 	return retval;
 }
 

@@ -391,5 +394,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	brelse(bh);
 	return 1;
 }
-
-

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 				 */
 				ext4_mark_recovery_complete(sb, es);
 			}
-			ext4_stop_mmpd(sbi);
 		} else {
 			/* Make sure we can mount this feature set readwrite */
 			if (ext4_has_feature_readonly(sb) ||

@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
 		ext4_release_system_zone(sb);
 
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
+
 	/*
 	 * Some options can be enabled by ext4 and/or by VFS mount flag
 	 * either way we need to make sure it matches in both *flags and

@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		kfree(to_free[i]);
 #endif
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
 	kfree(orig_data);
 	return err;
 }

-- 
2.31.0

On Jun 29, 2021, at 8:36 AM, Ye Bin [off-list ref] wrote:

Now sbi->s_mmp_tsk is created with kthread_run, then kmmpd maybe
already running and even exit as exception. Even though we set
sbi->s_mmp_tsk with NULL before kmmpd kthread exit, but
"sbi->s_mmp_tsk=kthread_run(XX)" may set after set with NULL.
  mount                     kmmpd
    |                         |
    |-call kthread_run        |
    |                         |-kmmpd runing
    |                         |-kmmpd exit sbi->s_mmp_tsk=NULL
    |                         |
    |-kthread_run return      |
    | and set sbi->s_mmp_tsk  |
    |                         |
    |-then we get wild ptr"sbi->s_mmp_tsk" and later trigger UAF

This patch is base on previous "ext4: Fix use-after-free about sbi->s_mmp_tsk".
Previous patch ensure kmmpd kthread exit by itself will set sbi->s_mmp_tsk with
NULL. We can create kthread first, and then wakeup kmmpd kthread later.

Signed-off-by: Ye Bin <redacted>

Please note minor typo in the patch subject, should be "use-after-free".

Reviewed-by: Andreas Dilger <redacted>

Cheers, Andreas

On Mon 05-07-21 16:35:28, Theodore Ts'o wrote:

On Mon, Jul 05, 2021 at 01:15:48PM +0200, Jan Kara wrote:

quoted

That being said for this scheme spinlock is enough, you don't need a mutex
for s_mmp_lock.

I think we can solve this without using using either a spinlock or a
mutex, and it's a smaller and simpler patch as a result.  (This is the
-v2 version of this patch, which removes an unused label compared to
the earlier version.)

Yeah, what you suggest is probably simpler. Some comments below.

We can probably use ext4_has_feature_mmp() macro when changing this?

+			ext4_warning(sb, "kmmpd being stopped since MMP feature"
+				     " has been disabled.");
+			goto wait_to_exit;
+		}
+		if (sb_rdonly(sb)) {
+			if (!kthread_should_stop())
+				schedule_timeout_interruptible(HZ);

Cannot this effectively block remount RO for 1s when we wait for kmmpd to
exit? I think doing 'break' when we detected RO super is fine. We'll write
the mmp block and then wait for kthread_should_stop() condition as in any
other abort case. Am I missing something?

This makes me a bit nervous that we could unnecessarily burn CPU for
potentially a long time (e.g. if somebody uses tune2fs to disable MMP, we
would be sitting in this loop until the fs in remounted / unmounted). So
maybe we should have something like:

	while (!kthread_should_stop()) {
		set_task_state(TASK_INTERRUPTIBLE);
		if (!kthread_should_stop())
			schedule();
	}

This should safely synchronize with (and not miss wakeup from)
kthread_stop() since that first sets KTHREAD_SHOULD_STOP and after that
calls wake_up_process().

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR

On Tue, Jul 06, 2021 at 01:11:37PM +0200, Jan Kara wrote:

quoted

--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -157,6 +157,17 @@ static int kmmpd(void *data)
 	       sizeof(mmp->mmp_nodename));
 
 	while (!kthread_should_stop()) {
+		if (!(le32_to_cpu(es->s_feature_incompat) &
+		    EXT4_FEATURE_INCOMPAT_MMP)) {

We can probably use ext4_has_feature_mmp() macro when changing this?

Ack, I'll make that change.

quoted

+		if (sb_rdonly(sb)) {
+			if (!kthread_should_stop())
+				schedule_timeout_interruptible(HZ);

Cannot this effectively block remount RO for 1s when we wait for kmmpd to
exit? I think doing 'break' when we detected RO super is fine. We'll write
the mmp block and then wait for kthread_should_stop() condition as in any
other abort case. Am I missing something?

Yeah, we do want to update the mmp block when remounting the file
system read-only.  So breaking out to exit is the right thing to do
here.

quoted

+wait_to_exit:
+	while (!kthread_should_stop())
+		schedule();

This makes me a bit nervous that we could unnecessarily burn CPU for
potentially a long time (e.g. if somebody uses tune2fs to disable MMP, we
would be sitting in this loop until the fs in remounted / unmounted). So
maybe we should have something like:

	while (!kthread_should_stop()) {
		set_task_state(TASK_INTERRUPTIBLE);
		if (!kthread_should_stop())
			schedule();
	}

This should safely synchronize with (and not miss wakeup from)
kthread_stop() since that first sets KTHREAD_SHOULD_STOP and after that
calls wake_up_process().

Yep, good catch.  I'll fix this and send out revised patch.

     	  	       	   	    - Ted

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
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/mmp.c   | 31 ++++++++++++++-----------------
 fs/ext4/super.c |  6 +++++-
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..16d69c0ff46a 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -156,7 +156,12 @@ static int kmmpd(void *data)
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
 	       sizeof(mmp->mmp_nodename));
 
-	while (!kthread_should_stop()) {
+	while (!kthread_should_stop() && !sb_rdonly(sb)) {
+		if (!ext4_has_feature_mmp(sb)) {
+			ext4_warning(sb, "kmmpd being stopped since MMP feature"
+				     " has been disabled.");
+			goto wait_to_exit;
+		}
 		if (++seq > EXT4_MMP_SEQ_MAX)
 			seq = 1;
 

@@ -177,16 +182,6 @@ static int kmmpd(void *data)
 			failed_writes++;
 		}
 
-		if (!(le32_to_cpu(es->s_feature_incompat) &
-		    EXT4_FEATURE_INCOMPAT_MMP)) {
-			ext4_warning(sb, "kmmpd being stopped since MMP feature"
-				     " has been disabled.");
-			goto exit_thread;
-		}
-
-		if (sb_rdonly(sb))
-			break;
-
 		diff = jiffies - last_update_time;
 		if (diff < mmp_update_interval * HZ)
 			schedule_timeout_interruptible(mmp_update_interval *

@@ -207,7 +202,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
 					       retval);
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);

@@ -221,7 +216,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, EBUSY, "abort");
 				put_bh(bh_check);
 				retval = -EBUSY;
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 			put_bh(bh_check);
 		}

@@ -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);
 
-exit_thread:
+wait_to_exit:
+	set_current_state(TASK_INTERRUPTIBLE);
+	while (!kthread_should_stop())
+		schedule();
+	set_current_state(TASK_RUNNING);
 	return retval;
 }
 

@@ -391,5 +390,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	brelse(bh);
 	return 1;
 }
-
-

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 				 */
 				ext4_mark_recovery_complete(sb, es);
 			}
-			ext4_stop_mmpd(sbi);
 		} else {
 			/* Make sure we can mount this feature set readwrite */
 			if (ext4_has_feature_readonly(sb) ||

@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
 		ext4_release_system_zone(sb);
 
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
+
 	/*
 	 * Some options can be enabled by ext4 and/or by VFS mount flag
 	 * either way we need to make sure it matches in both *flags and

@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		kfree(to_free[i]);
 #endif
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
 	kfree(orig_data);
 	return err;
 }

-- 
2.31.0

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
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

The patch looks mostly fine. Two comments below.

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

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
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/mmp.c   | 31 +++++++++++++++----------------
 fs/ext4/super.c |  6 +++++-
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 6cb598b549ca..bc364c119af6 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c

@@ -156,7 +156,12 @@ static int kmmpd(void *data)
 	memcpy(mmp->mmp_nodename, init_utsname()->nodename,
 	       sizeof(mmp->mmp_nodename));
 
-	while (!kthread_should_stop()) {
+	while (!kthread_should_stop() && !sb_rdonly(sb)) {
+		if (!ext4_has_feature_mmp(sb)) {
+			ext4_warning(sb, "kmmpd being stopped since MMP feature"
+				     " has been disabled.");
+			goto wait_to_exit;
+		}
 		if (++seq > EXT4_MMP_SEQ_MAX)
 			seq = 1;
 

@@ -177,16 +182,6 @@ static int kmmpd(void *data)
 			failed_writes++;
 		}
 
-		if (!(le32_to_cpu(es->s_feature_incompat) &
-		    EXT4_FEATURE_INCOMPAT_MMP)) {
-			ext4_warning(sb, "kmmpd being stopped since MMP feature"
-				     " has been disabled.");
-			goto exit_thread;
-		}
-
-		if (sb_rdonly(sb))
-			break;
-
 		diff = jiffies - last_update_time;
 		if (diff < mmp_update_interval * HZ)
 			schedule_timeout_interruptible(mmp_update_interval *

@@ -207,7 +202,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, -retval,
 					       "error reading MMP data: %d",
 					       retval);
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);

@@ -221,7 +216,7 @@ static int kmmpd(void *data)
 				ext4_error_err(sb, EBUSY, "abort");
 				put_bh(bh_check);
 				retval = -EBUSY;
-				goto exit_thread;
+				goto wait_to_exit;
 			}
 			put_bh(bh_check);
 		}

@@ -244,7 +239,13 @@ static int kmmpd(void *data)
 
 	retval = write_mmp_block(sb, bh);
 
-exit_thread:
+wait_to_exit:
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (!kthread_should_stop())
+			schedule();
+	}
+	set_current_state(TASK_RUNNING);
 	return retval;
 }
 

@@ -391,5 +392,3 @@ int ext4_multi_mount_protect(struct super_block *sb,
 	brelse(bh);
 	return 1;
 }
-
-

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cdbe71d935e8..b8ff0399e171 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c

@@ -5993,7 +5993,6 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 				 */
 				ext4_mark_recovery_complete(sb, es);
 			}
-			ext4_stop_mmpd(sbi);
 		} else {
 			/* Make sure we can mount this feature set readwrite */
 			if (ext4_has_feature_readonly(sb) ||

@@ -6107,6 +6106,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	if (!test_opt(sb, BLOCK_VALIDITY) && sbi->s_system_blks)
 		ext4_release_system_zone(sb);
 
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
+
 	/*
 	 * Some options can be enabled by ext4 and/or by VFS mount flag
 	 * either way we need to make sure it matches in both *flags and

@@ -6140,6 +6142,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
 	for (i = 0; i < EXT4_MAXQUOTAS; i++)
 		kfree(to_free[i]);
 #endif
+	if (!ext4_has_feature_mmp(sb) || sb_rdonly(sb))
+		ext4_stop_mmpd(sbi);
 	kfree(orig_data);
 	return err;
 }

-- 
2.31.0

On Tue 06-07-21 20:24:33, 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
Reported-by: Ye Bin <redacted>
Bug-Report-Link: [off-list ref]
Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR