Re: [PATCH 3/4] speakup: Guard late sysfs operations on speakup_kobj

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

Hongyu Xie, le mer. 14 oct. 2026 15:17:12 +0800, a ecrit:
> speakup_kobj_init() drops the kobject on its error path and
> speakup_kobj_exit() drops it on removal, but neither clears the
> global pointer.  When built-in speakup fails late in speakup_init()
> (e.g. a kthread_create() failure, whose error path runs
> speakup_kobj_exit()), or when the kobject setup itself fails, the
> dangling speakup_kobj stays behind: a later insmod of a synth driver
> then reaches sysfs_create_group() through synth_add()->
> do_synth_init() and dereferences the freed kobject:
> 
>     boot (built-in speakup)         later insmod (synth driver)
>     --------------------------      ---------------------------
>     speakup_init() fails late:
>       err_speakup:
>         kobject_put(speakup_kobj);
>         /* speakup_kobj stays set */
> 
>                                     synth_add() -> do_synth_init()
>                                       sysfs_create_group(speakup_kobj,
>                                                          &attributes)
>                                       /* freed kobject: UAF */
> 
> Clear the pointer right after kobject_put() so the late
> sysfs_create_group() fails cleanly on the NULL check instead of
> touching freed memory:
> 
>       err_speakup:
>         kobject_put(speakup_kobj);
>         speakup_kobj = NULL;
> 
>                                     sysfs_create_group(NULL, ...)
>                                       /* fails cleanly: the NULL
>                                          check is sysfs's own */
> 
> The removal side needs the same guard: when the kobject setup
> failed, a first insmod of a synth driver fails in do_synth_init()
> (synth stays set because that error path does not roll it back),
> and a second insmod then goes through synth_release(), whose
> sysfs_remove_group() would now meet speakup_kobj == NULL and crash
> on the NULL kobject:
> 
>     first insmod                    second insmod
>     -----------                     ------------
>     do_synth_init():
>       synth = in_synth;
>       probe() fails                 synth_release():
>       /* no rollback:                sysfs_remove_group(
>          synth stays set */            speakup_kobj == NULL, ...)
>                                       /* NULL kobject: the crash
>                                          the NULL clear above would
>                                          itself introduce without
>                                          the guard below */
> 
> Skip the group removal when the kobject is gone, mirroring the
> NULL check that sysfs_create_group() already performs on its own:
> 
>                                     if (synth->attributes.name &&
>                                         speakup_kobj)
>                                       sysfs_remove_group(...)
>                                       /* skipped: the kobject is
>                                          gone */
> 
> Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
> Cc: stable@vger.kernel.org
> Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Thanks!

Samuel

> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
>  drivers/accessibility/speakup/kobjects.c | 2 ++
>  drivers/accessibility/speakup/synth.c    | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c
> index a73b478e06de0..e1860f8d9d422 100644
> --- a/drivers/accessibility/speakup/kobjects.c
> +++ b/drivers/accessibility/speakup/kobjects.c
> @@ -1041,6 +1041,7 @@ int speakup_kobj_init(void)
>  	sysfs_remove_group(speakup_kobj, &main_attr_group);
>  err_speakup:
>  	kobject_put(speakup_kobj);
> +	speakup_kobj = NULL;
>  err_acc:
>  	kobject_put(accessibility_kobj);
>  out:
> @@ -1052,5 +1053,6 @@ void speakup_kobj_exit(void)
>  	sysfs_remove_group(speakup_kobj, &i18n_attr_group);
>  	sysfs_remove_group(speakup_kobj, &main_attr_group);
>  	kobject_put(speakup_kobj);
> +	speakup_kobj = NULL;
>  	kobject_put(accessibility_kobj);
>  }
> diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
> index 40870949b3c6b..1af80eb116af6 100644
> --- a/drivers/accessibility/speakup/synth.c
> +++ b/drivers/accessibility/speakup/synth.c
> @@ -580,7 +580,7 @@ void synth_release(void)
>  		wake_up_process(task);
>  	rcu_read_unlock();
>  	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
> -	if (synth->attributes.name)
> +	if (synth->attributes.name && speakup_kobj)
>  		sysfs_remove_group(speakup_kobj, &synth->attributes);
>  	for (var = synth->vars; var->var_id != MAXVARS; var++)
>  		speakup_unregister_var(var->var_id);
> -- 
> 2.25.1
> 

-- 
Samuel
<D> m'enfin, le 5 juillet, le mec vient visiter le labo...
* D a marque d'une croix rouge le 5 juillet sur son agenda
<y> niarc niarc niarc
<D> cet homme va souffrir
<B> c'est donc le 5 juillet qu'il meurt d'un accident de la route écrasé par un truck muni d'un pare buffle
 -+- #ens-mim - repaire de terroristes -+-