linux-kernel - Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 27 Aug 2023 16:36:00 +0100
From:   Simon Arlott <simon@...iron.net>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>
Cc:     Oliver Neukum <oneukum@...e.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        linux-serial@...r.kernel.org
Subject: Re: [PATCH] USB: cdc-acm: expose serial close_delay and closing_wait
 in sysfs

On 24/08/2023 20:22, Greg Kroah-Hartman wrote:
> So this looks like you feel there should be a way to modify serial port
> values without the ioctl api.  That's good, maybe we do need to do this,
> but then, if so, it needs to happen for all serial ports, not just one
> single device type.
> 
> Note that the tty api is really really old, so modifications and
> enhancements need to be done carefully.  And be sure that there isn't
> already another way to do this.  The open/close DTR/RTS issue has been
> brought up many times, and I thought that there was ways to prevent it
> already, are you sure that modern tools don't already take this into
> consideration?

On 25/08/2023 02:53, Alan Stern wrote:
> Is it feasible to add a sysfs attribute for ttys or the serial layer to 
> control the side effect of opening (avoid raising DTR/RTS)?  If that 
> could be done, a program could use the existing ioctl to set close_delay 
> and closing_wait to 0 with no penalties.

The port will still be "activated". That has side effects for an
application running on a microcontroller providing the USB CDC ACM
interface because it may be waiting for that state change to output a
message or start doing something.

> Or better yet, don't make any change until you actually open the port
> for access.  Why wory about these values until you need to use it?  Why
> insist on doing it from a udev rule when there is no real user of the

Because the applications that access the serial port aren't typically
aware that close() may block for 30 seconds. Even if they are, they
can't do much about it because of the next problem.

> port yet?  Who are you setting the port up for, and why can't they do it
> when they open and set the other values that they want?

Because they're not running as root and so don't have CAP_SYS_ADMIN and
can't change these two values.

>>> If any apis are needed, let's make them for all tty devices, through the
>>> existing ioctl api, so they work for all devices and userspace doesn't
>>> have to try to figure out just exactly what type of tty/serial device it
>>> is talking to (as that will not scale and is exactly the opposite of
>>> what common apis are for.)
>>
>> Are you ok with adding the same two attributes to sysfs for all ttys?
> 
> Why just these attributes, why not tty settings?  :)

I assume you mean all tty settings? Looking at termios(3) there are a
lot of them...

Restricting them to just the serial settings (include/uapi/linux/serial.h
serial_struct), some of them only apply to real 16550-like serial ports
("type") and won't be applicable everywhere ("irq").

There would then be several serial devices with attributes that don't do
anything, e.g. "irq" will read as 0 and writes will do nothing. We
wouldn't know at the tty layer which writes will do nothing because
there's only one operation for the whole serial_struct.

The "close_delay" and "closing_wait" values have universal application
because the tty layer uses them. They're set on the tty_port in
tty_port_init() and then the serial port drivers modify them when
TIOCSSERIAL is used. There doesn't appear to be a tty-level API to
modify them.

>> I'd use a different name (appending "_centisecs") because serial_core
>> already uses those names on the tty device and I think it's better to
>> define "infinite wait" and "no wait" as -1 and 0 instead of 0 and 65535.
> 
> Ah, so this already is an api?  Or is it a different one?

The serial_core adds read-only attributes to the tty device for most of
the TIOCGSERIAL values. That includes the tty closing options but
they're exact mirrors of the ioctl API which means it has special values
selected by the range of a u16 value.

A new sysfs attribute should use better special values.

-- 
Simon Arlott