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]

Message-ID: <2023082442-arose-jitters-00bb@gregkh>
Date:   Thu, 24 Aug 2023 21:22:18 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Simon Arlott <simon@...iron.net>
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 Thu, Aug 24, 2023 at 07:02:40PM +0100, Simon Arlott wrote:
> On 24/08/2023 15:48, Greg Kroah-Hartman wrote:
> > On Wed, Aug 23, 2023 at 09:37:45PM +0100, Simon Arlott wrote:
> >> This can happen with the ESP32-H2/ESP32-C6 USB serial interface. Instead of
> >> changing all userspace applications to flush (discard) their output in this
> >> specific scenario it would be easier to adjust the closing_wait timeout
> >> with udev rules but the only available interface is the TIOCGSERIAL ioctl.
> > 
> > Then why not use that?
> 
> It's not practical to use TIOCGSERIAL from a udev rule. Instead of a
> sysfs attribute (that udev has built-in support for writing) it would
> require a separate compiled process or other non-trivial dependencies
> (e.g. Python) to modify the closing_wait value. There's no shell script
> support for read-modify-write of a complex ioctl struct.

It's this way for all serial ports, cdc-acm is not "special" here,
sorry.

> The ioctl can't be used without opening and closing the tty, which has
> side effects. On open() it'll raise DTR/RTS and activate it. For cdc-acm
> that will indicate to the device that the serial port has been opened
> which will be visible to the software running on the USB device. On
> close() it'll be delayed by the close_delay if any process is currently
> doing a blocking open() and there's no carrier, then the closing_wait
> time if there's been any incomplete transmitted data (by any process).
> 
> I want to be able to automatically set close_delay to 0 and closing_wait
> to 65535 ("no waiting") on all USB serial devices without these kind of
> side effects. I'm sure these have a purpose when connected to a real tty
> but forcing a process to wait 30 seconds before it can close the port
> (or exit) if the USB device isn't reading data properly is inconvenient.
> 
> Those two values require CAP_SYS_ADMIN to modify (which is separately
> enforced by many of the tty drivers) so user applications can't change
> them even if they're aware of them.

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?

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
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?

> > 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'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?

confused,

greg k-h