linux-kernel - Re: [PATCH v8 1/2] usb: serial: add support for CH348

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aINXS813fmWNJh3A@hovoldconsulting.com>
Date: Fri, 25 Jul 2025 12:07:07 +0200
From: Johan Hovold <johan@...nel.org>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	david@...t.cz
Subject: Re: [PATCH v8 1/2] usb: serial: add support for CH348

On Tue, Jul 15, 2025 at 11:20:33PM +0200, Martin Blumenstingl wrote:

> On Mon, May 12, 2025 at 12:03 PM Johan Hovold <johan@...nel.org> wrote:

> > The read urbs should be submitted at first open and stopped at last
> > close to avoid wasting resources when no one is using the device.
> >
> > I know we have a few drivers that do not do this currently, but it
> > shouldn't be that hard to get this right from the start.

> If you're aware of an easy approach or you can recommend an existing
> driver that implements the desired behavior then please let me know.
> 
> The speciality about ch348 is that all ports share the RX/TX URBs.
> My current idea is to implement this using a ref count (for the number
> of open ports) and mutex for locking.

Just use a mutex and integer (not refcount) to count the number of open
ports. Submit the urbs on first open and stop them on last close.

Not doing so, and instead submitting at attach(), means that the host
controller will be wasting power by polling the endpoints continuously
as long as the device is plugged in.

> [...]
> > > +                     /*
> > > +                      * Writing larger buffers can take longer than the
> > > +                      * hardware allows before discarding the write buffer.
> > > +                      * Limit the transfer size in such cases.
> > > +                      * These values have been found by empirical testing.
> > > +                      */
> > > +                     max_bytes = 128;
> >
> > This is a potential buffer overflow if a (malicious) device has
> > endpoints smaller than this (use min()).

> For endpoints smaller than CH348_TX_HDRSIZE we'll also be in trouble.
> Validating against CH348_TX_HDRSIZE size here doesn't make much sense
> to me (as we'd never be able to progress). I think I should validate
> it in ch348_attach() instead - what do you think?

Sure you can do that.

> > > +             else
> > > +                     /*
> > > +                      * Only ingest as many bytes as we can transfer with
> > > +                      * one URB at a time keeping the TX header in mind.
> > > +                      */
> > > +                     max_bytes = hw_tx_port->bulk_out_size - CH348_TX_HDRSIZE;
> > > +
> > > +             count = kfifo_out_locked(&port->write_fifo, rxt->data,
> > > +                                      max_bytes, &port->lock);
> > > +             if (count)
> > > +                     break;
> > > +     }
> >
> > With this implementation writing data continuously to one port will
> > starve the others.
> >
> > The vendor implementation appears to write to more than one port in
> > parallel and track THRE per port which would avoid the starvation issue
> > and should also be much more efficient.
> >
> > Just track THRE per port and only submit the write urb when it the
> > transmitter is empty or when it becomes empty.

> I'm trying as you suggest:
> - submit the URB synchronously for port N
> - submit the URB synchronously for port N + 1
> - ...
> 
> This seems to work (using usb_bulk_msg). What doesn't work is
> submitting URBs in parallel (this is what the vendor driver prevents
> as well).

No, the vendor driver tracks THRE per port
(ttyport[portnum].write_empty) and allows writing to more than one port
in parallel (e.g. releases the device write_lock while waiting for the
transfer to complete).

I thought the problem was that you could not submit another urb for the
*same* port until the device buffer for that port had been emptied?

This seems to be what the vendor driver is preventing.

> > You should implement dtr_rts() as well.

> This will be the first time we need the "package type" information as
> CH348Q only supports CTS/RTS on the first four ports, for the last
> four the signals aren't routed outside the package.
> I need to see if I have other hardware with CTS/RTS pins to test this.

Just connect a multimeter to the DTR and RTS pins and verify that they
are asserted on open and deasserted on close after issuing those control
requests (see ch9344_port_dtr_rts()).

I didn't mean that you need to support hw flow control (CRTSCTS).

Johan