USB cdc-acm driver: break and command

USB cdc-acm driver: break and command

[H. Peter Anvin]

Hi,

I noticed looking at the CDC-ACM driver that it uses the assert/local 
delay/deassert method of sending BREAK.  Given that the CDC model has a 
delay specifier in the command packet, is there any reason not to set 
TTY_DRIVER_HARDWARE_BREAK and sending only one packet?

I'm also wondering if it would make sense to support the 
SEND_ENCAPSULATED_COMMAND and GET_ENCAPSULATED_RESPONSE commands, 
presumably via an ioctl().  I'm not 100% sure because I'm not sure there 
aren't potential security issues.

I'm guessing both of these could be of some use to embedded devices that 
emulate a ttyACM serial port.

	-hpa

Re: USB cdc-acm driver: break and command

[Greg KH]

On Tue, Jul 15, 2025 at 02:00:10PM -0700, H. Peter Anvin wrote:
> Hi,
> 
> I noticed looking at the CDC-ACM driver that it uses the assert/local
> delay/deassert method of sending BREAK.  Given that the CDC model has a
> delay specifier in the command packet, is there any reason not to set
> TTY_DRIVER_HARDWARE_BREAK and sending only one packet?
> 
> I'm also wondering if it would make sense to support the
> SEND_ENCAPSULATED_COMMAND and GET_ENCAPSULATED_RESPONSE commands, presumably
> via an ioctl().  I'm not 100% sure because I'm not sure there aren't
> potential security issues.
> 
> I'm guessing both of these could be of some use to embedded devices that
> emulate a ttyACM serial port.

No idea why this is that way, sorry.  Try making the change and see if
it actually works?

thanks,

greg k-h

Re: USB cdc-acm driver: break and command

[Oliver Neukum]

On 15.07.25 23:00, H. Peter Anvin wrote:
> Hi,
> 
> I noticed looking at the CDC-ACM driver that it uses the assert/local delay/deassert method of sending BREAK.  Given that the CDC model has a delay specifier in the command packet, is there any reason not to set TTY_DRIVER_HARDWARE_BREAK and sending only one packet?

1. The existing code is tested and usually works.
2. The locking goes away. I have no idea what happens if you are
sending a second break while a break is still going on.

> I'm also wondering if it would make sense to support the SEND_ENCAPSULATED_COMMAND and GET_ENCAPSULATED_RESPONSE commands, presumably via an ioctl().  I'm not 100% sure because I'm not sure there aren't potential security issues.

Well, one of the purposes of the CDC-ACM driver is to hide that
you are talking to a USB device.
In theory we could do that. I don't quite see the value.
Sending arbitrary data from user space to a control endpoint
does not make me happy.

	HTH
		Oliver
  

Re: USB cdc-acm driver: break and command

[H. Peter Anvin]

On July 16, 2025 1:29:27 AM PDT, Oliver Neukum <oneukum@suse.com> wrote:
>On 15.07.25 23:00, H. Peter Anvin wrote:
>> Hi,
>> 
>> I noticed looking at the CDC-ACM driver that it uses the assert/local delay/deassert method of sending BREAK.  Given that the CDC model has a delay specifier in the command packet, is there any reason not to set TTY_DRIVER_HARDWARE_BREAK and sending only one packet?
>
>1. The existing code is tested and usually works.
>2. The locking goes away. I have no idea what happens if you are
>sending a second break while a break is still going on.
>
>> I'm also wondering if it would make sense to support the SEND_ENCAPSULATED_COMMAND and GET_ENCAPSULATED_RESPONSE commands, presumably via an ioctl().  I'm not 100% sure because I'm not sure there aren't potential security issues.
>
>Well, one of the purposes of the CDC-ACM driver is to hide that
>you are talking to a USB device.
>In theory we could do that. I don't quite see the value.
>Sending arbitrary data from user space to a control endpoint
>does not make me happy.
>
>	HTH
>		Oliver
> 
However, the flipside is that ACM devices are very often used because it is one of the few ways you can avoid requiring device drivers on Windows.

This is why some devices do absolutely insane crap like Arduino which trigger a board reset if you set the speed to 1200 baud.

SEND_ENCAPSULATED_COMMAND at least takes a command string – it was intended, I believe, to be able to send AT commands to a modem while online without using the +++ escape code and all the potential race conditions (and security issues, since it is trivial for a user to generate) associated with that.

I am not suggesting that we ought to allow sending *arbitrary USB commands*; if that is what's needed then use the USB device interface, which of course has its own permission flags. The main problem with that is exactly that it is all-or-nothing (it is also a bit of a pain to find the underlying USB device corresponding to an ACM or USB tty, but that's another matter entirely; it is at least doable.)

As far as BREAK is concerned, there is also the option of locking out a second BREAK for the delay time; however, this probably should belong in the tty core.  What do other drivers supporting TTY_DRIVER_HARDWARE_BREAK do?

Re: USB cdc-acm driver: break and command

[Oliver Neukum]

On 16.07.25 17:06, H. Peter Anvin wrote:

> SEND_ENCAPSULATED_COMMAND at least takes a command string – it was intended, I believe, to be able to send AT commands to a modem while online without using the +++ escape code and all the potential race conditions (and security issues, since it is trivial for a user to generate) associated with that.

Understood. It still seems dirty to me. If you want to send strings to a device
the proper way is to use a device node and write().
  
> As far as BREAK is concerned, there is also the option of locking out a second BREAK for the delay time; however, this probably should belong in the tty core.  What do other drivers supporting TTY_DRIVER_HARDWARE_BREAK do?

I know of no driver but n_gsm which uses it. That driver needs to use it,
because it cannot switch off a break.

If you really wanted to use that API as it is right now, you'd
have breaks racing with each other and, worse, with open()
and close().
Are you sure POSIX says nothing about how to handle such cases?

You'd probably have to start a timer in the driver in send_break().
That timer would need to be properly handled in disconnect(),
pre/post_reset() and suspend()
That API is really not nice to use.

	Regards
		Oliver

Re: USB cdc-acm driver: break and command

[H. Peter Anvin]

On 2025-07-16 09:17, Oliver Neukum wrote:
> 
> If you really wanted to use that API as it is right now, you'd
> have breaks racing with each other and, worse, with open()
> and close().
> Are you sure POSIX says nothing about how to handle such cases?
> 

This is the POSIX definition for tcsendbreak():

NAME

    tcsendbreak — send a break for a specific duration

SYNOPSIS

    #include <termios.h>

    int tcsendbreak(int fildes, int duration);

DESCRIPTION

    If the terminal is using asynchronous serial data transmission,
tcsendbreak() shall cause transmission of a continuous stream of
zero-valued bits for a specific duration. If duration is 0, it shall
cause transmission of zero-valued bits for at least 0.25 seconds, and
not more than 0.5 seconds. If duration is not 0, it shall send
zero-valued bits for an implementation-defined period of time.

    The fildes argument is an open file descriptor associated with a
terminal.

    If the terminal is not using asynchronous serial data transmission,
it is implementation-defined whether tcsendbreak() sends data to
generate a break condition or returns without taking any action.

    Attempts to use tcsendbreak() from a process which is a member of a
background process group on a fildes associated with its controlling
terminal shall cause the process group to be sent a SIGTTOU signal. If
the calling thread is blocking SIGTTOU signals or the process is
ignoring SIGTTOU signals, the process shall be allowed to perform the
operation, and no signal is sent.

RETURN VALUE

    Upon successful completion, 0 shall be returned. Otherwise, -1 shall
be returned and errno set to indicate the error.

ERRORS

    The tcsendbreak() function shall fail if:

    [EBADF]
        The fildes argument is not a valid file descriptor.
    [EIO]
        The process group of the writing process is orphaned, the
calling thread is not blocking SIGTTOU, and the process is not ignoring
SIGTTOU.
    [ENOTTY]
        The file associated with fildes is not a terminal.

--- ---

The only other mentions of BREAK I can find are the BRKINT and IGNBRK
flags, respectively.

>
> You'd probably have to start a timer in the driver in send_break().
> That timer would need to be properly handled in disconnect(),
> pre/post_reset() and suspend()
> That API is really not nice to use.
> 

That's why I said if that is what is needed, it really belongs in the
tty core.  That's where the current internal delay is, after all.

	-hpa

Re: USB cdc-acm driver: break and command

[Oliver Neukum]

On 16.07.25 19:30, H. Peter Anvin wrote:
  
> ERRORS
> 
>      The tcsendbreak() function shall fail if:
> 
>      [EBADF]
>          The fildes argument is not a valid file descriptor.
>      [EIO]
>          The process group of the writing process is orphaned, the
> calling thread is not blocking SIGTTOU, and the process is not ignoring
> SIGTTOU.
>      [ENOTTY]
>          The file associated with fildes is not a terminal.

I would take this as meaning that we cannot just return -EBUSY or -EWOULDBLOCK.
Hence the generic layer would need to implement some sort of waiting
logic.

> That's why I said if that is what is needed, it really belongs in the
> tty core.  That's where the current internal delay is, after all.

Good. Don't get me wrong. I'd love to do this more efficiently,
but the current API is less than optimal.

	Regards
		Oliver

Re: USB cdc-acm driver: break and command

[H. Peter Anvin]

On 2025-07-16 09:17, Oliver Neukum wrote:
> On 16.07.25 17:06, H. Peter Anvin wrote:
> 
>> SEND_ENCAPSULATED_COMMAND at least takes a command string – it was
>> intended, I believe, to be able to send AT commands to a modem while
>> online without using the +++ escape code and all the potential race
>> conditions (and security issues, since it is trivial for a user to
>> generate) associated with that.
> 
> Understood. It still seems dirty to me. If you want to send strings to a
> device
> the proper way is to use a device node and write().
>  

There is definitely something to be said for that; or at least a file
descriptor.  We do have cases in the kernel -- notably opening the pts
corresponding to a ptmx file descriptor -- that do that sort of
"auxiliary open" kind of thing.

The big question is how that interacts with the rest of the ACM driver,
as well as all the lifetime issues you mentioned elsewhere.

	-hpa

Re: USB cdc-acm driver: break and command

[Oliver Neukum]

On 16.07.25 19:49, H. Peter Anvin wrote:
> On 2025-07-16 09:17, Oliver Neukum wrote:

>> Understood. It still seems dirty to me. If you want to send strings to a
>> device
>> the proper way is to use a device node and write().
>>   
> 
> There is definitely something to be said for that; or at least a file
> descriptor.  We do have cases in the kernel -- notably opening the pts
> corresponding to a ptmx file descriptor -- that do that sort of
> "auxiliary open" kind of thing.
> 
> The big question is how that interacts with the rest of the ACM driver,
> as well as all the lifetime issues you mentioned elsewhere.

It would seem to me that CDC already has something very similar in form
of CDC-WDM. If acm_probe() can call tty_port_register_device(), it can also
register a secondary character device. Or are you worried about how to tell
user space which devices belong together?

	Regards
		Oliver