[PATCH] tty: Only hangup once

[PATCH] tty: Only hangup once

[Peter Hurley]

Instrumented testing shows a tty can be hungup multiple times [1].
Although concurrent hangups are properly serialized, multiple
hangups for the same tty should be prevented.

If tty has already been HUPPED, abort hangup. Note it is not
necessary to cleanup file *redirect on subsequent hangups,
as only TIOCCONS can set that value and ioctls are disabled
after hangup.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

[1]
Test performed by simulating a concurrent async hangup via
tty_hangup() with a sync hangup via tty_vhangup(), while
__tty_hangup() was instrumented with:

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 26bb78c..fe8b061 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -629,6 +629,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)

 	tty_lock(tty);

+	WARN_ON(test_bit(TTY_HUPPED, &tty->flags));
+
 	/* some functions below drop BTM, so we need this bit */
 	set_bit(TTY_HUPPING, &tty->flags);

Test result:

WARNING: at /home/peter/src/kernels/mainline/drivers/tty/tty_io.c:632 __tty_hangup+0x459/0x460()
Modules linked in: ip6table_filter ip6_tables ebtable_nat <...snip...>
CPU: 6 PID: 1197 Comm: kworker/6:2 Not tainted 3.10.0-0+rfcomm-xeon #0+rfcomm
Hardware name: Dell Inc. Precision WorkStation T5400  /0RW203, BIOS A11 04/30/2012
Workqueue: events do_tty_hangup
 0000000000000009 ffff8802b16d7d18 ffffffff816b553e ffff8802b16d7d58
 ffffffff810407e0 ffff880254f95c00 ffff880254f95c00 ffff8802bfd92b00
 ffff8802bfd96b00 ffff880254f95e40 0000000000000180 ffff8802b16d7d68
Call Trace:
 [<ffffffff816b553e>] dump_stack+0x19/0x1b
 [<ffffffff810407e0>] warn_slowpath_common+0x70/0xa0
 [<ffffffff8104082a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff813fb279>] __tty_hangup+0x459/0x460
 [<ffffffff8107409c>] ? finish_task_switch+0xbc/0xe0
 [<ffffffff813fb297>] do_tty_hangup+0x17/0x20
 [<ffffffff8105fd6f>] process_one_work+0x16f/0x450
 [<ffffffff8106007c>] process_scheduled_works+0x2c/0x40
 [<ffffffff8106060a>] worker_thread+0x26a/0x380
 [<ffffffff810603a0>] ? rescuer_thread+0x310/0x310
 [<ffffffff810698a0>] kthread+0xc0/0xd0
 [<ffffffff816b0000>] ? destroy_compound_page+0x65/0x92
 [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
 [<ffffffff816c495c>] ret_from_fork+0x7c/0xb0
 [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
---[ end trace 98d9f01536cf411e ]---
---
 drivers/tty/tty_io.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 26bb78c..a9355ce 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -629,6 +629,11 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 
 	tty_lock(tty);
 
+	if (test_bit(TTY_HUPPED, &tty->flags)) {
+		tty_unlock(tty);
+		return;
+	}
+
 	/* some functions below drop BTM, so we need this bit */
 	set_bit(TTY_HUPPING, &tty->flags);
 
-- 
1.8.1.2

Re: [PATCH] tty: Only hangup once

[Greg Kroah-Hartman]

On Wed, Jul 31, 2013 at 02:05:45PM -0400, Peter Hurley wrote:
> Instrumented testing shows a tty can be hungup multiple times [1].
> Although concurrent hangups are properly serialized, multiple
> hangups for the same tty should be prevented.
> 
> If tty has already been HUPPED, abort hangup. Note it is not
> necessary to cleanup file *redirect on subsequent hangups,
> as only TIOCCONS can set that value and ioctls are disabled
> after hangup.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> 
> [1]
> Test performed by simulating a concurrent async hangup via
> tty_hangup() with a sync hangup via tty_vhangup(), while
> __tty_hangup() was instrumented with:
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 26bb78c..fe8b061 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -629,6 +629,8 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
> 
>  	tty_lock(tty);
> 
> +	WARN_ON(test_bit(TTY_HUPPED, &tty->flags));
> +
>  	/* some functions below drop BTM, so we need this bit */
>  	set_bit(TTY_HUPPING, &tty->flags);
> 
> Test result:
> 
> WARNING: at /home/peter/src/kernels/mainline/drivers/tty/tty_io.c:632 __tty_hangup+0x459/0x460()
> Modules linked in: ip6table_filter ip6_tables ebtable_nat <...snip...>
> CPU: 6 PID: 1197 Comm: kworker/6:2 Not tainted 3.10.0-0+rfcomm-xeon #0+rfcomm
> Hardware name: Dell Inc. Precision WorkStation T5400  /0RW203, BIOS A11 04/30/2012
> Workqueue: events do_tty_hangup
>  0000000000000009 ffff8802b16d7d18 ffffffff816b553e ffff8802b16d7d58
>  ffffffff810407e0 ffff880254f95c00 ffff880254f95c00 ffff8802bfd92b00
>  ffff8802bfd96b00 ffff880254f95e40 0000000000000180 ffff8802b16d7d68
> Call Trace:
>  [<ffffffff816b553e>] dump_stack+0x19/0x1b
>  [<ffffffff810407e0>] warn_slowpath_common+0x70/0xa0
>  [<ffffffff8104082a>] warn_slowpath_null+0x1a/0x20
>  [<ffffffff813fb279>] __tty_hangup+0x459/0x460
>  [<ffffffff8107409c>] ? finish_task_switch+0xbc/0xe0
>  [<ffffffff813fb297>] do_tty_hangup+0x17/0x20
>  [<ffffffff8105fd6f>] process_one_work+0x16f/0x450
>  [<ffffffff8106007c>] process_scheduled_works+0x2c/0x40
>  [<ffffffff8106060a>] worker_thread+0x26a/0x380
>  [<ffffffff810603a0>] ? rescuer_thread+0x310/0x310
>  [<ffffffff810698a0>] kthread+0xc0/0xd0
>  [<ffffffff816b0000>] ? destroy_compound_page+0x65/0x92
>  [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
>  [<ffffffff816c495c>] ret_from_fork+0x7c/0xb0
>  [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
> ---[ end trace 98d9f01536cf411e ]---
> ---
>  drivers/tty/tty_io.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 26bb78c..a9355ce 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -629,6 +629,11 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
>  
>  	tty_lock(tty);
>  
> +	if (test_bit(TTY_HUPPED, &tty->flags)) {
> +		tty_unlock(tty);
> +		return;
> +	}
> +
>  	/* some functions below drop BTM, so we need this bit */
>  	set_bit(TTY_HUPPING, &tty->flags);

A diff inside the changelog entry of a diff is going to cause havoc :)

I'll go edit this to make it not complain...

thanks,

greg k-h

Re: [PATCH] tty: Only hangup once

[Peter Hurley]

On 08/01/2013 11:46 PM, Greg Kroah-Hartman wrote:
> A diff inside the changelog entry of a diff is going to cause havoc :)

Right, sorry :)  Won't happen again.

> I'll go edit this to make it not complain...

Thanks,
Peter Hurley

Re: [PATCH] tty: Only hangup once

[Heorhi Valakhanovich]

On Wed, 31 Jul 2013 14:05:45 -0400
Peter Hurley <peter@hurleysoftware.com> wrote:

> Instrumented testing shows a tty can be hungup multiple times [1].
> Although concurrent hangups are properly serialized, multiple
> hangups for the same tty should be prevented.
> 
> If tty has already been HUPPED, abort hangup. Note it is not
> necessary to cleanup file *redirect on subsequent hangups,
> as only TIOCCONS can set that value and ioctls are disabled
> after hangup.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> 
> [1]
> Test performed by simulating a concurrent async hangup via
> tty_hangup() with a sync hangup via tty_vhangup(), while
> __tty_hangup() was instrumented with:
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 26bb78c..fe8b061 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -629,6 +629,8 @@ static void __tty_hangup(struct tty_struct *tty,
> int exit_session)
> 
>  	tty_lock(tty);
> 
> +	WARN_ON(test_bit(TTY_HUPPED, &tty->flags));
> +
>  	/* some functions below drop BTM, so we need this bit */
>  	set_bit(TTY_HUPPING, &tty->flags);
> 
> Test result:
> 
> WARNING: at /home/peter/src/kernels/mainline/drivers/tty/tty_io.c:632
> __tty_hangup+0x459/0x460() Modules linked in: ip6table_filter
> ip6_tables ebtable_nat <...snip...> CPU: 6 PID: 1197 Comm:
> kworker/6:2 Not tainted 3.10.0-0+rfcomm-xeon #0+rfcomm Hardware name:
> Dell Inc. Precision WorkStation T5400  /0RW203, BIOS A11 04/30/2012
> Workqueue: events do_tty_hangup 0000000000000009 ffff8802b16d7d18
> ffffffff816b553e ffff8802b16d7d58 ffffffff810407e0 ffff880254f95c00
> ffff880254f95c00 ffff8802bfd92b00 ffff8802bfd96b00 ffff880254f95e40
> 0000000000000180 ffff8802b16d7d68 Call Trace:
>  [<ffffffff816b553e>] dump_stack+0x19/0x1b
>  [<ffffffff810407e0>] warn_slowpath_common+0x70/0xa0
>  [<ffffffff8104082a>] warn_slowpath_null+0x1a/0x20
>  [<ffffffff813fb279>] __tty_hangup+0x459/0x460
>  [<ffffffff8107409c>] ? finish_task_switch+0xbc/0xe0
>  [<ffffffff813fb297>] do_tty_hangup+0x17/0x20
>  [<ffffffff8105fd6f>] process_one_work+0x16f/0x450
>  [<ffffffff8106007c>] process_scheduled_works+0x2c/0x40
>  [<ffffffff8106060a>] worker_thread+0x26a/0x380
>  [<ffffffff810603a0>] ? rescuer_thread+0x310/0x310
>  [<ffffffff810698a0>] kthread+0xc0/0xd0
>  [<ffffffff816b0000>] ? destroy_compound_page+0x65/0x92
>  [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
>  [<ffffffff816c495c>] ret_from_fork+0x7c/0xb0
>  [<ffffffff810697e0>] ? kthread_create_on_node+0x130/0x130
> ---[ end trace 98d9f01536cf411e ]---
> ---
>  drivers/tty/tty_io.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 26bb78c..a9355ce 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -629,6 +629,11 @@ static void __tty_hangup(struct tty_struct *tty,
> int exit_session) 
>  	tty_lock(tty);
>  
> +	if (test_bit(TTY_HUPPED, &tty->flags)) {
> +		tty_unlock(tty);
> +		return;
> +	}
> +
>  	/* some functions below drop BTM, so we need this bit */
>  	set_bit(TTY_HUPPING, &tty->flags);
>  

After upgrading to kernel 3.12 I noticed one issue with tmux software.
The easiest way to reproduce will be:
1. Start tmux session as root.
2. Connect via ssh and use "tmux attach" to attach to the running
session.
3. Kill ssh client.

You can notice that shell (zsh in my case) and "tmux attach" are still
remains in process' list. That didn't happen in previous kernels.
I've tried to bisect this in kernel sources and found commit
cb50e5235b8ae5aa0fe422eaaa8e444024c5bd98 which contains this exact
patch. I have not enough experience to investigate more so most likely
I will not find anything more. But it will be good if someone more
experienced will have a look at it.

Re: [PATCH] tty: Only hangup once

[One Thousand Gnomes]

> After upgrading to kernel 3.12 I noticed one issue with tmux software.
> The easiest way to reproduce will be:
> 1. Start tmux session as root.
> 2. Connect via ssh and use "tmux attach" to attach to the running
> session.
> 3. Kill ssh client.
> 
> You can notice that shell (zsh in my case) and "tmux attach" are still
> remains in process' list. That didn't happen in previous kernels.
> I've tried to bisect this in kernel sources and found commit
> cb50e5235b8ae5aa0fe422eaaa8e444024c5bd98 which contains this exact
> patch. I have not enough experience to investigate more so most likely
> I will not find anything more. But it will be good if someone more
> experienced will have a look at it.

The patch should be reverted. The submission gives no reason that the
patch was required - it just adds code and optimises a path that doesn't
need optimising anyway.

It's theoretically true you only need one hangup, unfortunately however
I think it has to be the *last* hangup not the first or there are races
between the tty code and the process group handling.

Alan

Re: [PATCH] tty: Only hangup once

[Peter Hurley]

On 11/18/2013 08:42 AM, One Thousand Gnomes wrote:
>> After upgrading to kernel 3.12 I noticed one issue with tmux software.
>> The easiest way to reproduce will be:
>> 1. Start tmux session as root.
>> 2. Connect via ssh and use "tmux attach" to attach to the running
>> session.
>> 3. Kill ssh client.

Heorhi,

Thanks for the report.

>> You can notice that shell (zsh in my case) and "tmux attach" are still
>> remains in process' list. That didn't happen in previous kernels.

This may have been a bug in previous kernels.
The tmux(1) man page has this to say:

      Each session is persistent and will survive accidental disconnection
      (such as ssh(1) connection timeout) or intentional detaching (with the
      `C-b d' key strokes).  tmux may be reattached using:

            $ tmux attach


I'll confirm with tmux author(s) what the intended behavior is.

>> I've tried to bisect this in kernel sources and found commit
>> cb50e5235b8ae5aa0fe422eaaa8e444024c5bd98 which contains this exact
>> patch. I have not enough experience to investigate more so most likely
>> I will not find anything more. But it will be good if someone more
>> experienced will have a look at it.
>
> The patch should be reverted. The submission gives no reason that the
> patch was required - it just adds code and optimises a path that doesn't
> need optimising anyway.

Alan,

This patch isn't about optimizing; it's about guaranteeing the line discipline
and a tty driver that ops->hangup() will only occur once for any given tty.

> It's theoretically true you only need one hangup, unfortunately however
> I think it has to be the *last* hangup not the first or there are races
> between the tty code and the process group handling.

I doubt this is caused by a race condition; the first hangup would do most
of the destruction regardless, and a second hangup can't really race with
the first because of the tty_lock() held for most of the hangup.

In any event, it's worth discovering what state a subsequent hangup can
effect that the first hangup left incomplete. I'll look into it.

Regards,
Peter Hurley

Re: [PATCH] tty: Only hangup once

[Peter Hurley]

On 11/18/2013 12:37 PM, Peter Hurley wrote:
> On 11/18/2013 08:42 AM, One Thousand Gnomes wrote:
>>> After upgrading to kernel 3.12 I noticed one issue with tmux software.
>>> The easiest way to reproduce will be:
>>> 1. Start tmux session as root.
>>> 2. Connect via ssh and use "tmux attach" to attach to the running
>>> session.
>>> 3. Kill ssh client.
>
> Heorhi,
>
> Thanks for the report.
>
>>> You can notice that shell (zsh in my case) and "tmux attach" are still
>>> remains in process' list. That didn't happen in previous kernels.

Heorhi,

I could not reproduce this behavior with zsh or bash.

In case I misunderstood, the steps I took to reproduce were,

[On ssh server machine running 3.12]
1.  Add 'set-option -g default-shell /usr/bin/zsh' to /etc/tmux.conf
2a. In terminal window, 'sudo tmux'
       - also tried -
2b. At root login on tty2, 'tmux'

Successfully starts tmux session. Then,

[On ssh client machine]
1. 'ssh user@server'
2. 'sudo tmux attach'
3. Switch to 2nd terminal window,
4. ps -ef | grep ssh
5. kill $pid_from_step4

This *did not* leave the 'tmux attach' process alive.

Is zsh your login shell as well? Maybe that's the required
component to reproduce the behavior you've observed.

Regards,
Peter Hurley

Re: [PATCH] tty: Only hangup once

[Heorhi Valakhanovich]

On Mon, 18 Nov 2013 15:32:59 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> On 11/18/2013 12:37 PM, Peter Hurley wrote:
> > On 11/18/2013 08:42 AM, One Thousand Gnomes wrote:
> >>> After upgrading to kernel 3.12 I noticed one issue with tmux
> >>> software. The easiest way to reproduce will be:
> >>> 1. Start tmux session as root.
> >>> 2. Connect via ssh and use "tmux attach" to attach to the running
> >>> session.
> >>> 3. Kill ssh client.
> >
> > Heorhi,
> >
> > Thanks for the report.
> >
> >>> You can notice that shell (zsh in my case) and "tmux attach" are
> >>> still remains in process' list. That didn't happen in previous
> >>> kernels.
> 
> Heorhi,
> 
> I could not reproduce this behavior with zsh or bash.
> 
> In case I misunderstood, the steps I took to reproduce were,
> 
> [On ssh server machine running 3.12]
> 1.  Add 'set-option -g default-shell /usr/bin/zsh' to /etc/tmux.conf

This is really not necessary. I also reproduced it with bash.

> 2a. In terminal window, 'sudo tmux'
>        - also tried -
> 2b. At root login on tty2, 'tmux'
> 
> Successfully starts tmux session. Then,
> 
> [On ssh client machine]
> 1. 'ssh user@server'
> 2. 'sudo tmux attach'
> 3. Switch to 2nd terminal window,
> 4. ps -ef | grep ssh
> 5. kill $pid_from_step4
> 
> This *did not* leave the 'tmux attach' process alive.
> 
> Is zsh your login shell as well? Maybe that's the required
> component to reproduce the behavior you've observed.
> 
> Regards,
> Peter Hurley

Thank you for your attention.
I tried to reproduce it exactly as you did and notice strange thing.
You use "sudo tmux attach" and tmux client uses the terminal created
for regular user and those works good.

But if you will use "ssh root@server" (you can use localhost too) the
"tmux attach" will use terminal created from root (i checked this with
lsof and ls -al /dev/pts) and behavior is different. Are terminals
created for root is somehow different from those created for regular
users?

If you can't use root ssh login I will try to reproduce it's for sudo by
mixing some kind of dtach and tmux but it will be very unclear at the
end.

Heorhi 

Re: [PATCH] tty: Only hangup once

[Peter Hurley]

On 11/18/2013 04:09 PM, Heorhi Valakhanovich wrote:
> On Mon, 18 Nov 2013 15:32:59 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> On 11/18/2013 12:37 PM, Peter Hurley wrote:
>>> On 11/18/2013 08:42 AM, One Thousand Gnomes wrote:
>>>>> After upgrading to kernel 3.12 I noticed one issue with tmux
>>>>> software. The easiest way to reproduce will be:
>>>>> 1. Start tmux session as root.
>>>>> 2. Connect via ssh and use "tmux attach" to attach to the running
>>>>> session.
>>>>> 3. Kill ssh client.
>>>
>>> Heorhi,
>>>
>>> Thanks for the report.
>>>
>>>>> You can notice that shell (zsh in my case) and "tmux attach" are
>>>>> still remains in process' list. That didn't happen in previous
>>>>> kernels.
>>
>> Heorhi,
>>
>> I could not reproduce this behavior with zsh or bash.
>>
>> In case I misunderstood, the steps I took to reproduce were,
>>
>> [On ssh server machine running 3.12]
>> 1.  Add 'set-option -g default-shell /usr/bin/zsh' to /etc/tmux.conf
>
> This is really not necessary. I also reproduced it with bash.
>
>> 2a. In terminal window, 'sudo tmux'
>>         - also tried -
>> 2b. At root login on tty2, 'tmux'
>>
>> Successfully starts tmux session. Then,
>>
>> [On ssh client machine]
>> 1. 'ssh user@server'
>> 2. 'sudo tmux attach'
>> 3. Switch to 2nd terminal window,
>> 4. ps -ef | grep ssh
>> 5. kill $pid_from_step4
>>
>> This *did not* leave the 'tmux attach' process alive.
>>
>> Is zsh your login shell as well? Maybe that's the required
>> component to reproduce the behavior you've observed.
>>
>> Regards,
>> Peter Hurley
>
> Thank you for your attention.
> I tried to reproduce it exactly as you did and notice strange thing.
> You use "sudo tmux attach" and tmux client uses the terminal created
> for regular user and those works good.
>
> But if you will use "ssh root@server" (you can use localhost too) the
> "tmux attach" will use terminal created from root (i checked this with
> lsof and ls -al /dev/pts) and behavior is different. Are terminals
> created for root is somehow different from those created for regular
> users?
>
> If you can't use root ssh login I will try to reproduce it's for sudo by
> mixing some kind of dtach and tmux but it will be very unclear at the
> end.

Thanks for the additional testing; yes, the "ssh root@server" is the
crucial difference. I was able to reproduce the problem immediately.

Before sshd execs a new root shell, it hangs up and then re-opens
the current tty as a security measure. Because the tty state was not
being reset, the subsequent re-open could not be hung up.

Would you please test the patch below and confirm the fix?

--->%---
Subject: [PATCH] tty: Reset hupped state on open

A common security idiom is to hangup the current tty (via vhangup())
after forking but before execing a root shell. This hangs up any
existing opens which other processes may have and ensures subsequent
opens have the necessary permissions to open the root shell tty/pty.

Reset the TTY_HUPPED state after the driver has successfully
returned the opened tty (perform the reset while the tty is locked
to avoid racing with concurrent hangups).

Reported-by: Heorhi Valakhanovich <valahanovich@tut.by>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
  drivers/tty/tty_io.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3a1a01a..c74a00a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2086,6 +2086,7 @@ retry_open:
  			filp->f_op = &tty_fops;
  		goto retry_open;
  	}
+	clear_bit(TTY_HUPPED, &tty->flags);
  	tty_unlock(tty);


-- 
1.8.1.2

Re: [PATCH] tty: Only hangup once

[Heorhi Valakhanovich]

On Tue, 19 Nov 2013 08:46:27 -0500
Peter Hurley <peter@hurleysoftware.com> wrote:

> Would you please test the patch below and confirm the fix?
> 
> --->%---
> Subject: [PATCH] tty: Reset hupped state on open
> 
> A common security idiom is to hangup the current tty (via vhangup())
> after forking but before execing a root shell. This hangs up any
> existing opens which other processes may have and ensures subsequent
> opens have the necessary permissions to open the root shell tty/pty.
> 
> Reset the TTY_HUPPED state after the driver has successfully
> returned the opened tty (perform the reset while the tty is locked
> to avoid racing with concurrent hangups).
> 
> Reported-by: Heorhi Valakhanovich <valahanovich@tut.by>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>   drivers/tty/tty_io.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 3a1a01a..c74a00a 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2086,6 +2086,7 @@ retry_open:
>   			filp->f_op = &tty_fops;
>   		goto retry_open;
>   	}
> +	clear_bit(TTY_HUPPED, &tty->flags);
>   	tty_unlock(tty);
> 
> 

It looks like this patch works. It solves my problem. Thanks.
Will wait for such fix in mainline.

Heorhi.

Re: [PATCH] tty: Only hangup once

[Greg KH]

On Tue, Nov 19, 2013 at 08:19:52PM +0300, Heorhi Valakhanovich wrote:
> On Tue, 19 Nov 2013 08:46:27 -0500
> Peter Hurley <peter@hurleysoftware.com> wrote:
> 
> > Would you please test the patch below and confirm the fix?
> > 
> > --->%---
> > Subject: [PATCH] tty: Reset hupped state on open
> > 
> > A common security idiom is to hangup the current tty (via vhangup())
> > after forking but before execing a root shell. This hangs up any
> > existing opens which other processes may have and ensures subsequent
> > opens have the necessary permissions to open the root shell tty/pty.
> > 
> > Reset the TTY_HUPPED state after the driver has successfully
> > returned the opened tty (perform the reset while the tty is locked
> > to avoid racing with concurrent hangups).
> > 
> > Reported-by: Heorhi Valakhanovich <valahanovich@tut.by>
> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > ---
> >   drivers/tty/tty_io.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 3a1a01a..c74a00a 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -2086,6 +2086,7 @@ retry_open:
> >   			filp->f_op = &tty_fops;
> >   		goto retry_open;
> >   	}
> > +	clear_bit(TTY_HUPPED, &tty->flags);
> >   	tty_unlock(tty);
> > 
> > 
> 
> It looks like this patch works. It solves my problem. Thanks.
> Will wait for such fix in mainline.

I'll queue this up after 3.13-rc1 is out.

It should be backported to 3.12, and any other kernels?

thanks,

greg k-h

Re: [PATCH] tty: Only hangup once

[Peter Hurley]

On 11/19/2013 12:40 PM, Greg KH wrote:
> It should be backported to 3.12, and any other kernels?

Greg,

This patch should apply cleanly to 3.12.

The first commit to rely on the correct TTY_HUPPED state was
commit 36697529b5bbe36911e39a6309e7a7c9250d280a,
    'tty: Replace ldisc locking with ldisc_sem'
which was introduced in the 3.12 cycle.

Regards,
Peter Hurley

PS - Did you receive 'n_tty: Fix echo overrun tail computation'?
I saw it didn't make 3.12.1.

Re: [PATCH] tty: Only hangup once

[Greg KH]

On Tue, Nov 19, 2013 at 04:34:19PM -0500, Peter Hurley wrote:
> On 11/19/2013 12:40 PM, Greg KH wrote:
> > It should be backported to 3.12, and any other kernels?
> 
> Greg,
> 
> This patch should apply cleanly to 3.12.
> 
> The first commit to rely on the correct TTY_HUPPED state was
> commit 36697529b5bbe36911e39a6309e7a7c9250d280a,
>     'tty: Replace ldisc locking with ldisc_sem'
> which was introduced in the 3.12 cycle.
> 
> Regards,
> Peter Hurley
> 
> PS - Did you receive 'n_tty: Fix echo overrun tail computation'?
> I saw it didn't make 3.12.1.

Yes, I got it, I can't apply anything to my trees until 3.13-rc1 is out,
and because it's not in Linus's tree yet, I can't apply it to a stable
tree.  So give us a few weeks, don't worry, it's not lost.

greg k-h

Re: [PATCH] tty: Only hangup once

[One Thousand Gnomes]

> I doubt this is caused by a race condition; the first hangup would do most
> of the destruction regardless, and a second hangup can't really race with
> the first because of the tty_lock() held for most of the hangup.
> 
> In any event, it's worth discovering what state a subsequent hangup can
> effect that the first hangup left incomplete. I'll look into it.
> 
> Regards,
> Peter Hurley

disassociate_ctty races with tty_hangup.

tty->pgrp is only protected by ctrl_lock

See no_tty and the FIXME note. Sorry the FIXME wasn't clearer.


Alan