ext4: fix use-after-free in update_super_work when racing with umount

Commit b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem") moved ext4_unregister_sysfs() before flushing s_sb_upd_work to prevent new error work from being queued via /proc/fs/ext4/xx/mb_groups reads during unmount. However, this introduced a use-after-free because update_super_work calls ext4_notify_error_sysfs() -> sysfs_notify() which accesses the kobject's kernfs_node after it has been freed:

update_super_work ext4_put_super
----------------- --------------

                                   ext4_unregister_sysfs(sb)
                                     kobject_del(&sbi->s_kobj)
                                       __kobject_del()
                                         sysfs_remove_dir()
                                           kobj->sd = NULL
                                         sysfs_put(sd)
                                           kernfs_put()  // RCU free
  ext4_notify_error_sysfs(sbi)
    sysfs_notify(&sbi->s_kobj)
      kn = kobj->sd              // stale pointer
      kernfs_get(kn)             // UAF on freed kernfs_node
                                   ext4_journal_destroy()
                                     flush_work(&sbi->s_sb_upd_work)

The original blamed commit needed procfs removal before the work flush to prevent /proc/fs/ext4/xx/mb_groups reads from queuing new error work. But it bundled procfs removal and kobject_del together in ext4_unregister_sysfs(), causing the sysfs kobject to be torn down too early.

The correct teardown ordering has three constraints:

1. procfs removal must happen before flushing s_sb_upd_work, to prevent /proc reads from queuing new error work that would BUG_ON.
2. sysfs kobject removal must happen after flushing s_sb_upd_work, since the work calls sysfs_notify() which accesses the kernfs_node.
3. sysfs kobject removal must happen before jbd2_journal_destroy(), since userspace could read the journal_task sysfs attribute and dereference j_task after the journal thread has been killed.

Fix this by:
- Adding ext4_sb_release_proc() to remove procfs entries early.
- Splitting ext4_journal_destroy() into ext4_journal_stop_work() and ext4_journal_finish(), so that ext4_unregister_sysfs() can be placed between them to satisfy all three ordering constraints.

Fixes b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")

Cc Jiayuan Chen <[email protected]>

Signed-off-by Jiayuan Chen <[email protected]>

---
 fs/ext4/ext4.h      |  1 +
 fs/ext4/ext4_jbd2.h | 45 ++++++++++++++++++++++++++++-----------------
 fs/ext4/super.c     | 34 +++++++++++++++++++++++-----------
 fs/ext4/sysfs.c     | 10 ++++++++++
 4 files changed, 62 insertions(+), 28 deletions(-)

fs/ext4/ext4.h +1 -0

index b76966dc06c0..a693365d224c 100644
@@ -3757,6 +3757,7 @@ extern const struct inode_operations ext4_fast_symlink_inode_operations;
 /* sysfs.c */
 extern void ext4_notify_error_sysfs(struct ext4_sb_info *sbi);
 extern int ext4_register_sysfs(struct super_block *sb);
+extern void ext4_sb_release_proc(struct super_block *sb);
 extern void ext4_unregister_sysfs(struct super_block *sb);
 extern int __init ext4_init_sysfs(void);
 extern void ext4_exit_sysfs(void);

fs/ext4/ext4_jbd2.h +28 -17

index 63d17c5201b5..2b7d68b11578 100644
@@ -430,32 +430,43 @@ static inline int ext4_should_dioread_nolock(struct inode *inode)
 }
 
 /*
- * Pass journal explicitly as it may not be cached in the sbi->s_journal in some
- * cases
+ * Stop new s_sb_upd_work from being queued and flush any pending work.
+ *
+ * At this point only two things can be operating on the journal:
+ * JBD2 thread performing transaction commit and s_sb_upd_work
+ * issuing sb update through the journal. Once we set
+ * EXT4_MF_JOURNAL_DESTROY, new ext4_handle_error() calls will not
+ * queue s_sb_upd_work and ext4_force_commit() makes sure any
+ * ext4_handle_error() calls from the running transaction commit are
+ * finished. Hence no new s_sb_upd_work can be queued after we
+ * flush it here.
  */
-static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *journal)
+static inline void ext4_journal_stop_work(struct ext4_sb_info *sbi)
 {
-	int err = 0;
-
-	/*
-	 * At this point only two things can be operating on the journal.
-	 * JBD2 thread performing transaction commit and s_sb_upd_work
-	 * issuing sb update through the journal. Once we set
-	 * EXT4_JOURNAL_DESTROY, new ext4_handle_error() calls will not
-	 * queue s_sb_upd_work and ext4_force_commit() makes sure any
-	 * ext4_handle_error() calls from the running transaction commit are
-	 * finished. Hence no new s_sb_upd_work can be queued after we
-	 * flush it here.
-	 */
 	ext4_set_mount_flag(sbi->s_sb, EXT4_MF_JOURNAL_DESTROY);
-
 	ext4_force_commit(sbi->s_sb);
 	flush_work(&sbi->s_sb_upd_work);
+}
+
+/*
+ * Destroy the journal. Must be called after ext4_journal_stop_work().
+ * Pass journal explicitly as it may not be cached in the sbi->s_journal
+ * in some cases.
+ */
+static inline int ext4_journal_finish(struct ext4_sb_info *sbi,
+				      journal_t *journal)
+{
+	int err;
 
 	err = jbd2_journal_destroy(journal);
 	sbi->s_journal = NULL;
-
 	return err;
 }
 
+static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *journal)
+{
+	ext4_journal_stop_work(sbi);
+	return ext4_journal_finish(sbi, journal);
+}
+
 #endif	/* _EXT4_JBD2_H */

fs/ext4/super.c +23 -11

index 752f414aa06b..9bba783f44e1 100644
@@ -1280,16 +1280,12 @@ static void ext4_put_super(struct super_block *sb)
 	int err;
 
 	/*
-	 * Unregister sysfs before destroying jbd2 journal.
-	 * Since we could still access attr_journal_task attribute via sysfs
-	 * path which could have sbi->s_journal->j_task as NULL
-	 * Unregister sysfs before flush sbi->s_sb_upd_work.
-	 * Since user may read /proc/fs/ext4/xx/mb_groups during umount, If
-	 * read metadata verify failed then will queue error work.
-	 * update_super_work will call start_this_handle may trigger
-	 * BUG_ON.
+	 * Remove procfs entries before flush s_sb_upd_work. Since user may
+	 * read /proc/fs/ext4/xx/mb_groups during umount, if read metadata
+	 * verify failed then will queue error work. update_super_work will
+	 * call start_this_handle which may trigger BUG_ON.
 	 */
-	ext4_unregister_sysfs(sb);
+	ext4_sb_release_proc(sb);
 
 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs unmount"))
 		ext4_msg(sb, KERN_INFO, "unmounting filesystem %pU.",

@@ -1301,14 +1297,30 @@ static void ext4_put_super(struct super_block *sb)
 	destroy_workqueue(sbi->rsv_conversion_wq);
 	ext4_release_orphan_info(sb);
 
+	/*
+	 * Flush s_sb_upd_work before unregistering sysfs, since
+	 * update_super_work calls ext4_notify_error_sysfs() which accesses
+	 * the kobject's kernfs_node via sysfs_notify(). Unregistering sysfs
+	 * before the flush could lead to a use-after-free on the
+	 * kernfs_node.
+	 *
+	 * Also unregister sysfs before destroying jbd2 journal, since
+	 * userspace could read the journal_task sysfs attribute while
+	 * jbd2_journal_destroy() is killing the journal thread, leading to
+	 * a NULL pointer dereference of j_task in journal_task_show().
+	 */
 	if (sbi->s_journal) {
 		aborted = is_journal_aborted(sbi->s_journal);
-		err = ext4_journal_destroy(sbi, sbi->s_journal);
+		ext4_journal_stop_work(sbi);
+		ext4_unregister_sysfs(sb);
+		err = ext4_journal_finish(sbi, sbi->s_journal);
 		if ((err < 0) && !aborted) {
 			ext4_abort(sb, -err, "Couldn't clean up the journal");
 		}
-	} else
+	} else {
 		flush_work(&sbi->s_sb_upd_work);
+		ext4_unregister_sysfs(sb);
+	}
 
 	ext4_es_unregister_shrinker(sbi);
 	timer_shutdown_sync(&sbi->s_err_report);

fs/ext4/sysfs.c +10 -0

index d2ecc1026c0c..f6947416c1e7 100644
@@ -638,6 +638,16 @@ int ext4_register_sysfs(struct super_block *sb)
 	return 0;
 }
 
+void ext4_sb_release_proc(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	if (sbi->s_proc) {
+		remove_proc_subtree(sb->s_id, ext4_proc_root);
+		sbi->s_proc = NULL;
+	}
+}
+
 void ext4_unregister_sysfs(struct super_block *sb)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(sb);

Signature · 1 line

2.43.0

Quoted 45 lines · Jiayuan Chen

Commit b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem") moved ext4_unregister_sysfs() before flushing s_sb_upd_work to prevent new error work from being queued via /proc/fs/ext4/xx/mb_groups reads during unmount. However, this introduced a use-after-free because update_super_work calls ext4_notify_error_sysfs() -> sysfs_notify() which accesses the kobject's kernfs_node after it has been freed:

   update_super_work                ext4_put_super
   -----------------                --------------
                                    ext4_unregister_sysfs(sb)
                                      kobject_del(&sbi->s_kobj)
                                        __kobject_del()
                                          sysfs_remove_dir()
                                            kobj->sd = NULL
                                          sysfs_put(sd)
                                            kernfs_put()  // RCU free
   ext4_notify_error_sysfs(sbi)
     sysfs_notify(&sbi->s_kobj)
       kn = kobj->sd              // stale pointer
       kernfs_get(kn)             // UAF on freed kernfs_node
                                    ext4_journal_destroy()
                                      flush_work(&sbi->s_sb_upd_work)

The original blamed commit needed procfs removal before the work flush to prevent /proc/fs/ext4/xx/mb_groups reads from queuing new error work. But it bundled procfs removal and kobject_del together in ext4_unregister_sysfs(), causing the sysfs kobject to be torn down too early.

The correct teardown ordering has three constraints:

1. procfs removal must happen before flushing s_sb_upd_work, to prevent /proc reads from queuing new error work that would BUG_ON.
2. sysfs kobject removal must happen after flushing s_sb_upd_work, since the work calls sysfs_notify() which accesses the kernfs_node.
3. sysfs kobject removal must happen before jbd2_journal_destroy(), since userspace could read the journal_task sysfs attribute and dereference j_task after the journal thread has been killed.

Fix this by:
- Adding ext4_sb_release_proc() to remove procfs entries early.
- Splitting ext4_journal_destroy() into ext4_journal_stop_work() and ext4_journal_finish(), so that ext4_unregister_sysfs() can be placed between them to satisfy all three ordering constraints.

Quoted 3 lines

Fixes b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")

Cc Jiayuan Chen <[email protected]>

Signed-off-by Jiayuan Chen <[email protected]>

Thanks for the analysis and the patch! I fully agree with your analysis but I think your solution shows that the teardown sequence is just too subtle (too many different dependencies). Ideally, we'd instead modify ext4_notify_error_sysfs() to detect sysfs is already torn down by ext4_unregister_sysfs() and do nothing. We can check sbi->s_kobj.state_in_sysfs for that. The only trouble with this is that sysfs_notify() could still race with kobject_del() so we also need some locking in ext4_unregister_sysfs() locking out parallel ext4_notify_error_sysfs() and we probably need to introduce a separate mutex for that. What do you think?

Honza

Quoted 60 lines · Jan Kara

Quoted 45 lines · Jiayuan Chen

Commit b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem") moved ext4_unregister_sysfs() before flushing s_sb_upd_work to prevent new error work from being queued via /proc/fs/ext4/xx/mb_groups reads during unmount. However, this introduced a use-after-free because update_super_work calls ext4_notify_error_sysfs() -> sysfs_notify() which accesses the kobject's kernfs_node after it has been freed:

   update_super_work                ext4_put_super
   -----------------                --------------
                                    ext4_unregister_sysfs(sb)
                                      kobject_del(&sbi->s_kobj)
                                        __kobject_del()
                                          sysfs_remove_dir()
                                            kobj->sd = NULL
                                          sysfs_put(sd)
                                            kernfs_put()  // RCU free
   ext4_notify_error_sysfs(sbi)
     sysfs_notify(&sbi->s_kobj)
       kn = kobj->sd              // stale pointer
       kernfs_get(kn)             // UAF on freed kernfs_node
                                    ext4_journal_destroy()
                                      flush_work(&sbi->s_sb_upd_work)

The original blamed commit needed procfs removal before the work flush to prevent /proc/fs/ext4/xx/mb_groups reads from queuing new error work. But it bundled procfs removal and kobject_del together in ext4_unregister_sysfs(), causing the sysfs kobject to be torn down too early.

The correct teardown ordering has three constraints:

1. procfs removal must happen before flushing s_sb_upd_work, to prevent /proc reads from queuing new error work that would BUG_ON.
2. sysfs kobject removal must happen after flushing s_sb_upd_work, since the work calls sysfs_notify() which accesses the kernfs_node.
3. sysfs kobject removal must happen before jbd2_journal_destroy(), since userspace could read the journal_task sysfs attribute and dereference j_task after the journal thread has been killed.

Fix this by:
- Adding ext4_sb_release_proc() to remove procfs entries early.
- Splitting ext4_journal_destroy() into ext4_journal_stop_work() and ext4_journal_finish(), so that ext4_unregister_sysfs() can be placed between them to satisfy all three ordering constraints.

Quoted 3 lines

Fixes b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")

Cc Jiayuan Chen <[email protected]>

Signed-off-by Jiayuan Chen <[email protected]>

Thanks for the analysis and the patch! I fully agree with your analysis but I think your solution shows that the teardown sequence is just too subtle (too many different dependencies). Ideally, we'd instead modify ext4_notify_error_sysfs() to detect sysfs is already torn down by ext4_unregister_sysfs() and do nothing. We can check sbi->s_kobj.state_in_sysfs for that. The only trouble with this is that sysfs_notify() could still race with kobject_del() so we also need some locking in ext4_unregister_sysfs() locking out parallel ext4_notify_error_sysfs() and we probably need to introduce a separate mutex for that. What do you think?

Quoted 2 lines

Honza

Hi Honza,

Thanks for the suggestion !

I tried the approach you described — having ext4_notify_error_sysfs() check s_kobj.state_in_sysfs and using a
dedicated mutex to serialize against kobject_del() in ext4_unregister_sysfs(). It is indeed much simpler than my original approach of reordering the teardown sequence.

fs/ext4/ext4.h +1 -0

index b76966dc06c0..c012e85d2515 100644
@@ -1574,6 +1574,7 @@ struct ext4_sb_info {
         struct proc_dir_entry *s_proc;
         struct kobject s_kobj;
         struct completion s_kobj_unregister;
+       struct mutex s_error_notify_mutex; /* protects sysfs_notify vs

kobject_del */
        struct super_block *s_sb;
        struct buffer_head *s_mmp_bh;

fs/ext4/sysfs.c +8 -1

index d2ecc1026c0c..f2505988e010 100644
@@ -597,7 +597,10 @@ static const struct kobj_type ext4_feat_ktype = {
 
  void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)
  {
-       sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
+       mutex_lock(&sbi->s_error_notify_mutex);
+       if (sbi->s_kobj.state_in_sysfs)
+               sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
+       mutex_unlock(&sbi->s_error_notify_mutex);
  }
 
  static struct kobject *ext4_root;

@@ -610,6 +613,7 @@ int ext4_register_sysfs(struct super_block *sb)
         int err;
 
         init_completion(&sbi->s_kobj_unregister);
+       mutex_init(&sbi->s_error_notify_mutex);
         err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, ext4_root,
                                    "%s", sb->s_id);
         if (err) {

@@ -644,7 +648,10 @@ void ext4_unregister_sysfs(struct super_block *sb)
 
         if (sbi->s_proc)
                 remove_proc_subtree(sb->s_id, ext4_proc_root);
+
+       mutex_lock(&sbi->s_error_notify_mutex);
         kobject_del(&sbi->s_kobj);
+       mutex_unlock(&sbi->s_error_notify_mutex);
  }
 
  int __init ext4_init_sysfs(void)