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

[IBM]

Jiayuan Chen <jiayuan.chen@xxxxxxxxx> writes:

> 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)

Yes, the race between kobject_del() and sysfs_notify() -> kernfs_get()
is real.

and sysfs_remove_dir() explicitely says:
	 * In general, kobject owner is responsible for ensuring removal
	 * doesn't race with other operations and sysfs doesn't provide any
	 * protection

Hence this patch which adds a new mutex lock to prevent the above race,
looks good to me. So please feel free to add:

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>