Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] dlm: sysfs UAF of ls_recoverd_task during lockspace teardown - [High] dlm: module unload UAF in dlm_ls_release under DEBUG_KOBJECT_RELEASE --
commit 5dce188281954f951472555cf7284cda831b20a7 Author: Ian Rogers <[email protected]> dlm: Free dlm_ls in kobject release callback This commit adds a kobject release callback, dlm_ls_release(), to properly free the dlm_ls structure when the last kobject reference is dropped. This fixes a use-after-free issue when CONFIG_DEBUG_KOBJECT_RELEASE is enabled. > diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c > index a9c98b4f378f3..1d226b27d8e61 100644 > --- a/fs/dlm/lockspace.c > +++ b/fs/dlm/lockspace.c [ ... ] > @@ -179,7 +179,15 @@ static const struct sysfs_ops dlm_attr_ops = { > .show = dlm_attr_show, > .store = dlm_attr_store, > }; > > +static void dlm_ls_release(struct kobject *kobj) > +{ > + struct dlm_ls *ls = container_of(kobj, struct dlm_ls, ls_kobj); > + > + kfree(ls); > +} [Severity: High] Can this callback trigger an instruction page fault if the module is unloaded? Under CONFIG_DEBUG_KOBJECT_RELEASE=y, kobject release is delayed by 1-4 seconds. If the dlm module is unloaded during this window, this callback will point to unmapped memory when the delayed work executes. [ ... ] > @@ -615,7 +623,6 @@ static int new_lockspace(const char *name, const char > *cluster, > rhashtable_destroy(&ls->ls_rsbtbl); > out_lsfree: > kobject_put(&ls->ls_kobj); > - kfree(ls); > out: > module_put(THIS_MODULE); > return error; [Severity: High] In the error path of new_lockspace(), we drop the kobject reference and then immediately drop the module reference via module_put(). Doesn't this allow the module to be unloaded while the delayed kobject release is still pending? Additionally, could this sequence open a use-after-free race window? Earlier in the error path of new_lockspace(), we call dlm_recoverd_stop(): out_recoverd: dlm_recoverd_stop(ls); out_callback: ... out_lsfree: kobject_put(&ls->ls_kobj); The recovery task is stopped and freed in dlm_recoverd_stop(), but the sysfs entries aren't removed until kobject_put() is called much later. A concurrent sysfs write to the control file could invoke dlm_ls_start() in fs/dlm/member.c and wake up the freed ls->ls_recoverd_task. [ ... ] > @@ -759,7 +766,7 @@ static int release_lockspace(struct dlm_ls *ls, unsigned > int release_option) > > dlm_delete_debug_file(ls); > > - kobject_put(&ls->ls_kobj); > + kobject_del(&ls->ls_kobj); > > xa_destroy(&ls->ls_recover_xa); > kfree(ls->ls_recover_buf); [Severity: High] Is there a similar use-after-free risk involving the recovery thread here? Looking at the context in release_lockspace(), we stop the recovery thread earlier: release_lockspace() { ... dlm_recoverd_stop(ls); ... clear_bit(LSFL_RUNNING, &ls->ls_flags); ... kobject_del(&ls->ls_kobj); ... module_put(THIS_MODULE); } Since the sysfs kobject isn't deleted until after the recovery thread is stopped and its task_struct is freed, the sysfs entries remain active in the interim. A concurrent sysfs write to the control file could invoke dlm_ls_start() which calls wake_up_process(ls->ls_recoverd_task) on the freed task_struct. Also, module_put() is called at the end of release_lockspace() while the kobject delayed release is pending, repeating the module unload race mentioned above. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
