Re: serial console does not wake from S3 suspend

Re: serial console does not wake from S3 suspend

[Valerio Vanni]

> When resuming from S3 suspend, serial console starts sending garbage
> on the serial port.
> Not continuously, it sends garbage only when in local console prints a
> text.
> It stops only when the (sending) machine is shut down, or if some
> text is sent manually (i.e. with "cat txtfile > /dev/ttyS0").
> 
> Full details are here:
> https://bugzilla.kernel.org/show_bug.cgi?id=69751

The problem is still present in 3.14.4, 3.15-rc7 and today's next.

I hope someone is going to look at this bug.

[PATCH 1/2] serial: core: Don't drop DTR if system console

[Peter Hurley]

If a tty is opened on a serial console, don't drop DTR on
last tty close, on tty hangup, or when resetting port hardware
via TIOCSSERIAL and TIOCSERCONFIG ioctls.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/serial_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b68550d..498b149 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -239,7 +239,8 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 		/*
 		 * Turn off DTR and RTS early.
 		 */
-		if (!tty || (tty->termios.c_cflag & HUPCL))
+		if (!uart_console(uport) &&
+		    (!tty || (tty->termios.c_cflag & HUPCL)))
 			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
 
 		uart_port_shutdown(port);
-- 
1.9.1

[PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Peter Hurley]

When a tty is opened for the serial console, the termios c_cflag
settings are inherited from the console line settings.
However, if the tty is subsequently closed, the termios settings
are lost. This results in a garbled console if the console is later
suspended and resumed.

Preserve the termios c_cflag for the serial console when the tty
is shutdown; this reflects the most recent line settings.

Fixes: Bugzilla #69751, 'serial console does not wake from S3'
Reported-by: Valerio Vanni <valerio.vanni@inwind.it>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/serial/serial_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 498b149..a67861c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -239,6 +239,9 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
 		/*
 		 * Turn off DTR and RTS early.
 		 */
+		if (uart_console(uport) && tty)
+			uport->cons->cflag = tty->termios.c_cflag;
+
 		if (!uart_console(uport) &&
 		    (!tty || (tty->termios.c_cflag & HUPCL)))
 			uart_clear_mctrl(uport, TIOCM_DTR | TIOCM_RTS);
-- 
1.9.1

Re: [PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Greg Kroah-Hartman]

On Wed, Jun 04, 2014 at 10:16:11AM -0400, Peter Hurley wrote:
> When a tty is opened for the serial console, the termios c_cflag
> settings are inherited from the console line settings.
> However, if the tty is subsequently closed, the termios settings
> are lost. This results in a garbled console if the console is later
> suspended and resumed.
> 
> Preserve the termios c_cflag for the serial console when the tty
> is shutdown; this reflects the most recent line settings.
> 
> Fixes: Bugzilla #69751, 'serial console does not wake from S3'
> Reported-by: Valerio Vanni <valerio.vanni@inwind.it>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/serial_core.c | 3 +++
>  1 file changed, 3 insertions(+)

Same as before, regression, or just a normal "new feature"?

greg k-h

Re: [PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Peter Hurley]

On 06/04/2014 10:22 AM, Greg Kroah-Hartman wrote:
> On Wed, Jun 04, 2014 at 10:16:11AM -0400, Peter Hurley wrote:
>> When a tty is opened for the serial console, the termios c_cflag
>> settings are inherited from the console line settings.
>> However, if the tty is subsequently closed, the termios settings
>> are lost. This results in a garbled console if the console is later
>> suspended and resumed.
>>
>> Preserve the termios c_cflag for the serial console when the tty
>> is shutdown; this reflects the most recent line settings.
>>
>> Fixes: Bugzilla #69751, 'serial console does not wake from S3'
>> Reported-by: Valerio Vanni <valerio.vanni@inwind.it>
>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/tty/serial/serial_core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>
> Same as before, regression, or just a normal "new feature"?

This was reported as a regression since 2.6.24, but that was likely
misreported, and more likely due to a userspace update which triggers
the bug.

I would be surprised if console resume _never_ worked if the serial tty
was opened then closed, but it probably has worked for a long time.

Regards,
Peter Hurley

Re: [PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Peter Hurley]

On 06/04/2014 10:46 AM, Peter Hurley wrote:
> On 06/04/2014 10:22 AM, Greg Kroah-Hartman wrote:
>> On Wed, Jun 04, 2014 at 10:16:11AM -0400, Peter Hurley wrote:
>>> When a tty is opened for the serial console, the termios c_cflag
>>> settings are inherited from the console line settings.
>>> However, if the tty is subsequently closed, the termios settings
>>> are lost. This results in a garbled console if the console is later
>>> suspended and resumed.
>>>
>>> Preserve the termios c_cflag for the serial console when the tty
>>> is shutdown; this reflects the most recent line settings.
>>>
>>> Fixes: Bugzilla #69751, 'serial console does not wake from S3'
>>> Reported-by: Valerio Vanni <valerio.vanni@inwind.it>
>>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>   drivers/tty/serial/serial_core.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>
>> Same as before, regression, or just a normal "new feature"?
>
> This was reported as a regression since 2.6.24, but that was likely
> misreported, and more likely due to a userspace update which triggers
> the bug.
>
> I would be surprised if console resume _never_ worked if the serial tty
> was opened then closed, but it probably has worked for a long time.
                                          ^^^^^
                                          hasn't

Re: [PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Valerio Vanni]

"Peter Hurley" <peter@hurleysoftware.com> ha scritto nel messaggio
news:538F3150.6040102@hurleysoftware.com
> On 06/04/2014 10:22 AM, Greg Kroah-Hartman wrote:
>> Same as before, regression, or just a normal "new feature"?
>
> This was reported as a regression since 2.6.24, but that was likely
> misreported, and more likely due to a userspace update which triggers
> the bug.
>
> I would be surprised if console resume _never_ worked if the serial
> tty was opened then closed, but it probably has worked for a long
> time.

I don't know what was my initial mistake, but I realized very soon it was 
not a regression and took away the "regression" flag from bugzilla.
I tried many and many kernels and all were failing. I tried also to go 
down from 2.6.24 version, but I had to stop rather early because at some 
point the kernels were incompatible with distribution (I don't remember 
the details since I did these tests some month ago, and later I tested 
only newer kernel versions).

The patches you sent are for -next? for -rc? for stable?
Can I try them on 3.13.11? (at the moment I'm using this) 

Re: [PATCH 2/2] serial: core: Preserve termios c_cflag for console resume

[Valerio Vanni]

"Valerio Vanni" <valerio@valeriovanni.com> ha scritto nel messaggio
news:lmoa97$q3h$1@ger.gmane.org
> "Peter Hurley" <peter@hurleysoftware.com> ha scritto nel messaggio
> news:538F3150.6040102@hurleysoftware.com

> The patches you sent are for -next? for -rc? for stable?
> Can I try them on 3.13.11? (at the moment I'm using this)

I've tried the patches on 3.13.11, 3.14.5 and 3.15-rc8.
They work. Thanks for the fix :-)

When I tried to apply them to serial-core.c I got an error (in all three 
kernels "Hunk #1 FAILED at 239.").
I had to apply them by hand (with copy and past).

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[Greg Kroah-Hartman]

On Wed, Jun 04, 2014 at 10:16:10AM -0400, Peter Hurley wrote:
> If a tty is opened on a serial console, don't drop DTR on
> last tty close, on tty hangup, or when resetting port hardware
> via TIOCSSERIAL and TIOCSERCONFIG ioctls.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/tty/serial/serial_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Is this a regression, or something we have never done?

thanks,

greg k-h

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[Peter Hurley]

On 06/04/2014 10:22 AM, Greg Kroah-Hartman wrote:
> On Wed, Jun 04, 2014 at 10:16:10AM -0400, Peter Hurley wrote:
>> If a tty is opened on a serial console, don't drop DTR on
>> last tty close, on tty hangup, or when resetting port hardware
>> via TIOCSSERIAL and TIOCSERCONFIG ioctls.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>   drivers/tty/serial/serial_core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> Is this a regression, or something we have never done?

Not sure. I just noticed this was wrong while working on the
other fix.

Regards,
Peter Hurley

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[One Thousand Gnomes]

On Wed,  4 Jun 2014 10:16:10 -0400
Peter Hurley <peter@hurleysoftware.com> wrote:

> If a tty is opened on a serial console, don't drop DTR on
> last tty close, on tty hangup, or when resetting port hardware
> via TIOCSSERIAL and TIOCSERCONFIG ioctls.
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>

NAK

This introduces a security flaw.

If you have a system with a remote console you dial into then with this
patch applied a modem drop eg from a bad line will no longer drop any
live session and ensure a login is required as it was before.

That's a pretty bad regression case.

If you are running a serial console and want to leave DTR high either
wire the cable that way or don't set HUPCL in the first place. The
technology for fixing this problem already exists!

Alan

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[Peter Hurley]

On 06/09/2014 09:08 AM, One Thousand Gnomes wrote:
> On Wed,  4 Jun 2014 10:16:10 -0400
> Peter Hurley <peter@hurleysoftware.com> wrote:
>
>> If a tty is opened on a serial console, don't drop DTR on
>> last tty close, on tty hangup, or when resetting port hardware
>> via TIOCSSERIAL and TIOCSERCONFIG ioctls.
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>
> NAK
>
> This introduces a security flaw.
>
> If you have a system with a remote console you dial into then with this
> patch applied a modem drop eg from a bad line will no longer drop any
> live session and ensure a login is required as it was before.

There's no security flaw here.

The situation you're referring to above is managed by the CLOCAL termios
setting (which by default does _not_ hangup the tty on carrier loss).

This patch only affects the line state if the last tty reference is closed
or the tty is hung up by software (like on controlling process exit).
In this case, any login session is already dying, and it would not be possible
to hijack a live session. A successful re-login is still required.

> That's a pretty bad regression case.
>
> If you are running a serial console and want to leave DTR high either
> wire the cable that way or don't set HUPCL in the first place. The
> technology for fixing this problem already exists!

Notwithstanding what I wrote above, this patch does change behavior
with remote consoles, which may be unacceptable.

For example, if the remote user logs out, the current behavior hangs up
the modem (if HUPCL), whereas the patch behavior just presents a new
login prompt.

So yeah, I agree; this patch should be dropped.

Regards,
Peter Hurley

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[One Thousand Gnomes]

> This patch only affects the line state if the last tty reference is closed
> or the tty is hung up by software (like on controlling process exit).
> In this case, any login session is already dying, and it would not be possible
> to hijack a live session. A successful re-login is still required.

It breaks the other direction - yes sorry.
 
> Notwithstanding what I wrote above, this patch does change behavior
> with remote consoles, which may be unacceptable.
 
> For example, if the remote user logs out, the current behavior hangs up
> the modem (if HUPCL), whereas the patch behavior just presents a new
> login prompt.

which means the modem will probably no longer answer calls.

> So yeah, I agree; this patch should be dropped.

Possibly we should also fix this the other way in tty_port_shutdown,
which does wrongly skip dtr/rts handling of consoles.

As far as I can see providing HUPCL is clear the desired console
behaviour for most situations with a remote console link is obtained.

Alan

Re: [PATCH 1/2] serial: core: Don't drop DTR if system console

[Peter Hurley]

On 06/10/2014 07:01 AM, One Thousand Gnomes wrote:
>> This patch only affects the line state if the last tty reference is closed
>> or the tty is hung up by software (like on controlling process exit).
>> In this case, any login session is already dying, and it would not be possible
>> to hijack a live session. A successful re-login is still required.
>
> It breaks the other direction - yes sorry.
>
>> Notwithstanding what I wrote above, this patch does change behavior
>> with remote consoles, which may be unacceptable.
>
>> For example, if the remote user logs out, the current behavior hangs up
>> the modem (if HUPCL), whereas the patch behavior just presents a new
>> login prompt.
>
> which means the modem will probably no longer answer calls.

I would expect the vast majority of modems to be configured to hang-up
after carrier loss (when the remote user hangs up).

I think the more significant impact is that killing the login process
(by the sysadmin) wouldn't disconnect a malicious user (but would still
force the re-login). I see this as more nuisance value than any
realistic DoS.

>> So yeah, I agree; this patch should be dropped.
>
> Possibly we should also fix this the other way in tty_port_shutdown,
> which does wrongly skip dtr/rts handling of consoles.
>
> As far as I can see providing HUPCL is clear the desired console
> behaviour for most situations with a remote console link is obtained.

I agree that serial core and tty port should handle the console
identically, so if the serial core defaults are the expected behavior,
then, yes, tty_port_shutdown() should not skip dropping dtr/rts.

However, a console on USB tty is much more likely to be local, rather
than remote, so changing dtr/rts handling could impact these setups.

Since the termios c_cflag is inherited from the console, what about adding
new flow control flag decodes to uart_set_options(); eg.,
  'r'     =  CRTSCTS |  HUPCL  /* existing */
  'R'     =  CRTSCTS | !HUPCL
  default = !CRTSCTS |  HUPCL
  'd'     = !CRTSCTS | !HUPCL
?

Regards,
Peter Hurley