[patch 1/1] staging: speakup: fix async usb removal

[Okash Khawaja]

When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. In case of error, it sets
synth->alive to zero and starts ttys. It also updates catch_up kthread
to check for synth->alive whenever synth_out fails. This way, when
synth_out fails, catch_up thread won't be looping forever.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/spk_ttyio.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -210,13 +210,35 @@ static int spk_ttyio_out(struct spk_synt
return 0;
}

+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n", spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+ * need to do it ourselves. Now that there is no synth we can
+ * let application flood anyway
+ */
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
+ return 0;
+}
+
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
}

@@ -257,6 +279,9 @@ static unsigned char spk_ttyio_in_nowait

static void spk_ttyio_flush_buffer(void)
{
+ if (check_tty(speakup_tty))
+ return;
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
}

[Okash Khawaja]

Hi,

Post by Okash Khawaja
When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. In case of error, it sets
synth->alive to zero and starts ttys. It also updates catch_up kthread
to check for synth->alive whenever synth_out fails. This way, when
synth_out fails, catch_up thread won't be looping forever.

Sorry this description is not correct. Last two sentences, starting from
"It also updates..." should be ignored. I'll send a v2.

Thanks,
Okash

[Okash Khawaja]

When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. In case of error, it sets
synth->alive to zero and starts ttys.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/spk_ttyio.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -210,13 +210,35 @@ static int spk_ttyio_out(struct spk_synt
return 0;
}

+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n", spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+ * need to do it ourselves. Now that there is no synth we can
+ * let application flood anyway
+ */
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
+ return 0;
+}
+
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
}

@@ -257,6 +279,9 @@ static unsigned char spk_ttyio_in_nowait

static void spk_ttyio_flush_buffer(void)
{
+ if (check_tty(speakup_tty))
+ return;
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
}

[Samuel Thibault]

Hello,

I have tested this patch with apollo and it correctly deactivates the
module when usb is asynchronously unplugged. On unplugging, the tty
becomes null so using it caused null pointer deref. When this happens
in the context of catch_up kthread, it leads to kernel lock up a little
while later.
+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n", spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+ * need to do it ourselves. Now that there is no synth we can
+ * let application flood anyway
+ */
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
+ return 0;
+}
+
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

This is still unsafe: the unplug might come just between testing for
speakup_tty becoming NULL and using it. Yes, that's very unlikely, but
that's still not acceptable :)

You should rather find out through which path the speakup_tty variable
becomes NULL, and make speakup non-alive there *before* setting it to
NULL.

Samuel

[Okash Khawaja]

Hi,

Post by Samuel Thibault

Post by Okash Khawaja
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

This is still unsafe: the unplug might come just between testing for
speakup_tty becoming NULL and using it.

I thought about that but couldn't find a way to overcome it in a
foolproof way. Here's my reasoning. Since the unplugging of usb is
asynchronous to us calling methods on speakup_tty, speakup_tty can
become null in a separate thread. In situation where usb is unplugged
immediately before we call a method on speakup_tty, even if we
deactivate speakup the method on null speakup_tty will still be called.

Have I missed something?

Thanks,
Okash

[Samuel Thibault]

Post by Okash Khawaja

Post by Samuel Thibault

Post by Okash Khawaja
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

This is still unsafe: the unplug might come just between testing for
speakup_tty becoming NULL and using it.

I thought about that but couldn't find a way to overcome it in a
foolproof way. Here's my reasoning. Since the unplugging of usb is
asynchronous to us calling methods on speakup_tty, speakup_tty can
become null in a separate thread.

Then more synchronization is needed: spk_ttyio_ldisc_close needs to
wait for methods to be finished calling. It would probably be enough
to introduce a mere mutex that would be taken around the uses of
speakup_tty, and in spk_ttyio_ldisc_close, so that it both waits for
uses of speakup_tty, and delay others from happening.

Samuel

[Okash Khawaja]

Post by Samuel Thibault

Post by Okash Khawaja

Post by Samuel Thibault

Post by Okash Khawaja
static void spk_ttyio_send_xchar(char ch)
{
+ if (check_tty(speakup_tty))
+ return;
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
}

This is still unsafe: the unplug might come just between testing for
speakup_tty becoming NULL and using it.

I thought about that but couldn't find a way to overcome it in a
foolproof way. Here's my reasoning. Since the unplugging of usb is
asynchronous to us calling methods on speakup_tty, speakup_tty can
become null in a separate thread.

Then more synchronization is needed: spk_ttyio_ldisc_close needs to
wait for methods to be finished calling. It would probably be enough
to introduce a mere mutex that would be taken around the uses of
speakup_tty, and in spk_ttyio_ldisc_close, so that it both waits for
uses of speakup_tty, and delay others from happening.

I see what you mean. Thanks for clarifying.

Okash

[Okash Khawaja]

When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. Since tty can become null
between the check and its usage, a mutex is introduced. tty usage is
now surrounded by the mutex, as is the code in speakup_ldisc_close which
sets the tty to null. The mutex also serialises calls to tty from
speakup code.

In case of tty being null, this sets synth->alive to zero and starts
ttys.

Signed-off-by: Okash Khawaja <***@gmail.com>

---
drivers/staging/speakup/spk_ttyio.c | 50 ++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)

--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -15,6 +15,11 @@ struct spk_ldisc_data {

static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+/* mutex to protect against speakup_tty disappearing from underneath us while
+ * we are using it. this can happen when the device physically unplugged,
+ * while in use. it also serialises access to speakup_tty.
+ */
+static DEFINE_MUTEX(speakup_tty_mutex);

static int ser_to_dev(int ser, dev_t *dev_no)
{
@@ -60,8 +65,10 @@ static int spk_ttyio_ldisc_open(struct t

static void spk_ttyio_ldisc_close(struct tty_struct *tty)
{
+ mutex_lock(&speakup_tty_mutex);
kfree(speakup_tty->disc_data);
speakup_tty = NULL;
+ mutex_unlock(&speakup_tty_mutex);
}

static int spk_ttyio_receive_buf2(struct tty_struct *tty,
@@ -189,9 +196,11 @@ void spk_ttyio_unregister_ldisc(void)

static int spk_ttyio_out(struct spk_synth *in_synth, const char ch)
{
+ mutex_lock(&speakup_tty_mutex);
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);

+ mutex_unlock(&speakup_tty_mutex);
if (ret == 0)
/* No room */
return 0;
@@ -207,17 +216,50 @@ static int spk_ttyio_out(struct spk_synt
}
return 1;
}
+
+ mutex_unlock(&speakup_tty_mutex);
+ return 0;
+}
+
+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n",
+ spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+ * need to do it ourselves. Now that there is no synth we can
+ * let application flood anyway
+ */
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
return 0;
}

static void spk_ttyio_send_xchar(char ch)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
+ mutex_unlock(&speakup_tty_mutex);
}

static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+ mutex_unlock(&speakup_tty_mutex);
}

static unsigned char ttyio_in(int timeout)
@@ -257,8 +299,16 @@ static unsigned char spk_ttyio_in_nowait

static void spk_ttyio_flush_buffer(void)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
+
+ mutex_unlock(&speakup_tty_mutex);
}

int spk_ttyio_synth_probe(struct spk_synth *synth)

[Samuel Thibault]

Hello,

Post by Okash Khawaja
When an external USB synth is unplugged while the module is loaded, we
get a null pointer deref. This is because the tty disappears while
speakup tries to use to to communicate with the synth. This patch fixes
it by checking tty for null before using it. Since tty can become null
between the check and its usage, a mutex is introduced. tty usage is
now surrounded by the mutex, as is the code in speakup_ldisc_close which
sets the tty to null. The mutex also serialises calls to tty from
speakup code.
In case of tty being null, this sets synth->alive to zero and starts
ttys.

Perhaps rather say "restarts ttys in case they were stopped by speakup".

Post by Okash Khawaja
---
drivers/staging/speakup/spk_ttyio.c | 50 ++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -15,6 +15,11 @@ struct spk_ldisc_data {
static struct spk_synth *spk_ttyio_synth;
static struct tty_struct *speakup_tty;
+/* mutex to protect against speakup_tty disappearing from underneath us while
+ * we are using it. this can happen when the device physically unplugged,
+ * while in use. it also serialises access to speakup_tty.
+ */
+static DEFINE_MUTEX(speakup_tty_mutex);
static int ser_to_dev(int ser, dev_t *dev_no)
{
@@ -60,8 +65,10 @@ static int spk_ttyio_ldisc_open(struct t
static void spk_ttyio_ldisc_close(struct tty_struct *tty)
{
+ mutex_lock(&speakup_tty_mutex);
kfree(speakup_tty->disc_data);
speakup_tty = NULL;
+ mutex_unlock(&speakup_tty_mutex);
}
static int spk_ttyio_receive_buf2(struct tty_struct *tty,
@@ -189,9 +196,11 @@ void spk_ttyio_unregister_ldisc(void)
static int spk_ttyio_out(struct spk_synth *in_synth, const char ch)
{
+ mutex_lock(&speakup_tty_mutex);
if (in_synth->alive && speakup_tty && speakup_tty->ops->write) {
int ret = speakup_tty->ops->write(speakup_tty, &ch, 1);
+ mutex_unlock(&speakup_tty_mutex);
if (ret == 0)
/* No room */
return 0;
@@ -207,17 +216,50 @@ static int spk_ttyio_out(struct spk_synt
}
return 1;
}
+
+ mutex_unlock(&speakup_tty_mutex);
+ return 0;
+}
+
+static int check_tty(struct tty_struct *tty)
+{
+ if (!tty) {
+ pr_warn("%s: I/O error, deactivating speakup\n",
+ spk_ttyio_synth->long_name);
+ /* No synth any more, so nobody will restart TTYs, and we thus
+ * need to do it ourselves. Now that there is no synth we can
+ * let application flood anyway
+ */
+ spk_ttyio_synth->alive = 0;
+ speakup_start_ttys();
+ return 1;
+ }
+
return 0;
}
static void spk_ttyio_send_xchar(char ch)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->send_xchar(speakup_tty, ch);
+ mutex_unlock(&speakup_tty_mutex);
}
static void spk_ttyio_tiocmset(unsigned int set, unsigned int clear)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
speakup_tty->ops->tiocmset(speakup_tty, set, clear);
+ mutex_unlock(&speakup_tty_mutex);
}
static unsigned char ttyio_in(int timeout)
@@ -257,8 +299,16 @@ static unsigned char spk_ttyio_in_nowait
static void spk_ttyio_flush_buffer(void)
{
+ mutex_lock(&speakup_tty_mutex);
+ if (check_tty(speakup_tty)) {
+ mutex_unlock(&speakup_tty_mutex);
+ return;
+ }
+
if (speakup_tty->ops->flush_buffer)
speakup_tty->ops->flush_buffer(speakup_tty);
+
+ mutex_unlock(&speakup_tty_mutex);
}
int spk_ttyio_synth_probe(struct spk_synth *synth)