From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752862AbcAGRLo (ORCPT ); Thu, 7 Jan 2016 12:11:44 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:36346 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752150AbcAGRLm (ORCPT ); Thu, 7 Jan 2016 12:11:42 -0500 Date: Thu, 7 Jan 2016 09:11:41 -0800 From: Greg Kroah-Hartman To: Mateusz Guzik Cc: Jiri Slaby , stable@vger.kernel.org, linux-kernel@vger.kernel.org, security@kernel.org, milos@redhat.com Subject: Re: [PATCH] tty: plug a use-after-free in TIOCGETD ioctl Message-ID: <20160107171141.GA28979@kroah.com> References: <1452178680-30284-1-git-send-email-mguzik@redhat.com> <20160107153310.GA26031@kroah.com> <20160107162112.GD14492@mguzik> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160107162112.GD14492@mguzik> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 07, 2016 at 05:21:14PM +0100, Mateusz Guzik wrote: > On Thu, Jan 07, 2016 at 07:33:10AM -0800, Greg Kroah-Hartman wrote: > > On Thu, Jan 07, 2016 at 03:58:00PM +0100, Mateusz Guzik wrote: > > > When the line discipline is being changed, the old one is freed. > > > However, the handler for TIOCGETD would dereference it without taking > > > any locks, in effect possibly reading freed memory. > > > > > > Line discipline changes are protected with tty lock. Use it on reader > > > side as well. > > > > > > CVE: CVE-2016-0723 > > > > Why a cve tag? > > > > Red Hat SRT assigned a CVE and asked me to included in the commit > message. I did a quick check how people mark such stuff and found the > tag. I definitely don't insist on having it mentioned. It seems odd that any random kernel bug can get assigned a CVE without actually talking to the developers first, but whatever... > > > Found-by: Milos Vyletel > > > Signed-off-by: Mateusz Guzik > > > --- > > > drivers/tty/tty_io.c | 23 ++++++++++++++++++++++- > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > > > index 892c923..1b10469 100644 > > > --- a/drivers/tty/tty_io.c > > > +++ b/drivers/tty/tty_io.c > > > @@ -2626,6 +2626,27 @@ static int tiocgsid(struct tty_struct *tty, struct tty_struct *real_tty, pid_t _ > > > } > > > > > > /** > > > + * tiocgetd - get line discipline > > > + * @tty: tty device > > > + * @p: pointer to returned line discipline > > > + * > > > + * Get the line discipline associated with the tty. > > > + * > > > + * Locking: none > > > + */ > > > + > > > +static int tiocgetd(struct tty_struct *tty, int __user *p) > > > +{ > > > + int ldisc; > > > + > > > + tty_lock(tty); > > > + ldisc = tty->ldisc->ops->num; > > > + tty_unlock(tty); > > > + > > > + return put_user(ldisc, p); > > > > Does this really protect anything? What is preventing ldisc from going > > away right after the tty_unlock call? > > I guess I should have elaborated, sorry. > > Yes, ldisc can be freed just after tty_unlock, but it does not matter. > There is only a need to store the number (which is done with the lock > held) and line discipline is not touched afterwards. You don't store the number, you store a pointer, which isn't good. If you want to just store the number, just store the number. > > And how are you able to trigger the tty to go away while the file is > > still held open and this ioctl is being called? > > > > It's not the tty going away, but the memory pointed to by previous value > of tty->ldisc. > > tty_set_ldisc will reassign tty->ldisc to a new value, and will later > free the old one with tty_ldisc_put. > > In the current code TIOCGETD is: > return put_user(tty->ldisc->ops->num, (int __user *)p); > > A thread doing this ioctl can load tty->ldisc's value, but memory > pointed to it can be freed before it loads ops's address. But your fix doesn't solve this, you are keeping a stale pointer around as the ldisc could have gone away. See Peter's fix for the "correct" way to solve this. thanks, greg k-h