linux-kernel - Re: [PATCH] kdesu broken

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: <20090729120704.3299ff2e@lxorguk.ukuu.org.uk>
Date:	Wed, 29 Jul 2009 12:07:04 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Gene Heskett <gene.heskett@...izon.net>,
	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Ray Lee <ray-lk@...rabbit.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH] kdesu broken

On Tue, 28 Jul 2009 21:49:05 -0700 (PDT)
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> 
> 
> On Tue, 28 Jul 2009, Gene Heskett wrote:
> > 
> > It doesn't seem to do anything for bz 13841 here.  Sorry, I wish it did.
> 
> No, bz 13841 isn't about pty's, it's about usb serial. The patches in this 
> thread would mainly be pty-related, and would affect other tty drivers 
> (like your usb one) only purely by chance.
> 
> could you bisect the 13841 behavior?

See the thread in 13821 on linux-usb list which extends from the "why do
we leak ttyUSB numbers" into this as well. Its been pinned down and the in
tree fix is confirmed to do the job for that case. Not perfectly because
it needs a further small fix as

a) it should call drv->close() within the mutex on the error path if the
tty_block_til_ready() fails without which you get a leak on a few drivers
but nothing too harmful. (Noted by Alan Stern inspecting the patch)

b) it fails the 'variable frequency generator on carrier pin' test - but
that isn't a regression as such as all 2.6 kernels I've tried crash
during that test with USB.  (Running my 'extreme violence' test setup)

I suspect that doing

        retval = tty_port_block_til_ready(&port->port, tty, filp);
        if (retval == 0) {
                if (!first)
                        usb_serial_put(serial);
                return 0;
        }
        mutex_lock(&port->mutex);

	if (tty_hung_up_p(filp)) {
		mutex_unlock(&port->mutex);
		return retval;
	}
	dev->close(port);

	...

fixes both. The race basically is

		open
		drop mutex so we can wait for the port
		wait for the port
						[hangup]
						[serial_do_down]
		block_til_ready reports an error
		take mutex
			duplicate serial_do_down

Most serial drivers don't try and do open clean up in open() instead they
do it in close() which is called by the tty layer on an open failure
(always has been) and which turns out to be a useful design
decision as it means the driver doesn't have to tie itself in knots and
tty_port_close_start() handles all the mechanics. Plus they use
ASYNC_INITIALIZED as flag to avoid double shutdowns on a device.

Simply deleting most of the clean up from serial_open and letting close()
do its job would I think clean that up no end but not in an -rc perhaps.

--
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/