[PATCH v2] speakup: Serialize synth clearing against locked readers

[Hongyu Xie]

speakup_key() validates the global synth pointer before taking
speakup_info.spinlock, but do_spkup() and its handlers run inside
that lock and dereference synth: 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 0  /* passes */     synth_release():
    spin_lock(&speakup_info.         synth->alive = 0   (spinlock)
        spinlock)                   ... drain in-flight catch_up() ...
                                    synth->release(synth)
    do_spkup():
      speech_kill():
        synth->is_alive()
        /* dereferences a synth whose release() has
           already torn down its I/O */

Closing this window needs both sides of the race:

- Recheck synth at do_spkup() entry, inside the lock, so the whole
  handler table runs only while a synth is still registered.
  speakup_con_write() and speakup_con_update() used to gate on synth
  before their trylock; they now recheck it after acquiring the lock
  instead, so these readers no longer validate outside the critical
  section that dereferences.

- Clear synth under speakup_info.spinlock in synth_release() and
  in the do_synth_init() probe-failure path, so a clear can never
  land between a locked reader's check and its dereference.

With the check and the clear under the same lock they are
mutually exclusive for these paths, instead of merely close
together:

    CPU0 (speakup key)             CPU1 (rmmod -> synth_release)
    ------------------             ----------------------------
    spin_lock(&speakup_info.       ... drain in-flight catch_up() ...
        spinlock)                  spin_lock(&speakup_info.spinlock)
    do_spkup():                    synth = NULL
      if (!synth)                  spin_unlock(...)
        return  /* in-lock recheck */
      speech_kill():
        synth->is_alive()  /* cannot interleave with a clear */

The console notifier paths had the same window between the
pre-trylock check and the in-lock use:

    CPU0 (vt notifier)             CPU1 (rmmod -> synth_release)
    ------------------             ----------------------------
    speakup_con_write():
      if (!synth)                  (the clear lands between
        return  /* pre-trylock */    the pre-trylock check
    spin_trylock(&speakup_info.      and the in-lock use)
        spinlock)
    ... uses synth ...  /* stale */

and now recheck synth after the trylock, in the lock, exactly
like do_spkup().

The remaining locked readers had the same window between an
unlocked check and the in-lock dereference, and gain the same
recheck:

- do_handle_shift() and do_handle_spec() used to gate on synth
  before taking speakup_info.spinlock and dereference it via
  synth_printf() inside the lock.
- speakup_key()'s special-handler dispatch dereferences synth
  inside the lock speakup_key() holds (handle_goto(),
  edit_bits(), the help handler).
- synth_direct_store() used to validate synth before the lock
  while its synth_write() call runs inside it.
- the /dev/synth and /dev/synthu write paths used to validate
  synth once before their copy_from_user() loop while every round
  calls synth_write() (or synth_buffer_add() plus
  synth_start()) inside the lock.

With these rechecks the check and the clear are mutually
exclusive for every reader that dereferences synth inside
speakup_info.spinlock, and the unlocked synth checks that used
to gate these paths are dropped: they only duplicate the in-lock
recheck, with a window a clear can still slip through.  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, and closing it needs the cursor_timer lifetime
handled against synth teardown, which is beyond this series.
The serial read buffer handler (synth_readbuf_handler()) also
dereferences synth inside the lock with no synth check.  It
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 <samuel.thibault@xxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Cc: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
Signed-off-by: Hongyu Xie <xiehongyu1@xxxxxxxxxx>
---
Changes in v2: per Samuel's review, drop the now-duplicate unlocked
synth checks on all the paths this patch rechecks inside
speakup_info.spinlock:

- speakup_file_write() and speakup_file_writeu() in devsynth.c
- synth_direct_store() in kobjects.c
- do_handle_shift() and do_handle_spec(), which now only test
  up_flag and spk_killed before taking the lock
- speakup_con_write() and speakup_con_update(), which now only test
  their other preconditions before the trylock
- speakup_key(), whose pre-lock entry check is gone; the in-lock
  recheck returns the same 0 via the existing "goto out"

The only behavior difference I could find from dropping them is that
an empty write to /dev/synth or /dev/synthu with no synth loaded now
returns 0 instead of -ENODEV, which should be harmless.

Also exercised the in-lock recheck at runtime: with the whole series
applied in a QEMU boot test, a write through a stale /dev/synth fd
kept open across the synth removal returns -ENODEV.

 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