linux-kernel - Re: flush_to_ldisc accesses tty after free (was: [PATCH 21/21] TTY: move tty buffers to tty

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]

Date:	Sat, 01 Dec 2012 15:06:23 -0500
From:	Peter Hurley <peter@...leysoftware.com>
To:	Jiri Slaby <jslaby@...e.cz>, Jiri Slaby <jirislaby@...il.com>,
	alan@...ux.intel.com
Cc:	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	Dave Jones <davej@...hat.com>,
	Sasha Levin <levinsasha928@...il.com>
Subject: Re: flush_to_ldisc accesses tty after free  (was: [PATCH 21/21]
 TTY: move tty buffers to tty_port)

On Sat, 2012-12-01 at 09:59 -0500, Peter Hurley wrote:
....
> From instrumenting the tty_release() path, it's clear that tty_buffer
> work is still scheduled even after tty_release_ldisc() has run. For
> example, with this patch I get the warning below it.
> 
> [Further analysis to follow in subsequent mail...]

[ Please note: this analysis only refers to the pty driver. The
situation with hardware drivers has further complications.]

Firstly, this problem predates Jiri's changes; only because he was
cautious by checking the lifetime of the itty in flush_to_ldisc(), did
he uncover this existing problem.

One example of how it is possible for buffer work to be scheduled even
after tty_release_ldisc() stems from how tty_ldisc_halt() works (or
rather doesn't). (I've snipped out the relevant code from tty_ldisc.c
for annotation below.)

tty_ldisc_halt() has only 2 callers; tty_release_ldisc() and
tty_set_ldisc(). A 3rd code site -- tty_ldisc_hangup() -- has similar
logic.

The idea behind tty_ldisc_halt() is to prevent __future__ use of this
ldisc (since users are required to acquire an ldisc reference via
tty_ldisc_try() -- also below).  Annotations are mine.

----------------------------------

static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty)
{
	unsigned long flags;
	struct tty_ldisc *ld;

	/* this spin_lock is irrelevant to this discussion */
	spin_lock_irqsave(&tty_ldisc_lock, flags);
	ld = NULL;

	/*
	 * Atomically test if __new__ ldisc references are
	 * allowed. Please note, there can be any number of
	 * existing users (ie., outstanding references).
	 */
	if (test_bit(TTY_LDISC, &tty->flags))
		ld = get_ldisc(tty->ldisc);

	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
	return ld;
}

static int tty_ldisc_halt(struct tty_struct *tty)
{
	/* Prevent any __new__ ldisc references from being acquired. */

	clear_bit(TTY_LDISC, &tty->flags);

	/* Since __existing__ ldisc references can still schedule new
	 * buffer work (via tty_flip_buffer_push()), the cancellation
	 * below is pointless. The instant that cancellation completes
	 * an existing ldisc user can schedule new work.
	 *
	 * At a minimum, we must wait for all ldisc references **here**
	 * rather than **after** cancelling the work.
	 */

	return cancel_work_sync(&tty->buf.work);
}



**** A special note about locking *****

Locking around tty_ldisc_halt() ...
- does not prevent existing ldisc users from continuing to use the ldisc
- is unnecessary for the 3 'callers' because all 3 are trying to
accomplish the same goal by the same means
- can deadlock. Reference: https://lkml.org/lkml/2012/11/21/267

Regards,
Peter Hurley

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