Re: [PATCH v3] ext4: fix use-after-free in update_super_work when racing with umount

[Jan Kara]

On Thu 19-03-26 20:03:35, Jiayuan Chen wrote:
> From: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> 
> 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 by kobject_del()
> in ext4_unregister_sysfs():
> 
>   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)
> 
> Instead of reordering the teardown sequence, fix this by making
> ext4_notify_error_sysfs() detect that sysfs has already been torn down
> by checking s_kobj.state_in_sysfs, and skipping the sysfs_notify() call
> in that case. A dedicated mutex (s_error_notify_mutex) serializes
> ext4_notify_error_sysfs() against kobject_del() in ext4_unregister_sysfs()
> to prevent TOCTOU races where the kobject could be deleted between the
> state_in_sysfs check and the sysfs_notify() call.
> 
> Fixes: b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")
> Cc: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> v2 -> v3: Fix issues reported by AI.
> https://sashiko.dev/#/patchset/20260319073651.79209-1-jiayuan.chen%40linux.dev
> 
> v1 -> v2: Use state_in_sysfs instead of reordering the teardown
> sequence.
> https://lore.kernel.org/linux-ext4/20260313065206.152645-1-jiayuan.chen@xxxxxxxxx/
> ---
>  fs/ext4/ext4.h  |  1 +
>  fs/ext4/super.c |  1 +
>  fs/ext4/sysfs.c | 10 +++++++++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b76966dc06c0..c012e85d2515 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -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;
>  
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 752f414aa06b..b2d58a9b0413 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5397,6 +5397,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
>  
>  	timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
>  	spin_lock_init(&sbi->s_error_lock);
> +	mutex_init(&sbi->s_error_notify_mutex);
>  	INIT_WORK(&sbi->s_sb_upd_work, update_super_work);
>  
>  	err = ext4_group_desc_init(sb, es, logical_sb_block, &first_not_zeroed);
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index d2ecc1026c0c..0503bf02aea1 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -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,8 +613,10 @@ int ext4_register_sysfs(struct super_block *sb)
>  	int err;
>  
>  	init_completion(&sbi->s_kobj_unregister);
> +	mutex_lock(&sbi->s_error_notify_mutex);
>  	err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, ext4_root,
>  				   "%s", sb->s_id);
> +	mutex_unlock(&sbi->s_error_notify_mutex);
>  	if (err) {
>  		kobject_put(&sbi->s_kobj);
>  		wait_for_completion(&sbi->s_kobj_unregister);
> @@ -644,7 +649,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)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR