linux-kernel - Re: [PATCH] [98/223] TTY: open/hangup race fixup

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: <4D05E6BB.3050108@suse.cz>
Date:	Mon, 13 Dec 2010 10:26:19 +0100
From:	Jiri Slaby <jslaby@...e.cz>
To:	Andi Kleen <andi@...stfloor.org>
CC:	alan@...rguk.ukuu.org.uk, gregkh@...e.de, ak@...ux.intel.com,
	linux-kernel@...r.kernel.org, stable@...nel.org
Subject: Re: [PATCH] [98/223] TTY: open/hangup race fixup

On 12/13/2010 12:46 AM, Andi Kleen wrote:
> 2.6.35-longterm review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> From: Jiri Slaby <jslaby@...e.cz>
> 
> commit acfa747baf73922021a047f2d87a2d866f5dbab5 upstream.
> 
> Like in the "TTY: don't allow reopen when ldisc is changing" patch,
> this one fixes a TTY WARNING as described in the option 1) there:
> 1) __tty_hangup from tty_ldisc_hangup to tty_ldisc_enable. During this
> section tty_lock is held. However tty_lock is temporarily dropped in
> the middle of the function by tty_ldisc_hangup.
> 
> The fix is to introduce a new flag which we set during the unlocked
> window and check it in tty_reopen too. The flag is TTY_HUPPING and is
> cleared after TTY_HUPPED is set.
> 
> While at it, remove duplicate TTY_HUPPED set_bit. The one after
> calling ops->hangup seems to be more correct. But anyway, we hold
> tty_lock, so there should be no difference.
> 
> Also document the function it does that kind of crap.
> 
> Nicely reproducible with two forked children:
> static void do_work(const char *tty)
> {
> 	if (signal(SIGHUP, SIG_IGN) == SIG_ERR) exit(1);
> 	setsid();
> 	while (1) {
> 		int fd = open(tty, O_RDWR|O_NOCTTY);
> 		if (fd < 0) continue;
> 		if (ioctl(fd, TIOCSCTTY)) continue;
> 		if (vhangup()) continue;
> 		close(fd);
> 	}
> 	exit(0);
> }
> 
> Signed-off-by: Jiri Slaby <jslaby@...e.cz>
> Reported-by: <Valdis.Kletnieks@...edu>
> Reported-by: Kyle McMartin <kyle@...artin.ca>
> Cc: Alan Cox <alan@...rguk.ukuu.org.uk>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> 
> ---
>  drivers/char/tty_io.c |   10 +++++++++-
>  include/linux/tty.h   |    1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/char/tty_io.c
> ===================================================================
> --- linux.orig/drivers/char/tty_io.c
> +++ linux/drivers/char/tty_io.c
> @@ -514,7 +514,10 @@ static void do_tty_hangup(struct work_st
>  	spin_unlock(&redirect_lock);
>  
>  	/* inuse_filps is protected by the single kernel lock */
> +
>  	lock_kernel();
> +	/* some functions below drop BTM, so we need this bit */
> +	set_bit(TTY_HUPPING, &tty->flags);

This and 97/223 should not be needed in 2.6.35 -- there is BKL
protecting the race. IOW BKL removal in 2.6.36 introduced the regression
these 2 patches fix.

regards,
-- 
js
suse labs
--
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/