speakup_key() checks the global synth before taking speakup_info.spinlock, but do_spkup() and its handlers dereference synth inside that lock: speech_kill() calls synth->is_alive(), and the flush path reaches synth_start() reading synth->alive. synth_release() clears synth under spk_mutex, which does not serialize against the key path, so a speakup key pressed while the synth is being removed can pass the unlocked check and oops on the stale pointer: CPU0 (speakup key) CPU1 (rmmod -> synth_release) ------------------ ---------------------------- speakup_key(): if (!synth) mutex_lock(&spk_mutex) return /* passes */ synth_release(): spin_lock(&speakup_info. ... drain in-flight catch_up() ... spinlock) synth->release(synth) do_spkup(): speech_kill(): synth->is_alive() /* synth whose release() already tore down its I/O */ Close the window on both sides: clear synth under speakup_info.spinlock in synth_release() and in the do_synth_init() probe-failure path, and recheck it after acquiring the lock in every reader that dereferences it there - do_spkup() at entry, the console notifier paths after their trylock, do_handle_shift(), do_handle_spec(), the speakup_key() special-handler dispatch, synth_direct_store(), and the /dev/synth and /dev/synthu write paths. The unlocked checks that used to gate these paths only duplicated the in-lock recheck, with a window a clear could still slip through, and are dropped; the only behavior difference is that an empty write to /dev/synth or /dev/synthu with no synth loaded now returns 0 instead of -ENODEV, which is harmless. The speakup thread keeps its existing fence: synth_release() drains the catch-up count before clearing. Two sites of this shape remain, preexisting. The cursor_done() timer callback dereferences synth inside the lock with no synth check at all; closing it needs the cursor_timer lifetime handled against synth teardown, which is beyond this series. The serial read buffer handler (synth_readbuf_handler()) has the same shape but cannot run in this tree: the only registration chain goes through spk_serial_synth_probe(), which has no callers, and any registration from outside the tree is still serialized by the free_irq() in the serial synth release path, which runs before the in-lock clear. Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging") Suggested-by: Samuel Thibault Cc: stable@vger.kernel.org Cc: Samuel Thibault Reviewed-by: Samuel Thibault Signed-off-by: Hongyu Xie --- drivers/accessibility/speakup/devsynth.c | 13 ++++++---- drivers/accessibility/speakup/kobjects.c | 8 +++--- drivers/accessibility/speakup/main.c | 31 ++++++++++++++++++------ drivers/accessibility/speakup/synth.c | 4 +++ 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/drivers/accessibility/speakup/devsynth.c b/drivers/accessibility/speakup/devsynth.c index e3d909bd0480..5a32d3bbf7b8 100644 --- a/drivers/accessibility/speakup/devsynth.c +++ b/drivers/accessibility/speakup/devsynth.c @@ -20,8 +20,6 @@ static ssize_t speakup_file_write(struct file *fp, const char __user *buffer, unsigned long flags; u_char buf[256]; - if (!synth) - return -ENODEV; while (count > 0) { bytes = min(count, sizeof(buf)); if (copy_from_user(buf, ptr, bytes)) @@ -29,6 +27,10 @@ static ssize_t speakup_file_write(struct file *fp, const char __user *buffer, count -= bytes; ptr += bytes; spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return -ENODEV; + } synth_write(buf, bytes); spin_unlock_irqrestore(&speakup_info.spinlock, flags); } @@ -47,9 +49,6 @@ static ssize_t speakup_file_writeu(struct file *fp, const char __user *buffer, u16 ubuf[256]; size_t in, out; - if (!synth) - return -ENODEV; - want = 1; while (count >= want) { /* Copy some UTF-8 piece from userland */ @@ -84,6 +83,10 @@ static ssize_t speakup_file_writeu(struct file *fp, const char __user *buffer, /* And speak this up */ if (out) { spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return -ENODEV; + } for (in = 0; in < out; in++) synth_buffer_add(ubuf[in]); synth_start(); diff --git a/drivers/accessibility/speakup/kobjects.c b/drivers/accessibility/speakup/kobjects.c index e1860f8d9d42..45a8e9fbd8de 100644 --- a/drivers/accessibility/speakup/kobjects.c +++ b/drivers/accessibility/speakup/kobjects.c @@ -416,9 +416,6 @@ static ssize_t synth_direct_store(struct kobject *kobj, char *unescaped; unsigned long flags; - if (!synth) - return -EPERM; - unescaped = kstrdup(buf, GFP_KERNEL); if (!unescaped) return -ENOMEM; @@ -426,6 +423,11 @@ static ssize_t synth_direct_store(struct kobject *kobj, string_unescape_any_inplace(unescaped); spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + kfree(unescaped); + return -EPERM; + } synth_write(unescaped, strlen(unescaped)); spin_unlock_irqrestore(&speakup_info.spinlock, flags); diff --git a/drivers/accessibility/speakup/main.c b/drivers/accessibility/speakup/main.c index e9b7c2761f6f..60be4d4df363 100644 --- a/drivers/accessibility/speakup/main.c +++ b/drivers/accessibility/speakup/main.c @@ -1166,9 +1166,13 @@ static void do_handle_shift(struct vc_data *vc, u_char value, char up_flag) { unsigned long flags; - if (!synth || up_flag || spk_killed) + if (up_flag || spk_killed) return; spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return; + } if (cursor_track == read_all_mode) { switch (value) { case KVAL(K_SHIFT): @@ -1763,11 +1767,15 @@ static void speakup_con_write(struct vc_data *vc, u16 *str, int len) { unsigned long flags; - if ((vc->vc_num != fg_console) || spk_shut_up || !synth) + if ((vc->vc_num != fg_console) || spk_shut_up) return; if (!spin_trylock_irqsave(&speakup_info.spinlock, flags)) /* Speakup output, discard */ return; + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return; + } if (spk_bell_pos && spk_keydown && (vc->state.x == spk_bell_pos - 1)) bleep(3); if ((is_cursor) || (cursor_track == read_all_mode)) { @@ -1792,11 +1800,15 @@ static void speakup_con_update(struct vc_data *vc) { unsigned long flags; - if (!speakup_console[vc->vc_num] || spk_parked || !synth) + if (!speakup_console[vc->vc_num] || spk_parked) return; if (!spin_trylock_irqsave(&speakup_info.spinlock, flags)) /* Speakup output, discard */ return; + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return; + } speakup_date(vc); if (vc->vc_mode == KD_GRAPHICS && !spk_paused && spk_str_pause[0]) { synth_printf("%s", spk_str_pause); @@ -1811,9 +1823,13 @@ static void do_handle_spec(struct vc_data *vc, u_char value, char up_flag) int on_off = 2; char *label; - if (!synth || up_flag || spk_killed) + if (up_flag || spk_killed) return; spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) { + spin_unlock_irqrestore(&speakup_info.spinlock, flags); + return; + } spk_shut_up &= 0xfe; if (spk_no_intr) spk_do_flush(); @@ -2087,6 +2103,8 @@ static spkup_hand spkup_handler[] = { static void do_spkup(struct vc_data *vc, u_char value) { + if (!synth) + return; if (spk_killed && value != SPEECH_KILL) return; spk_keydown = 0; @@ -2115,10 +2133,9 @@ speakup_key(struct vc_data *vc, int shift_state, int keycode, u16 keysym, u_char shift_info, offset; int ret = 0; - if (!synth) - return 0; - spin_lock_irqsave(&speakup_info.spinlock, flags); + if (!synth) + goto out; tty = vc->port.tty; if (type >= 0xf0) type -= 0xf0; diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c index 1af80eb116af..aef482f8b14e 100644 --- a/drivers/accessibility/speakup/synth.c +++ b/drivers/accessibility/speakup/synth.c @@ -519,7 +519,9 @@ static int do_synth_init(struct spk_synth *in_synth) pr_warn("synth probe\n"); if (synth->probe(synth) < 0) { pr_warn("%s: device probe failed\n", in_synth->name); + spin_lock_irqsave(&speakup_info.spinlock, flags); synth = NULL; + spin_unlock_irqrestore(&speakup_info.spinlock, flags); return -ENODEV; } synth_time_vars[0].u.n.value = @@ -585,7 +587,9 @@ void synth_release(void) for (var = synth->vars; var->var_id != MAXVARS; var++) speakup_unregister_var(var->var_id); synth->release(synth); + spin_lock_irqsave(&speakup_info.spinlock, flags); synth = NULL; + spin_unlock_irqrestore(&speakup_info.spinlock, flags); } /* called by: all_driver_init() */ -- 2.32.0