[patch] staging: speakup: safely close tty

[Okash Khawaja]

Speakup opens tty using tty_open_by_driver. When closing, it calls
tty_ldisc_release but doesn't close and remove the tty itself. As a
result, that tty cannot then be opened from user space. This patch calls
tty_release_struct which ensures that tty is safely removed and freed
up. It also calls tty_ldisc_release, so speakup doesn't need to call it.

This patch also unregisters N_SPEAKUP. It is registered when a speakup
module is loaded.

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

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

--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -300,7 +300,9 @@ void spk_ttyio_release(void)

tty_ldisc_flush(speakup_tty);
tty_unlock(speakup_tty);
- tty_ldisc_release(speakup_tty);
+ tty_release_struct(speakup_tty, speakup_tty->index);
+ if (tty_unregister_ldisc(N_SPEAKUP))
+ pr_warn("speakup: failed to unregister line discipline N_SPEAKUP\n");
}
EXPORT_SYMBOL_GPL(spk_ttyio_release);

[Okash Khawaja]

Hi,

On Fri, 7 Jul 2017 20:13:01 +0100

Post by Okash Khawaja
Speakup opens tty using tty_open_by_driver. When closing, it calls
tty_ldisc_release but doesn't close and remove the tty itself. As a
result, that tty cannot then be opened from user space. This patch calls
tty_release_struct which ensures that tty is safely removed and freed
up. It also calls tty_ldisc_release, so speakup doesn't need to call it.
This patch also unregisters N_SPEAKUP. It is registered when a speakup
module is loaded.

What happens if after you register it someone assigns that ldisc to
another tty as well ?
You should register the ldisc when the relevant module is initialized and
release it only when the module is unloaded. That way the module ref
counts will handle cases where someone uses the ldisc with something else.

Sorry if I misunderstood it. That's what we do here.
spk_ttyio_initialise_ldisc is called separately for each module (e.g.
speakup_apollo, speakup_ltlk etc) when it is loaded. spk_ttyio_release
is also called separately for each module when it is unloaded. The ldisc
stays around until the last of the modules is unloaded.

I'd also btw strongly recommend putting the ldisc and the speakup tty
driver as different modules.

Sure, that makes sense. I will do that following these patches.

Thanks,
Okash

[Okash Khawaja]

Post by Okash Khawaja
spk_ttyio_initialise_ldisc is called separately for each module (e.g.
speakup_apollo, speakup_ltlk etc) when it is loaded. spk_ttyio_release
is also called separately for each module when it is unloaded. The ldisc
stays around until the last of the modules is unloaded.

What guarantees that someone hasn't decided to set the ldisc on unrelated
hardware to speakup (eg on a pty/tty pair).

Post by Okash Khawaja

I'd also btw strongly recommend putting the ldisc and the speakup tty
driver as different modules.

Sure, that makes sense. I will do that following these patches.

If the ldisc is just unregistered when the module implementing it is
unloaded then the ref counts on the ldisc module should do everything
needed if the above isn't correctly handled, and if it is will still be
cleaner.

Right, I understand now. Thanks. I will update and resend this patch.

[Okash Khawaja]

Speakup opens tty using tty_open_by_driver. When closing, it calls
tty_ldisc_release but doesn't close and remove the tty itself. As a
result, that tty cannot be opened from user space. This patch calls
tty_release_struct which ensures that tty is safely removed and freed
up. It also calls tty_ldisc_release, so speakup doesn't need to call it.

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

---
drivers/staging/speakup/spk_ttyio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -300,7 +300,7 @@ void spk_ttyio_release(void)

tty_ldisc_flush(speakup_tty);
tty_unlock(speakup_tty);
- tty_ldisc_release(speakup_tty);
+ tty_release_struct(speakup_tty, speakup_tty->index);
}
EXPORT_SYMBOL_GPL(spk_ttyio_release);