[PATCH 0/2] USB: symbolserial: Fix OOPS (regression) and data length

[PATCH 0/2] USB: symbolserial: Fix OOPS (regression) and data length

[Philipp Hachtmann]

I bought a Symbol DS3508 2D imager barcode scanner, configured it to
behave as "simple COM port", and my machine OOPSed immediately.

The result are the two attached patches. The first one is actually needed
to get the whole thing again without that nasty NULL pointer dereference.

The second patch makes the driver exclude useless zero bytes from the payload
data. I assume this could lead to discussions because I found (and removed)
a comment that says that the driver will send all data from the urb buffer to
the tty layer without taking the communicated length (from the scanner, in
the first byte of a buffer) into account.

For your amusement here's what happened to my machine before writing the first
patch:

[ 3801.712295] usb 6-2: new full-speed USB device number 12 using uhci_hcd
[ 3801.883708] usb 6-2: New USB device found, idVendor=05e0, idProduct=0600
[ 3801.883712] usb 6-2: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[ 3801.883714] usb 6-2: Product: Symbol Bar Code Scanner
[ 3801.883716] usb 6-2: Manufacturer: Symbol Technologies, Inc, 2008
[ 3801.883718] usb 6-2: SerialNumber: S/N:5520B838ED22C34CBC619E74103732B8 Rev:PAAALS00-002-R01-
[ 3801.889804] symbolserial 6-2:1.0: symbol converter detected
[ 3801.889962] usb 6-2: symbol converter now attached to ttyUSB0
[ 3801.896738] BUG: unable to handle kernel NULL pointer dereference at           (null)
[ 3801.896741] IP: [<ffffffff817ef2ab>] _raw_spin_lock_irqsave+0xb/0x30
[ 3801.896746] PGD 61fb5b067 PUD 6208dd067 PMD 0 
[ 3801.896748] Oops: 0002 [#1] SMP 
[ 3801.896749] Modules linked in: nvidia(O)
[ 3801.896751] CPU: 7 PID: 8640 Comm: ModemManager Tainted: G           O    4.2.0-rc7 #27
[ 3801.896752] Hardware name: System manufacturer System Product Name/P6T WS PRO, BIOS 1204    09/16/2010
[ 3801.896753] task: ffff88061daaba80 ti: ffff8800ba608000 task.ti: ffff8800ba608000
[ 3801.896754] RIP: 0010:[<ffffffff817ef2ab>]  [<ffffffff817ef2ab>] _raw_spin_lock_irqsave+0xb/0x30
[ 3801.896756] RSP: 0018:ffff8800ba60bba8  EFLAGS: 00010046
[ 3801.896757] RAX: 0000000000000000 RBX: 0000000000000296 RCX: ffff8806220b4800
[ 3801.896757] RDX: 0000000000000001 RSI: ffff8806220b4800 RDI: 0000000000000000
[ 3801.896758] RBP: ffff8806220b4800 R08: 0000000000000001 R09: ffffc900004d6000
[ 3801.896759] R10: 00003ffffffff000 R11: ffffc900004d5fff R12: ffff880623832780
[ 3801.896759] R13: ffff880591b09000 R14: ffff880591b09000 R15: ffff88061daaba80
[ 3801.896760] FS:  00007fc35e569840(0000) GS:ffff88063fce0000(0000) knlGS:0000000000000000
[ 3801.896761] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3801.896762] CR2: 0000000000000000 CR3: 000000061df01000 CR4: 00000000000006e0
[ 3801.896762] Stack:
[ 3801.896763]  0000000000000000 ffffffff8153267b ffff8806243dd0a8 ffff8806220b4808
[ 3801.896764]  ffff8806238327e8 ffffffff81511cc6 ffff880591b09000 ffffffff813c4b2e
[ 3801.896765]  ffff880591b09000 ffff8806220b4808 ffff880591b09000 ffff8806220b4918
[ 3801.896766] Call Trace:
[ 3801.896769]  [<ffffffff8153267b>] ? symbol_open+0x1b/0x70
[ 3801.896772]  [<ffffffff81511cc6>] ? serial_port_activate+0x56/0x90
[ 3801.896775]  [<ffffffff813c4b2e>] ? tty_port_tty_set+0x2e/0x90
[ 3801.896776]  [<ffffffff813c50c6>] ? tty_port_open+0x76/0xc0
[ 3801.896778]  [<ffffffff813bd94f>] ? tty_init_dev+0x8f/0x1a0
[ 3801.896779]  [<ffffffff813bdb55>] ? tty_open+0xf5/0x5a0
[ 3801.896782]  [<ffffffff8118c2f0>] ? mount_fs+0x160/0x160
[ 3801.896784]  [<ffffffff8118cabc>] ? chrdev_open+0x9c/0x170
[ 3801.896785]  [<ffffffff8118ca20>] ? cdev_put+0x20/0x20
[ 3801.896786]  [<ffffffff81186d00>] ? do_dentry_open+0x1c0/0x2f0
[ 3801.896788]  [<ffffffff81194bd0>] ? path_openat+0x380/0x1030
[ 3801.896789]  [<ffffffff81191982>] ? terminate_walk+0xa2/0xb0
[ 3801.896790]  [<ffffffff81196a60>] ? do_filp_open+0x70/0xd0
[ 3801.896792]  [<ffffffff811a1a97>] ? __alloc_fd+0x37/0x100
[ 3801.896794]  [<ffffffff8118808e>] ? do_sys_open+0x11e/0x200
[ 3801.896795]  [<ffffffff817ef617>] ? entry_SYSCALL_64_fastpath+0x12/0x6a
[ 3801.896795] Code: 31 c0 ba 01 00 00 00 f0 0f b1 17 85 c0 75 02 f3 c3 89 c6 e9 68 6d 91 ff 0f 1f 84 00 00 00 00 00 53 9c 5b fa 31 c0 ba 01 00 00 00 <f0> 0f b1 17 85 c0 75 05 48 89 d8 5b c3 89 c6 e8 41 6d 91 ff 48 
[ 3801.896809] RIP  [<ffffffff817ef2ab>] _raw_spin_lock_irqsave+0xb/0x30
[ 3801.896811]  RSP <ffff8800ba60bba8>
[ 3801.896812] CR2: 0000000000000000
[ 3801.896813] ---[ end trace 196415182e1cd411 ]---


Philipp Hachtmann (2):
  USB: symbolserial: Use usb_get_serial_port_data
  USB: symbolserial: Correct transferred data size

 drivers/usb/serial/symbolserial.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.1.4

[PATCH 1/2] USB: symbolserial: Use usb_get_serial_port_data

[Philipp Hachtmann]

The driver used usb_get_serial_data(port->serial) which compiled but resulted
in a NULL pointer being returned (and subsequently used). I did not go deeper
into this but I guess this is a regression.

Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
---
 drivers/usb/serial/symbolserial.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/symbolserial.c b/drivers/usb/serial/symbolserial.c
index 8fceec7..6ed8044 100644
--- a/drivers/usb/serial/symbolserial.c
+++ b/drivers/usb/serial/symbolserial.c
@@ -94,7 +94,7 @@ exit:
 
 static int symbol_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
-	struct symbol_private *priv = usb_get_serial_data(port->serial);
+	struct symbol_private *priv = usb_get_serial_port_data(port);
 	unsigned long flags;
 	int result = 0;
 
@@ -120,7 +120,7 @@ static void symbol_close(struct usb_serial_port *port)
 static void symbol_throttle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct symbol_private *priv = usb_get_serial_data(port->serial);
+	struct symbol_private *priv = usb_get_serial_port_data(port);
 
 	spin_lock_irq(&priv->lock);
 	priv->throttled = true;
@@ -130,7 +130,7 @@ static void symbol_throttle(struct tty_struct *tty)
 static void symbol_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-	struct symbol_private *priv = usb_get_serial_data(port->serial);
+	struct symbol_private *priv = usb_get_serial_port_data(port);
 	int result;
 	bool was_throttled;
 
-- 
2.1.4

Re: [PATCH 1/2] USB: symbolserial: Use usb_get_serial_port_data

[Greg KH]

On Mon, Aug 17, 2015 at 05:31:46PM +0200, Philipp Hachtmann wrote:
> The driver used usb_get_serial_data(port->serial) which compiled but resulted
> in a NULL pointer being returned (and subsequently used). I did not go deeper
> into this but I guess this is a regression.
> 
> Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
> ---
>  drivers/usb/serial/symbolserial.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Doh, obviously no one has used this driver in a long time, thanks for
these patches...

Johan, mind if I take these directly?

thanks,

greg k-h

Re: [PATCH 1/2] USB: symbolserial: Use usb_get_serial_port_data

[Johan Hovold]

On Mon, Aug 17, 2015 at 09:06:15AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Aug 17, 2015 at 05:31:46PM +0200, Philipp Hachtmann wrote:
> > The driver used usb_get_serial_data(port->serial) which compiled but resulted
> > in a NULL pointer being returned (and subsequently used). I did not go deeper
> > into this but I guess this is a regression.
> > 
> > Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
> > ---
> >  drivers/usb/serial/symbolserial.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Doh, obviously no one has used this driver in a long time, thanks for
> these patches...

Wow...

> Johan, mind if I take these directly?

Please do.

This one should be tagged:

Fixes: a85796ee5149 ("USB: symbolserial: move private-data allocation to
port_probe")
Cc: stable <stable@vger.kernel.org>	# v3.10

And for both patches:

Acked-by: Johan Hovold <johan@kernel.org>

Thanks,
Johan

[PATCH 2/2] USB: symbolserial: Correct transferred data size

[Philipp Hachtmann]

The scanner (here DS3508) always returns 64 bytes per urb buffer. The first
byte indicates the data length used in the current buffer. There even was
a comment describing this. But the comment also said that we'll send
everything in the buffer to the tty layer. That means sending the actual
barcode data and lots of trailing zeroes. This patch lets the driver only
send the real data.

Signed-off-by: Philipp Hachtmann <hachti@hachti.de>
---
 drivers/usb/serial/symbolserial.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/serial/symbolserial.c b/drivers/usb/serial/symbolserial.c
index 6ed8044..37f3ad1 100644
--- a/drivers/usb/serial/symbolserial.c
+++ b/drivers/usb/serial/symbolserial.c
@@ -60,17 +60,15 @@ static void symbol_int_callback(struct urb *urb)
 
 	usb_serial_debug_data(&port->dev, __func__, urb->actual_length, data);
 
+	/*
+	 * Data from the device comes with a 1 byte header:
+	 *
+	 * <size of data> <data>...
+	 */
 	if (urb->actual_length > 1) {
-		data_length = urb->actual_length - 1;
-
-		/*
-		 * Data from the device comes with a 1 byte header:
-		 *
-		 * <size of data>data...
-		 * 	This is real data to be sent to the tty layer
-		 * we pretty much just ignore the size and send everything
-		 * else to the tty layer.
-		 */
+		data_length = data[0];
+		if (data_length > (urb->actual_length - 1))
+			data_length = urb->actual_length - 1;
 		tty_insert_flip_string(&port->port, &data[1], data_length);
 		tty_flip_buffer_push(&port->port);
 	} else {
-- 
2.1.4