On Mon, Mar 19, 2012 at 12:57:19PM +0800, Ken Huang wrote:
> 2012/3/17 Johan Hovold <jhovold@xxxxxxxxx>:
> > Fix race between probe and open by making sure that the disconnected
> > flag is not cleared until all ports have been registered and the serial
> > struct is fully initialised.
> >
> > A call to tty_open while probe is running may get a reference to the
> > serial structure in serial_install before its ports have been
> > registered. This may lead to usb_serial_core calling driver open before
> > port is fully initialised.
> >
> > With ftdi_sio this result in the following NULL-pointer dereference as
> > the private data has not been initialised at open:
> >
> > [ 199.698286] IP: [<f811a089>] ftdi_open+0x59/0xe0 [ftdi_sio]
> > [ 199.698297] *pde = 00000000
> > [ 199.698303] Oops: 0000 [#1] PREEMPT SMP
> > [ 199.698313] Modules linked in: ftdi_sio usbserial
> > [ 199.698323]
> > [ 199.698327] Pid: 1146, comm: ftdi_open Not tainted 3.2.11 #70 Dell Inc. Vostro 1520/0T816J
> > [ 199.698339] EIP: 0060:[<f811a089>] EFLAGS: 00010286 CPU: 0
> > [ 199.698344] EIP is at ftdi_open+0x59/0xe0 [ftdi_sio]
> > [ 199.698348] EAX: 0000003e EBX: f5067000 ECX: 00000000 EDX: 80000600
> > [ 199.698352] ESI: f48d8800 EDI: 00000001 EBP: f515dd54 ESP: f515dcfc
> > [ 199.698356] DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> > [ 199.698361] Process ftdi_open (pid: 1146, ti=f515c000 task=f481e040 task.ti=f515c000)
> > [ 199.698364] Stack:
> > [ 199.698368] f811a9fe f811a9e0 f811b3ef 00000000 00000000 00001388 00000000 f4a86800
> > [ 199.698387] 00000002 00000000 f806e68e 00000000 f532765c f481e040 00000246 22222222
> > [ 199.698479] 22222222 22222222 22222222 f5067004 f5327600 f5327638 f515dd74 f806e6ab
> > [ 199.698496] Call Trace:
> > [ 199.698504] [<f806e68e>] ? serial_activate+0x2e/0x70 [usbserial]
> > [ 199.698511] [<f806e6ab>] serial_activate+0x4b/0x70 [usbserial]
> > [ 199.698521] [<c126380c>] tty_port_open+0x7c/0xd0
> > [ 199.698527] [<f806e660>] ? serial_set_termios+0xa0/0xa0 [usbserial]
> > [ 199.698534] [<f806e76f>] serial_open+0x2f/0x70 [usbserial]
> > [ 199.698540] [<c125d07c>] tty_open+0x20c/0x510
> > [ 199.698546] [<c10e9eb7>] chrdev_open+0xe7/0x230
> > [ 199.698553] [<c10e48f2>] __dentry_open+0x1f2/0x390
> > [ 199.698559] [<c144bfec>] ? _raw_spin_unlock+0x2c/0x50
> > [ 199.698565] [<c10e4b76>] nameidata_to_filp+0x66/0x80
> > [ 199.698570] [<c10e9dd0>] ? cdev_put+0x20/0x20
> > [ 199.698576] [<c10f3e08>] do_last+0x198/0x730
> > [ 199.698581] [<c10f4440>] path_openat+0xa0/0x350
> > [ 199.698587] [<c10f47d5>] do_filp_open+0x35/0x80
> > [ 199.698593] [<c144bfec>] ? _raw_spin_unlock+0x2c/0x50
> > [ 199.698599] [<c10ff110>] ? alloc_fd+0xc0/0x100
> > [ 199.698605] [<c10f0b72>] ? getname_flags+0x72/0x120
> > [ 199.698611] [<c10e4450>] do_sys_open+0xf0/0x1c0
> > [ 199.698617] [<c11fcc08>] ? trace_hardirqs_on_thunk+0xc/0x10
> > [ 199.698623] [<c10e458e>] sys_open+0x2e/0x40
> > [ 199.698628] [<c144c990>] sysenter_do_call+0x12/0x36
> > [ 199.698632] Code: 85 89 00 00 00 8b 16 8b 4d c0 c1 e2 08 c7 44 24 14 88 13 00 00 81 ca 00 00 00 80 c7 44 24 10 00 00 00 00 c7 44 24 0c 00 00 00 00 <0f> b7 41 78 31 c9 89 44 24 08 c7 44 24 04 00 00 00 00 c7 04 24
> > [ 199.698884] EIP: [<f811a089>] ftdi_open+0x59/0xe0 [ftdi_sio] SS:ESP 0068:f515dcfc
> > [ 199.698893] CR2: 0000000000000078
> > [ 199.698925] ---[ end trace 77c43ec023940cff ]---
> >
> > Reported-by: Ken Huang <csuhgw@xxxxxxxxx>
> > Cc: stable <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Johan Hovold <jhovold@xxxxxxxxx>
> > ---
> > drivers/usb/serial/usb-serial.c | 7 +++++++
> > 1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> > index cc274fd..cb08235 100644
> > --- a/drivers/usb/serial/usb-serial.c
> > +++ b/drivers/usb/serial/usb-serial.c
> > @@ -1059,6 +1059,12 @@ int usb_serial_probe(struct usb_interface *interface,
> > serial->attached = 1;
> > }
> >
> > + /* Avoid race with tty_open and serial_install by setting the
> > + * disconnected flag and not clearing it until all ports are
> > + * registered and the serial struct is fully initialised.
> > + */
> > + serial->disconnected = 1;
> > +
> > if (get_free_serial(serial, num_ports, &minor) == NULL) {
> > dev_err(&interface->dev, "No more free serial devices\n");
> > goto probe_error;
> > @@ -1088,6 +1094,7 @@ int usb_serial_probe(struct usb_interface *interface,
> > exit:
> > /* success */
> > usb_set_intfdata(interface, serial);
> > + serial->disconnected = 0;
> > module_put(type->driver.owner);
> > return 0;
> >
> > --
> > 1.7.8.4
> >
>
> Here is my fixed code according to yours:
>
> diff --git a/serial/usb-serial.c b/serial/usb-serial.c
> index 131fc74..2ba1538 100644
> --- a/serial/usb-serial.c
> +++ b/serial/usb-serial.c
> @@ -985,6 +985,7 @@ int usb_serial_probe(struct usb_interface *interface,
> goto exit;
> }
> }
> + serial->disconnected = 1;
>
> if (get_free_serial(serial, num_ports, &minor) == NULL) {
> dev_err(&interface->dev, "No more free serial devices\n");
> @@ -1013,6 +1014,7 @@ int usb_serial_probe(struct usb_interface *interface,
> exit:
> /* success */
> usb_set_intfdata(interface, serial);
> + serial->disconnected = 0;
> return 0;
>
> Is that right? does it cause any side effects?
That looks right but you can just apply the patch e.g. using git am.
There should be no side effects.
/Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
