Re: [PATCH] tty: plug a use-after-free in TIOCGETD ioctl

[Greg Kroah-Hartman <gregkh@linuxfoundation.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 <milos@redhat.com>
> > > Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
> > > ---
> > >  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