[PATCH 0/5] USB: serial: ir-usb fixes and cleanups

[PATCH 0/5] USB: serial: ir-usb fixes and cleanups

[Johan Hovold]

This series fixes a possible NULL-pointer dereference due to a missing
endpoint sanity check, as well as two long-standing regressions in the
ir-usb driver.

The last two patches clean up the set_termios() and switches to rely on
the relatively new feature to let USB serial core to verify the required
endpoints.

Johan


Johan Hovold (5):
  USB: serial: ir-usb: add missing endpoint sanity check
  USB: serial: ir-usb: fix link-speed handling
  USB: serial: ir-usb: fix IrLAP framing
  USB: serial: ir-usb: make set_termios synchronous
  USB: serial: ir-usb: simplify endpoint check

 drivers/usb/serial/ir-usb.c | 185 ++++++++++++++++++++++--------------
 include/linux/usb/irda.h    |  13 ++-
 2 files changed, 126 insertions(+), 72 deletions(-)

-- 
2.24.1

[PATCH 1/5] USB: serial: ir-usb: add missing endpoint sanity check

[Johan Hovold]

Add missing endpoint sanity check to avoid dereferencing a NULL-pointer
on open() in case a device lacks a bulk-out endpoint.

Note that prior to commit f4a4cbb2047e ("USB: ir-usb: reimplement using
generic framework") the oops would instead happen on open() if the
device lacked a bulk-in endpoint and on write() if it lacked a bulk-out
endpoint.

Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 302eb9530859..c3b06fc5a7f0 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -195,6 +195,9 @@ static int ir_startup(struct usb_serial *serial)
 	struct usb_irda_cs_descriptor *irda_desc;
 	int rates;
 
+	if (serial->num_bulk_in < 1 || serial->num_bulk_out < 1)
+		return -ENODEV;
+
 	irda_desc = irda_usb_find_class_desc(serial, 0);
 	if (!irda_desc) {
 		dev_err(&serial->dev->dev,
-- 
2.24.1

Re: [PATCH 1/5] USB: serial: ir-usb: add missing endpoint sanity check

[Greg KH]

On Wed, Jan 22, 2020 at 11:15:26AM +0100, Johan Hovold wrote:
> Add missing endpoint sanity check to avoid dereferencing a NULL-pointer
> on open() in case a device lacks a bulk-out endpoint.
> 
> Note that prior to commit f4a4cbb2047e ("USB: ir-usb: reimplement using
> generic framework") the oops would instead happen on open() if the
> device lacked a bulk-in endpoint and on write() if it lacked a bulk-out
> endpoint.
> 
> Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 2/5] USB: serial: ir-usb: fix link-speed handling

[Johan Hovold]

Commit e0d795e4f36c ("usb: irda: cleanup on ir-usb module") added a USB
IrDA header with common defines, but mistakingly switched to using the
class-descriptor baud-rate bitmask values for the outbound header.

This broke link-speed handling for rates above 9600 baud, but a device
would also be able to operate at the default 9600 baud until a
link-speed request was issued (e.g. using the TCGETS ioctl).

Fixes: e0d795e4f36c ("usb: irda: cleanup on ir-usb module")
Cc: stable <stable@vger.kernel.org>     # 2.6.27
Cc: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 20 ++++++++++----------
 include/linux/usb/irda.h    | 13 ++++++++++++-
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index c3b06fc5a7f0..26eab1307165 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -335,34 +335,34 @@ static void ir_set_termios(struct tty_struct *tty,
 
 	switch (baud) {
 	case 2400:
-		ir_baud = USB_IRDA_BR_2400;
+		ir_baud = USB_IRDA_LS_2400;
 		break;
 	case 9600:
-		ir_baud = USB_IRDA_BR_9600;
+		ir_baud = USB_IRDA_LS_9600;
 		break;
 	case 19200:
-		ir_baud = USB_IRDA_BR_19200;
+		ir_baud = USB_IRDA_LS_19200;
 		break;
 	case 38400:
-		ir_baud = USB_IRDA_BR_38400;
+		ir_baud = USB_IRDA_LS_38400;
 		break;
 	case 57600:
-		ir_baud = USB_IRDA_BR_57600;
+		ir_baud = USB_IRDA_LS_57600;
 		break;
 	case 115200:
-		ir_baud = USB_IRDA_BR_115200;
+		ir_baud = USB_IRDA_LS_115200;
 		break;
 	case 576000:
-		ir_baud = USB_IRDA_BR_576000;
+		ir_baud = USB_IRDA_LS_576000;
 		break;
 	case 1152000:
-		ir_baud = USB_IRDA_BR_1152000;
+		ir_baud = USB_IRDA_LS_1152000;
 		break;
 	case 4000000:
-		ir_baud = USB_IRDA_BR_4000000;
+		ir_baud = USB_IRDA_LS_4000000;
 		break;
 	default:
-		ir_baud = USB_IRDA_BR_9600;
+		ir_baud = USB_IRDA_LS_9600;
 		baud = 9600;
 	}
 
diff --git a/include/linux/usb/irda.h b/include/linux/usb/irda.h
index 396d2b043e64..556a801efce3 100644
--- a/include/linux/usb/irda.h
+++ b/include/linux/usb/irda.h
@@ -119,11 +119,22 @@ struct usb_irda_cs_descriptor {
  * 6 - 115200 bps
  * 7 - 576000 bps
  * 8 - 1.152 Mbps
- * 9 - 5 mbps
+ * 9 - 4 Mbps
  * 10..15 - Reserved
  */
 #define USB_IRDA_STATUS_LINK_SPEED	0x0f
 
+#define USB_IRDA_LS_NO_CHANGE		0
+#define USB_IRDA_LS_2400		1
+#define USB_IRDA_LS_9600		2
+#define USB_IRDA_LS_19200		3
+#define USB_IRDA_LS_38400		4
+#define USB_IRDA_LS_57600		5
+#define USB_IRDA_LS_115200		6
+#define USB_IRDA_LS_576000		7
+#define USB_IRDA_LS_1152000		8
+#define USB_IRDA_LS_4000000		9
+
 /* The following is a 4-bit value used only for
  * outbound header:
  *
-- 
2.24.1

Re: [PATCH 2/5] USB: serial: ir-usb: fix link-speed handling

[Greg KH]

On Wed, Jan 22, 2020 at 11:15:27AM +0100, Johan Hovold wrote:
> Commit e0d795e4f36c ("usb: irda: cleanup on ir-usb module") added a USB
> IrDA header with common defines, but mistakingly switched to using the
> class-descriptor baud-rate bitmask values for the outbound header.
> 
> This broke link-speed handling for rates above 9600 baud, but a device
> would also be able to operate at the default 9600 baud until a
> link-speed request was issued (e.g. using the TCGETS ioctl).

People still use this driver/hardware?  And this wasn't found until now?
wow...

> Fixes: e0d795e4f36c ("usb: irda: cleanup on ir-usb module")
> Cc: stable <stable@vger.kernel.org>     # 2.6.27
> Cc: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/5] USB: serial: ir-usb: fix link-speed handling

[Johan Hovold]

On Wed, Jan 22, 2020 at 02:24:50PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 22, 2020 at 11:15:27AM +0100, Johan Hovold wrote:
> > Commit e0d795e4f36c ("usb: irda: cleanup on ir-usb module") added a USB
> > IrDA header with common defines, but mistakingly switched to using the
> > class-descriptor baud-rate bitmask values for the outbound header.
> > 
> > This broke link-speed handling for rates above 9600 baud, but a device
> > would also be able to operate at the default 9600 baud until a
> > link-speed request was issued (e.g. using the TCGETS ioctl).
> 
> People still use this driver/hardware?  And this wasn't found until now?
> wow...

Good question. I was considering just removing the driver, but since it
would have continued to work on low and default link speed I figured I
better just fix it up.

> > Fixes: e0d795e4f36c ("usb: irda: cleanup on ir-usb module")
> > Cc: stable <stable@vger.kernel.org>     # 2.6.27
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing.

Johan

[PATCH 3/5] USB: serial: ir-usb: fix IrLAP framing

[Johan Hovold]

Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
switched to using the generic write implementation which may combine
multiple write requests into larger transfers. This can break the IrLAP
protocol where end-of-frame is determined using the USB short packet
mechanism, for example, if multiple frames are sent in rapid succession.

Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
Cc: stable <stable@vger.kernel.org>     # 2.6.35
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 26eab1307165..627bea7e6cfb 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -45,9 +45,10 @@ static int buffer_size;
 static int xbof = -1;
 
 static int  ir_startup (struct usb_serial *serial);
-static int  ir_open(struct tty_struct *tty, struct usb_serial_port *port);
-static int ir_prepare_write_buffer(struct usb_serial_port *port,
-						void *dest, size_t size);
+static int ir_write(struct tty_struct *tty, struct usb_serial_port *port,
+		const unsigned char *buf, int count);
+static int ir_write_room(struct tty_struct *tty);
+static void ir_write_bulk_callback(struct urb *urb);
 static void ir_process_read_urb(struct urb *urb);
 static void ir_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios);
@@ -77,8 +78,9 @@ static struct usb_serial_driver ir_device = {
 	.num_ports		= 1,
 	.set_termios		= ir_set_termios,
 	.attach			= ir_startup,
-	.open			= ir_open,
-	.prepare_write_buffer	= ir_prepare_write_buffer,
+	.write			= ir_write,
+	.write_room		= ir_write_room,
+	.write_bulk_callback	= ir_write_bulk_callback,
 	.process_read_urb	= ir_process_read_urb,
 };
 
@@ -254,35 +256,102 @@ static int ir_startup(struct usb_serial *serial)
 	return 0;
 }
 
-static int ir_open(struct tty_struct *tty, struct usb_serial_port *port)
+static int ir_write(struct tty_struct *tty, struct usb_serial_port *port,
+		const unsigned char *buf, int count)
 {
-	int i;
+	struct urb *urb = NULL;
+	unsigned long flags;
+	int ret;
 
-	for (i = 0; i < ARRAY_SIZE(port->write_urbs); ++i)
-		port->write_urbs[i]->transfer_flags = URB_ZERO_PACKET;
+	if (port->bulk_out_size == 0)
+		return -EINVAL;
 
-	/* Start reading from the device */
-	return usb_serial_generic_open(tty, port);
-}
+	if (count == 0)
+		return 0;
 
-static int ir_prepare_write_buffer(struct usb_serial_port *port,
-						void *dest, size_t size)
-{
-	unsigned char *buf = dest;
-	int count;
+	count = min(count, port->bulk_out_size - 1);
+
+	spin_lock_irqsave(&port->lock, flags);
+	if (__test_and_clear_bit(0, &port->write_urbs_free)) {
+		urb = port->write_urbs[0];
+		port->tx_bytes += count;
+	}
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	if (!urb)
+		return 0;
 
 	/*
 	 * The first byte of the packet we send to the device contains an
-	 * inbound header which indicates an additional number of BOFs and
+	 * outbound header which indicates an additional number of BOFs and
 	 * a baud rate change.
 	 *
 	 * See section 5.4.2.2 of the USB IrDA spec.
 	 */
-	*buf = ir_xbof | ir_baud;
+	*(u8 *)urb->transfer_buffer = ir_xbof | ir_baud;
+
+	memcpy(urb->transfer_buffer + 1, buf, count);
+
+	urb->transfer_buffer_length = count + 1;
+	urb->transfer_flags = URB_ZERO_PACKET;
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret) {
+		dev_err(&port->dev, "failed to submit write urb: %d\n", ret);
+
+		spin_lock_irqsave(&port->lock, flags);
+		__set_bit(0, &port->write_urbs_free);
+		port->tx_bytes -= count;
+		spin_unlock_irqrestore(&port->lock, flags);
+
+		return ret;
+	}
+
+	return count;
+}
+
+static void ir_write_bulk_callback(struct urb *urb)
+{
+	struct usb_serial_port *port = urb->context;
+	int status = urb->status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	__set_bit(0, &port->write_urbs_free);
+	port->tx_bytes -= urb->transfer_buffer_length - 1;
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	switch (status) {
+	case 0:
+		break;
+	case -ENOENT:
+	case -ECONNRESET:
+	case -ESHUTDOWN:
+		dev_dbg(&port->dev, "write urb stopped: %d\n", status);
+		return;
+	case -EPIPE:
+		dev_err(&port->dev, "write urb stopped: %d\n", status);
+		return;
+	default:
+		dev_err(&port->dev, "nonzero write-urb status: %d\n", status);
+		break;
+	}
+
+	usb_serial_port_softint(port);
+}
+
+static int ir_write_room(struct tty_struct *tty)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	int count = 0;
+
+	if (port->bulk_out_size == 0)
+		return 0;
+
+	if (test_bit(0, &port->write_urbs_free))
+		count = port->bulk_out_size - 1;
 
-	count = kfifo_out_locked(&port->write_fifo, buf + 1, size - 1,
-								&port->lock);
-	return count + 1;
+	return count;
 }
 
 static void ir_process_read_urb(struct urb *urb)
-- 
2.24.1

Re: [PATCH 3/5] USB: serial: ir-usb: fix IrLAP framing

[Greg KH]

On Wed, Jan 22, 2020 at 11:15:28AM +0100, Johan Hovold wrote:
> Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> switched to using the generic write implementation which may combine
> multiple write requests into larger transfers. This can break the IrLAP
> protocol where end-of-frame is determined using the USB short packet
> mechanism, for example, if multiple frames are sent in rapid succession.
> 
> Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> Cc: stable <stable@vger.kernel.org>     # 2.6.35
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
>  1 file changed, 91 insertions(+), 22 deletions(-)

Ah, nice fix, sorry about that :(

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 3/5] USB: serial: ir-usb: fix IrLAP framing

[Johan Hovold]

On Wed, Jan 22, 2020 at 02:25:42PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 22, 2020 at 11:15:28AM +0100, Johan Hovold wrote:
> > Commit f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> > switched to using the generic write implementation which may combine
> > multiple write requests into larger transfers. This can break the IrLAP
> > protocol where end-of-frame is determined using the USB short packet
> > mechanism, for example, if multiple frames are sent in rapid succession.
> > 
> > Fixes: f4a4cbb2047e ("USB: ir-usb: reimplement using generic framework")
> > Cc: stable <stable@vger.kernel.org>     # 2.6.35
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/serial/ir-usb.c | 113 +++++++++++++++++++++++++++++-------
> >  1 file changed, 91 insertions(+), 22 deletions(-)
> 
> Ah, nice fix, sorry about that :(

The offending commit was mine so same to you. ;)

> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Johan

[PATCH 4/5] USB: serial: ir-usb: make set_termios synchronous

[Johan Hovold]

Use a synchronous usb_bulk_msg() when switching link speed in
set_termios(). This way we do not need to keep track of outstanding URBs
in order to be able to stop them at close.

Note that there's no need to set URB_ZERO_PACKET as the one-byte
transfer will always be short.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 50 ++++++++-----------------------------
 1 file changed, 11 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 627bea7e6cfb..3cd70392e2a2 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -376,23 +376,15 @@ static void ir_process_read_urb(struct urb *urb)
 	tty_flip_buffer_push(&port->port);
 }
 
-static void ir_set_termios_callback(struct urb *urb)
-{
-	kfree(urb->transfer_buffer);
-
-	if (urb->status)
-		dev_dbg(&urb->dev->dev, "%s - non-zero urb status: %d\n",
-			__func__, urb->status);
-}
-
 static void ir_set_termios(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	struct urb *urb;
+	struct usb_device *udev = port->serial->dev;
 	unsigned char *transfer_buffer;
-	int result;
+	int actual_length;
 	speed_t baud;
 	int ir_baud;
+	int ret;
 
 	baud = tty_get_baud_rate(tty);
 
@@ -447,42 +439,22 @@ static void ir_set_termios(struct tty_struct *tty,
 	/*
 	 * send the baud change out on an "empty" data packet
 	 */
-	urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!urb)
-		return;
-
 	transfer_buffer = kmalloc(1, GFP_KERNEL);
 	if (!transfer_buffer)
-		goto err_buf;
+		return;
 
 	*transfer_buffer = ir_xbof | ir_baud;
 
-	usb_fill_bulk_urb(
-		urb,
-		port->serial->dev,
-		usb_sndbulkpipe(port->serial->dev,
-			port->bulk_out_endpointAddress),
-		transfer_buffer,
-		1,
-		ir_set_termios_callback,
-		port);
-
-	urb->transfer_flags = URB_ZERO_PACKET;
-
-	result = usb_submit_urb(urb, GFP_KERNEL);
-	if (result) {
-		dev_err(&port->dev, "%s - failed to submit urb: %d\n",
-							__func__, result);
-		goto err_subm;
+	ret = usb_bulk_msg(udev,
+			usb_sndbulkpipe(udev, port->bulk_out_endpointAddress),
+			transfer_buffer, 1, &actual_length, 5000);
+	if (ret || actual_length != 1) {
+		if (actual_length != 1)
+			ret = -EIO;
+		dev_err(&port->dev, "failed to change line speed: %d\n", ret);
 	}
 
-	usb_free_urb(urb);
-
-	return;
-err_subm:
 	kfree(transfer_buffer);
-err_buf:
-	usb_free_urb(urb);
 }
 
 static int __init ir_init(void)
-- 
2.24.1

Re: [PATCH 4/5] USB: serial: ir-usb: make set_termios synchronous

[Greg KH]

On Wed, Jan 22, 2020 at 11:15:29AM +0100, Johan Hovold wrote:
> Use a synchronous usb_bulk_msg() when switching link speed in
> set_termios(). This way we do not need to keep track of outstanding URBs
> in order to be able to stop them at close.
> 
> Note that there's no need to set URB_ZERO_PACKET as the one-byte
> transfer will always be short.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ir-usb.c | 50 ++++++++-----------------------------
>  1 file changed, 11 insertions(+), 39 deletions(-)
> 

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 5/5] USB: serial: ir-usb: simplify endpoint check

[Johan Hovold]

Simplify the endpoint sanity check by letting core verify that the
required endpoints are present.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ir-usb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ir-usb.c b/drivers/usb/serial/ir-usb.c
index 3cd70392e2a2..79d0586e2b33 100644
--- a/drivers/usb/serial/ir-usb.c
+++ b/drivers/usb/serial/ir-usb.c
@@ -76,6 +76,8 @@ static struct usb_serial_driver ir_device = {
 	.description		= "IR Dongle",
 	.id_table		= ir_id_table,
 	.num_ports		= 1,
+	.num_bulk_in		= 1,
+	.num_bulk_out		= 1,
 	.set_termios		= ir_set_termios,
 	.attach			= ir_startup,
 	.write			= ir_write,
@@ -197,9 +199,6 @@ static int ir_startup(struct usb_serial *serial)
 	struct usb_irda_cs_descriptor *irda_desc;
 	int rates;
 
-	if (serial->num_bulk_in < 1 || serial->num_bulk_out < 1)
-		return -ENODEV;
-
 	irda_desc = irda_usb_find_class_desc(serial, 0);
 	if (!irda_desc) {
 		dev_err(&serial->dev->dev,
-- 
2.24.1

Re: [PATCH 5/5] USB: serial: ir-usb: simplify endpoint check

[Greg KH]

On Wed, Jan 22, 2020 at 11:15:30AM +0100, Johan Hovold wrote:
> Simplify the endpoint sanity check by letting core verify that the
> required endpoints are present.
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/serial/ir-usb.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>