linux-kernel - Re: [PATCH] [usb-serial] fix Ooops on uplug

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20090721202123.404339aa@lxorguk.ukuu.org.uk>
Date:	Tue, 21 Jul 2009 20:21:23 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Daniel Mack <daniel@...aq.de>,
	Kernel development list <linux-kernel@...r.kernel.org>,
	USB list <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] [usb-serial] fix Ooops on uplug

> > 	file->f_ops = &hung_up_tty_ops;
> > 
> > 		(so the USB close will never be called)
> 
> That doesn't make sense.  The driver's open method has been called, so 
> of course the driver expects its close method to be called.

The driver close method yes - but that uses tty_port_close_start() which
sees the port was hung up and leaves well alone.

> > 	ldisc hangup
> > 	tty->ops->hangup (no-op on USB serial)
> 
> What do you mean?  There is a serial_hangup method in usb-serial.c
> and it does get called; see below.

Thats me not reading carefully. It isn't just a resource free it does
stuff. That calls drv->close() so in fact the USB layer for want of a
better word "fakes" the close.

> [  283.624088]  [<f08b09d7>] serial_hangup+0x45/0x66 [usbserial]
> [  283.624187]  [<c112018c>] do_tty_hangup+0x28c/0x2b9

So we passed

        /* This breaks for file handles being sent over AF_UNIX sockets ?
        */ list_for_each_entry(filp, &tty->tty_files, f_u.fu_list) {
                if (filp->f_op->write == redirected_tty_write)
                        cons_filp = filp;
                if (filp->f_op->write != tty_write)
                        continue;
                closecount++;
                tty_fasync(-1, filp, 0);        /* can't block */
                filp->f_op = &hung_up_tty_fops;
        }

and changed the fops. As you say my theory was completely wrong

> Close the open file descriptor:
> [  291.227977] tty_release_dev of tty2 (tty count=2)...
> [  291.230492] tty_release_dev of ttyUSB0 (tty count=1)...
> [  291.230630] serial_close port 107 (ef7fd920)
> 
> That line was inserted in serial_close.  As you can see, the port 
> number is wrong because the port structure has already been 
> deallocated by port_free.  And that leads to the following corruption.

Bingo - and that in turn means that the tty layer doesn't realise the
port has been hung up which makes tty_port_close_start do random things
which causes us to double free.

So in fact we need to delay the resource free until the tty layer has
really finished with it as the port resource contains the tty layer port.

We can't just skip freeing the resources in the hangup method as
tty_port_close_start() will return 0 and leak them on a hangup.

Alan: does this make sense

	Take an extra tty layer reference to the usb_serial at open time

	Put that reference in the tty shutdown() hook which is called
	when the tty struct gets its final kref_put (ie after the close,
	and if there is any outstanding other use eg in an IRQ handler on
	another processor).

Am I understanding the usb_serial_port lifetime correctly ?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/