[patch] staging: speakup: safely close tty

[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 <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);
 

Re: [patch] staging: speakup: safely close tty

[Alan Cox]

On Fri, 7 Jul 2017 20:13:01 +0100
Okash Khawaja <okash.khawaja@gmail.com> wrote:

> 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.

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

Alan

Re: [patch] staging: speakup: safely close tty

[Okash Khawaja]

Hi,

On Mon, Jul 10, 2017 at 12:55:44PM +0100, Alan Cox wrote:
> On Fri, 7 Jul 2017 20:13:01 +0100
> Okash Khawaja <okash.khawaja@gmail.com> wrote:
> 
> > 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

Re: [patch] staging: speakup: safely close tty

[Alan Cox]

> 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).
> 
> > 
> > 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.

Alan

Re: [patch] staging: speakup: safely close tty

[Okash Khawaja]

On Wed, Jul 12, 2017 at 07:25:22PM +0100, Alan Cox wrote:
> > 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).
> > 
> > > 
> > > 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.

[patch v2 0/1] staging: speakup: safely close tty

[Okash Khawaja]

Hi,

Let's deal with the ldisc refcount problem separately. Purpose of this
patch is to close tty safely, so I have removed the call to unregister
the ldisc. I will follow this up with a separate patch which addresses
the ldisc issue.

Thanks,
Okash

[patch v2 1/1] staging: speakup: safely close tty

[Okash Khawaja]

[-- Attachment #1: 16_safely_close_tty --]
[-- Type: text/plain, Size: 829 bytes --]

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 <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);