staging: speakup: safely close tty
PATCH 1/1 staging: speakup: safely close tty
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.
--- drivers/staging/speakup/spk_ttyio.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/staging/speakup/spk_ttyio.c +3 -1
@@ -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);
Quoted 8 lines · On Fri, 7 Jul 2017 20:13:01 +0100 Okash Khawaja <[email protected]> wrote:
Speakup opens tty using
tty_open_by_driver. When closing, it callstty_ldisc_releasebut doesn't close and remove the tty itself. As a result, that tty cannot then be opened from user space. This patch callstty_release_structwhich ensures that tty is safely removed and freed up. It also callstty_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
Hi,
Quoted 12 lines · Alan Cox
Quoted 8 lines · On Fri, 7 Jul 2017 20:13:01 +0100 Okash Khawaja <[email protected]> wrote:
Speakup opens tty using
tty_open_by_driver. When closing, it callstty_ldisc_releasebut doesn't close and remove the tty itself. As a result, that tty cannot then be opened from user space. This patch callstty_release_structwhich ensures that tty is safely removed and freed up. It also callstty_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.
Quoted 6 lines
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.
Quoted 3 lines
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
Quoted 4 lines
spk_ttyio_initialise_ldiscis called separately for each module (e.g.speakup_apollo,speakup_ltlketc) when it is loaded.spk_ttyio_releaseis 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).
Quoted 5 lines
Quoted 4 lines
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
Quoted 9 lines · Alan Cox
Quoted 4 lines
spk_ttyio_initialise_ldiscis called separately for each module (e.g.speakup_apollo,speakup_ltlketc) when it is loaded.spk_ttyio_releaseis 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).
Quoted 2 lines
Quoted 2 lines
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
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.
v2 cover staging: speakup: safely close tty
In reply to Alan CoxHi,
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
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.
--- drivers/staging/speakup/spk_ttyio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/speakup/spk_ttyio.c +1 -1
@@ -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);