Message-ID: <54BD5E54.3050909@symas.com>
Date:	Mon, 19 Jan 2015 19:43:16 +0000
From:	Howard Chu <hyc@...as.com>
To:	Peter Hurley <peter@...leysoftware.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Jiri Slaby <jslaby@...e.cz>,
	Linux Kernel Mailing List <Linux-Kernel@...r.Kernel.ORG>,
	linux-serial@...r.kernel.org, Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH] n_tty: Remove LINEMODE support

Peter Hurley wrote:
> On 01/19/2015 11:36 AM, Howard Chu wrote:
>> Peter Hurley wrote:
>>> Thanks, Howard.
>>>
>>> [ adding Ted too ]
>>>
>>> On 01/19/2015 07:46 AM, Howard Chu wrote:
>>>> Peter Hurley wrote:
>>>>> On 01/18/2015 05:45 PM, Howard Chu wrote:
>>>>>> Peter Hurley wrote:
>>>>>>> Commit 26df6d13406d1 ("tty: Add EXTPROC support for LINEMODE") added
>>>>>>> the undocumented EXTPROC input processing mode, which ignores the ICANON
>>>>>>> setting and forces pty slave input to be processed in non-canonical
>>>>>>> mode.
>>>>>>
>>>>>> What's the motivation to remove this code, rather than improve it if it
>>>>>> needs fixing? It has been removed from the Linux kernel at least once
>>>>>> before already, and that was a mistake back then too.
>>>>>
>>>>> It is a significant maintenance burden, and I have concerns about the
>>>>> level of support it's receiving. Here's some outstanding issues:
>>>>>
>>>>> 1. No man page documentation. At a minimum, tty_ioctl(4) and termios(3)
>>>>>       need the userspace visible definitions and behavior documented. Better
>>>>>       would be a LINEMODE (7) description of how this implementation works
>>>>>       wrt supporting RFC 1116.
>>>>
>>>> That can be added easily enough.
>>>
>>> Is this you signing up?  :)
>>
>> Sure. I suppose the obvious place to add it is near the TIOCPKT doc in tty_ioctl(4). But, about your further comment:
>
> Thanks.
>
>>> Which brings up another point: only a pty master should be able to set EXTPROC
>>> mode. Right now, any tty can be set to EXTPROC and the pty slave can even
>>> accidentally unset it. This argues for a new pty ioctl() to set EXTPROC in
>>> termios. pty_set_termios() can silently merge the bit.
>>
>> Historically EXTPROC could also be set on regular ttys, for lines that were hooked up to special terminal interfaces. E.g. X.29 PADs that did their own input processing. I'm not sure how relevant this is now, but this is the reason EXTPROC is not a pty-specific ioctl.
>
> How does the remote end find out when non-canon mode is selected?
> And who does the EXTPROC setting, because if it's the program that opened the
> tty, then it can just select raw mode instead.

There's a few significant differences in semantics between raw mode and 
EXTPROC mode. The point of EXTPROC is to maintain the tty driver state 
as if cooked mode were still in effect. One obvious difference is that 
VMIN/VTIME are overlaid on VEOL/VEOF in raw mode, so the two are not 
directly interchangeable.

The fact that EXTPROC can be manually unset is by design. Quoting from 
the original again:

> stty.diff:
>     This file contains the changes needed for the stty(1) program
>     to report on the current status of the TS_EXTPROC bit.  It also
>     allows the user to turn on/off the TS_EXTPROC bit.  This is useful
>     because it allows the user to say "stty -extproc", and the
>     LINEMODE option will be automatically disabled, and saying "stty
>     extproc" will re-enable the LINEMODE option.


>>>>> 6. EXTPROC still does some input processing on the server. For example,
>>>>>       7-bit mode (ISTRIP), tolower mode (IUCLC) and processing while
>>>>>       closing; if input processing is being done on the local/client side,
>>>>>       why the extra work here?
>>>>
>>>> That's defensive, on the assumption that something else might break if e.g. the tty expected only 7-bit input but 8-bit characters were sent to it.
>>>
>>> Ok, is that because RFC 1116 doesn't specify ISTRIP and IUCLC handling so
>>> the server can't be sure the client did it? If so, that should be documented
>>> so that refactors don't remove that handling.
>
> Could you get back to me about this, as well?

The telnet protocol (RFC854) defines a Network Virtual Terminal (NVT) as 
using 7-bit USASCII in an 8-bit field. As such, it expects the client to 
be able to generate both upper and lower case itself, so there's no 
analogue to IUCLC, and there would be no reason to use ISTRIP.

RFC5198 updates the protocols to use UTF8. So again, it assumes full 
octets are being transmitted.

Perhaps we can drop these special cases from the driver.

>>> Regular ptys dump input while closing so this should too.
>>>
>>>>> 7. This needs a reference userspace implementation which for the moment
>>>>>       could double as regression testing. A library with unit tests would
>>>>>       be ideal.
>>>>
>>>> telnet/telnetd can probably used as a starting point for this.
>>>
>>> Except telnet is unmaintained except by each distro. Your patches don't apply
>>> to my distro telnet; they had been applied but are now ifdef'd out. I messed
>>> around with rebuilding it but never got telnetd to actually set EXTPROC.
>>>
>>> My point is there is no straightforward way to test this, and that's a problem.
>>>
>>> In fact, this is my main concern: no matter how useful this might be, it's
>>> pointless if the complementary userspace puzzle piece is unsupported.
>>> I know you have written patches but that's not the same as support; for example
>>> your OpenSSH fork is 4 years old, which is a non-starter because I'm sure
>>> there's known exploits which have been fixed in those 4 years.
>>>
>>> Why doesn't Ubuntu/Debian telnet even compile for line mode support? Did the
>>> package maintainer shut it off because it was causing bug reports?
>>
>> I don't know. Looking at the package history it appears they have never enabled it. https://launchpad.net/ubuntu/+source/netkit-telnet/+changelog
>>
>> I also don't know where the actual VCS repo is for this code.
>
> There isn't one. The base code is from downloaded tarball.
>
> I don't know what distro you use (or if you even do), but for Ubuntu/Debian
> the base package information is here https://packages.debian.org/wheezy/telnetd
> including maintainer email, etc.
>
> Typically, to get changes merged requires making a patch(es) against the package
> source and sending/uploading those.

This was already done, and the package maintainer said he was going to 
integrate it. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=585527

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists