| 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
| ||
|
Message-ID: <CACW=pY7P_i7gUjg5vOS9_kh2Z4jivP_nO5o8jH1+7UzU6MvuzQ@mail.gmail.com>
Date: Thu, 5 Jun 2025 09:15:24 +0800
From: 逸曉塵 <hsyemail2@...il.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: Johan Hovold <johan@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
Sheng-Yuan Huang <syhuang3@...oton.com>
Subject: Re: [PATCH v1 1/1] USB: serial: nct_usb_serial: add support for
Nuvoton USB adapter
Hi Oliver Neukum,
Appreciate the excellent suggestions! I'm in the process of revising
the code based on your input. It'll take me a little while to
thoroughly address everything, but I'll post an update here when I
have something new to share. Thank you very much.
Oliver Neukum <oneukum@...e.com> 於 2025年6月3日 週二 下午7:57寫道：
>
> Hi,
>
> On 03.06.25 05:20, hsyemail2@...il.com wrote:
> > From: Sheng-Yuan Huang <syhuang3@...oton.com>
> >
> > Add support for the Nuvoton USB-to-serial adapter, which provides
> > multiple serial ports over a single USB interface.
> >
> > The device exposes one control endpoint, one bulk-in endpoint, and
> > one bulk-out endpoint for data transfer. Port status is reported via
> > an interrupt-in or bulk-in endpoint, depending on device configuration.
>
> I am afraid there are a few issue that will not to be addressed
> before this can be merged.
>
> > This driver implements basic TTY operations.
> >
> > Signed-off-by: Sheng-Yuan Huang <syhuang3@...oton.com>
> > ---
> > drivers/usb/serial/nct_usb_serial.c | 1523 +++++++++++++++++++++++++++
> > 1 file changed, 1523 insertions(+)
> > create mode 100644 drivers/usb/serial/nct_usb_serial.c
> >
> > diff --git a/drivers/usb/serial/nct_usb_serial.c b/drivers/usb/serial/nct_usb_serial.c
> > new file mode 100644
> > index 000000000000..424c604229b3
> > --- /dev/null
> > +++ b/drivers/usb/serial/nct_usb_serial.c
>
> > +/* Index definition */
> > +enum {
> > + NCT_VCOM_INDEX_0 = 0,
> > + NCT_VCOM_INDEX_1,
> > + NCT_VCOM_INDEX_2,
> > + NCT_VCOM_INDEX_3,
> > + NCT_VCOM_INDEX_4,
> > + NCT_VCOM_INDEX_5,
> > + NCT_VCOM_INDEX_GLOBAL = 0xF,
> > +};
>
> What use is this? A number is a number.
>
> > +/* Command */
> > +enum {
> > + NCT_VCOM_GET_NUM_PORTS = 0,
> > + NCT_VCOM_GET_PORTS_SUPPORT,
> > + NCT_VCOM_GET_BAUD,
> > + NCT_VCOM_SET_INIT,
> > + NCT_VCOM_SET_CONFIG,
> > + NCT_VCOM_SET_BAUD,
> > + NCT_VCOM_SET_HCR,
> > + NCT_VCOM_SET_OPEN_PORT,
> > + NCT_VCOM_SET_CLOSE_PORT,
> > + NCT_VCOM_SILENT,
> > + /* Use bulk-in status instead of interrupt-in status */
> > + NCT_VCON_SET_BULK_IN_STATUS,
> > +};
>
> No. This is an abuse of enumeration. These are just commands that
> happen to use the number space consecutively. These need to be
> defines.
>
> > +union nct_vendor_cmd {
> > + struct pkg0 {
> > + u16 index:4;
> > + u16 cmd:8;
> > + } p;
> > + u16 val;
> > +} __packed;
>
> This definition is an endianness bug waiting to happen.
> If this goes over the wire, it has a defined endianness,
> which needs to be declared.
>
> > +#define NCT_HDR_MAGIC 0xA5
> > +#define NCT_HDR_MAGIC2 0x5A
> > +#define NCT_HDR_MAGIC_STATUS 0x5B
> > +
> > +struct nct_packet_header {
> > + unsigned int magic:8;
> > + unsigned int magic2:8;
> > + unsigned int idx:4;
> > + unsigned int len:12;
> > +} __packed;
>
> Again endianness.
>
> > +/* The definitions are for the feilds of nct_ctrl_msg */
> > +#define NCT_VCOM_1_STOP_BIT 0
> > +#define NCT_VCOM_2_STOP_BITS 1
> > +#define NCT_VCOM_PARITY_NONE 0
> > +#define NCT_VCOM_PARITY_ODD 1
> > +#define NCT_VCOM_PARITY_EVEN 2
> > +#define NCT_VCOM_DL5 0
> > +#define NCT_VCOM_DL6 1
> > +#define NCT_VCOM_DL7 2
> > +#define NCT_VCOM_DL8 3
> > +#define NCT_VCOM_DISABLE_FLOW_CTRL 0
> > +#define NCT_VCOM_XOFF 1
> > +#define NCT_VCOM_RTS_CTS 2
> > +union nct_ctrl_msg {
> > + struct pkg1 {
> > + u16 stop_bit:1;
> > + u16 parity:2;
> > + u16 data_len:2;
> > + u16 flow:2;
> > + u16 spd:5;
> > + u16 reserved:4;
> > + } p;
> > + u16 val;
> > +} __packed;
>
> At the risk of repeating myself: endianness
>
> > +
> > +/* Read from USB control pipe */
> > +static int nct_vendor_read(struct usb_interface *intf, union nct_vendor_cmd cmd,
> > + unsigned char *buf, int size)
> > +{
> > + struct device *dev = &intf->dev;
> > + struct usb_device *udev = interface_to_usbdev(intf);
> > + u8 *tmp_buf;
> > + int res;
> > +
> > + tmp_buf = kmalloc(NCT_MAX_VENDOR_READ_SIZE, GFP_KERNEL);
> > + if (!tmp_buf)
> > + return -ENOMEM;
> > +
> > + if (size > NCT_MAX_VENDOR_READ_SIZE)
> > + dev_err(dev, NCT_DRVNAME ": %s - failed to read [%04x]: over size %d\n",
> > + __func__, cmd.p.cmd, size);
>
> And you just go on and overwrite kernel memory?
> If you test for plausibility, do something with the result.
>
>
> > +static int nct_vendor_write(struct usb_interface *intf, union nct_vendor_cmd cmd, u16 val)
> > +{
> > + struct device *dev = &intf->dev;
> > + struct usb_device *udev = interface_to_usbdev(intf);
> > + int res;
> > + u8 *buf_val;
>
> Why is this u8* ?
> It should be le16*
>
> > + buf_val = kmalloc(2, GFP_KERNEL);
> > + if (!buf_val)
> > + return -ENOMEM;
> > +
> > + /* Copy data to the buffer for sending */
> > + buf_val[0] = val & 0xff;
> > + buf_val[1] = (val >> 8) & 0xff;
>
> We have macros for that.
>
> > +static u16 nct_set_baud(struct usb_interface *intf, u16 index, unsigned int cflag)
> > +{
> > + union nct_ctrl_msg msg;
> > + union nct_vendor_cmd cmd;
> > + u16 i;
> > +
> > + msg.val = 0;
> > + cmd.p.cmd = NCT_VCOM_SET_BAUD;
> > + msg.p.spd = NCT_DEFAULT_BAUD;
> > + cmd.p.index = index;
> > + dev_dbg(&intf->dev, NCT_DRVNAME ": %s tty baud: 0x%X\n", __func__,
> > + (cflag & CBAUD));
> > + for (i = 0; i < ARRAY_SIZE(NCT_BAUD_SUP); i++) {
> > + if ((cflag & CBAUD) == NCT_BAUD_SUP[i]) {
> > + msg.p.spd = i;
> > + dev_dbg(&intf->dev,
> > + NCT_DRVNAME ": %s index %d set baud: NCT_BAUD_SUP[%d]=%d\n",
> > + __func__, cmd.p.index, msg.p.spd, NCT_BAUD_SUP[i]);
> > + if (nct_vendor_write(intf, cmd, msg.val))
> > + dev_err(&intf->dev,
> > + NCT_DRVNAME ": %s - Set index: %d speed error\n",
> > + __func__, cmd.p.index);
> > +
> > + break;
> > + }
>
> If nothing matches, you do nothing?
> > + }
> > +
> > + return msg.p.spd;
>
> So errors are ignored?
>
>
> > +static int nct_serial_tiocmset(struct tty_struct *tty, unsigned int set,
> > + unsigned int clear)
> > +{
> > + return nct_tiocmset_helper(tty, set, clear);
> > +}
>
> Why? Does this function do anything useful?
>
> > +static void nct_rx_throttle(struct tty_struct *tty)
> > +{
> > + unsigned int set;
> > + unsigned int clear = 0;
>
> Why?
>
> > +
> > + /* If we are implementing RTS/CTS, control that line */
> > + if (C_CRTSCTS(tty)) {
> > + set = 0;
> > + clear = TIOCM_RTS;
> > + nct_tiocmset_helper(tty, set, clear);
> > + }
> > +}
> > +
> > +static void nct_rx_unthrottle(struct tty_struct *tty)
> > +{
> > + unsigned int set;
> > + unsigned int clear = 0;
>
> Why?
>
> > + /* If we are implementing RTS/CTS, control that line */
> > + if (C_CRTSCTS(tty)) {
> > + set = 0;
> > + set |= TIOCM_RTS;
> > + nct_tiocmset_helper(tty, set, clear);
> > + }
> > +}
> > +
> > +static int nct_serial_write_data(struct tty_struct *tty, struct usb_serial_port *port,
> > + const unsigned char *buf, int count)
> > +{
> > + int ret;
> > + unsigned long flags;
> > + struct nct_packet_header hdr;
> > + int wr_len;
> > + struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > +
> > + wr_len = min((unsigned int)count, NCT_MAX_SEND_BULK_SIZE - sizeof(hdr));
> > +
> > + if (!wr_len)
> > + return 0;
> > +
> > + spin_lock_irqsave(&tport->port_lock, flags);
> > +
> > + if (tport->write_urb_in_use) {
> > + spin_unlock_irqrestore(&tport->port_lock, flags);
> > + return 0;
> > + }
> > +
> > + /* Fill header */
> > + hdr.magic = NCT_HDR_MAGIC;
> > + hdr.magic2 = NCT_HDR_MAGIC2;
> > + hdr.idx = tport->hw_idx; /* The 'hw_idx' is based on 1 */
>
> Endianness.
>
> > +
> > + /* Copy data */
> > + memcpy(port->write_urb->transfer_buffer + sizeof(hdr),
> > + (const void *)buf, wr_len);
> > +
> > + hdr.len = wr_len; /* File filed 'len' of header */
>
> Endiannes
>
> > +static int nct_startup_device(struct usb_serial *serial)
> > +{
> > + int ret = 0;
> > + struct nct_serial *serial_priv = usb_get_serial_data(serial);
> > + struct usb_serial_port *port;
> > + unsigned long flags;
> > +
> > + /* Be sure this happens exactly once */
> > + spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > +
> > + if (serial_priv->device_init) {
> > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > + return 0;
> > + }
> > + serial_priv->device_init = true;
> > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > +
> > + /* Start reading from bulk in endpoint */
> > + port = serial->port[0];
> > + if (!port->read_urb)
> > + dev_dbg(&port->dev, NCT_DRVNAME ": %s: port->read_urb is null, index=%d\n",
> > + __func__, 0);
> > +
> > + ret = usb_submit_urb(port->read_urb, GFP_KERNEL);
> > + if (ret)
> > + dev_err(&port->dev,
> > + NCT_DRVNAME ": %s: usb_submit_urb failed, ret=%d, port=%d\n",
> > + __func__, ret, 0);
>
> Error handling?
> > +
> > + /* For getting status from interrupt-in */
> > + if (!serial_priv->status_trans_mode) {
> > + /* Start reading from interrupt pipe */
> > + port = serial->port[0];
> > + ret = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
> > + if (ret)
> > + dev_err(&port->dev,
> > + NCT_DRVNAME ": %s: usb_submit_urb(intr) failed, ret=%d, port=%d\n",
> > + __func__, ret, 0);
> > + }
> > + return ret;
> > +}
> > +
> > +static void nct_serial_port_end(struct usb_serial_port *port)
> > +{
> > + struct nct_tty_port *tport = usb_get_serial_port_data(port);
> > + struct usb_serial *serial = port->serial;
> > + struct usb_interface *intf = serial->interface;
> > + union nct_ctrl_msg msg;
> > + union nct_vendor_cmd cmd;
> > +
> > + /* Send 'Close Port' to the device */
> > + cmd.p.index = (u16)tport->hw_idx;
> > + cmd.p.cmd = NCT_VCOM_SET_CLOSE_PORT;
>
> Endianness
>
>
> > +again:
> > + spin_lock_irqsave(&serial_priv->serial_lock, flags);
> > + tport = serial_priv->cur_port;
> > + if (!tport) {
> > + /*
> > + * Handle a new data package (i.e., it is not
> > + * the remaining data without a header).
> > + * The package does not need to be combined this time.
> > + */
> > +
> > + for (i = 0; i < urb->actual_length; i++) {
> > + hdr = (struct nct_packet_header *)data;
> > + /* Decode the header */
> > +
> > + if (serial_priv->status_trans_mode) {
> > + /*
> > + * Status data is also transmitted via bulk-in
> > + * pipe.
> > + */
> > + if (hdr->magic == NCT_HDR_MAGIC &&
> > + hdr->magic2 == NCT_HDR_MAGIC_STATUS &&
> > + hdr->len == 24 && actual_len >= 28) {
>
> Endianness
>
> > + /*
> > + * Notice: actual_len will be decreased,
> > + * it is equal to urb->actual_length
> > + * only at the beginning.
> > + */
> > +
> > + /*
> > + * Status report.
> > + * It should be a standalone package in
> > + * one URB
> > + */
> > + data += sizeof(struct nct_packet_header);
> > + actual_len -=
> > + sizeof(struct nct_packet_header);
> > +
> > + nps = (struct nct_port_status *)data;
> > +
> > + for (j = 0; j < actual_len - 4; j++) {
> > + nct_update_status(serial,
> > + (unsigned char *)nps);
> > + nps++;
> > + }
> > +
> > + spin_unlock_irqrestore(&serial_priv->serial_lock, flags);
> > + return;
> > + }
> > + }
> > +
> > + if (hdr->magic == NCT_HDR_MAGIC &&
> > + hdr->magic2 == NCT_HDR_MAGIC2 &&
> > + hdr->idx <= NCT_MAX_NUM_COM_DEVICES &&
> > + hdr->len <= 512)
> > + break;
>
> Endianness
>
> Regards
> Oliver
>
Powered by blists - more mailing lists