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

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

[Greg Kroah-Hartman]

Adding Peter and linux-serial to the list here, as Peter has been doing
a ton of work in this area...

Peter, does this seem sane with the tty locking rules?

thanks,

greg k-h

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
> 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);
> +}
> +
> +/**
>   *	tiocsetd	-	set line discipline
>   *	@tty: tty device
>   *	@p: pointer to user data
> @@ -2874,7 +2895,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case TIOCGSID:
>  		return tiocgsid(tty, real_tty, p);
>  	case TIOCGETD:
> -		return put_user(tty->ldisc->ops->num, (int __user *)p);
> +		return tiocgetd(tty, p);
>  	case TIOCSETD:
>  		return tiocsetd(tty, p);
>  	case TIOCVHANGUP:
> -- 
> 1.8.3.1

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

[Peter Hurley]

On 01/07/2016 08:14 AM, Greg Kroah-Hartman wrote:
> Adding Peter and linux-serial to the list here, as Peter has been doing
> a ton of work in this area...
> 
> Peter, does this seem sane with the tty locking rules?
> 
> thanks,
> 
> greg k-h

No.

Fix for this is right here:
http://lkml.iu.edu/hypermail/linux/kernel/1511.3/03045.html

Regards,
Peter Hurley

> 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
>> 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);
>> +}
>> +
>> +/**
>>   *	tiocsetd	-	set line discipline
>>   *	@tty: tty device
>>   *	@p: pointer to user data
>> @@ -2874,7 +2895,7 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>  	case TIOCGSID:
>>  		return tiocgsid(tty, real_tty, p);
>>  	case TIOCGETD:
>> -		return put_user(tty->ldisc->ops->num, (int __user *)p);
>> +		return tiocgetd(tty, p);
>>  	case TIOCSETD:
>>  		return tiocsetd(tty, p);
>>  	case TIOCVHANGUP:
>> -- 
>> 1.8.3.1

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

[Greg Kroah-Hartman]

On Thu, Jan 07, 2016 at 08:38:04AM -0800, Peter Hurley wrote:
> On 01/07/2016 08:14 AM, Greg Kroah-Hartman wrote:
> > Adding Peter and linux-serial to the list here, as Peter has been doing
> > a ton of work in this area...
> > 
> > Peter, does this seem sane with the tty locking rules?
> > 
> > thanks,
> > 
> > greg k-h
> 
> No.
> 
> Fix for this is right here:
> http://lkml.iu.edu/hypermail/linux/kernel/1511.3/03045.html

Ah, I thought I had seen this before, thanks for the reminder :)

But, I didn't apply that series for some reason, was it waiting on
something before that?  Or did I just miss it with the holliday break?

thanks,

greg k-h

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

[Peter Hurley]

On 01/07/2016 09:08 AM, Greg Kroah-Hartman wrote:
> On Thu, Jan 07, 2016 at 08:38:04AM -0800, Peter Hurley wrote:
>> On 01/07/2016 08:14 AM, Greg Kroah-Hartman wrote:
>>> Adding Peter and linux-serial to the list here, as Peter has been doing
>>> a ton of work in this area...
>>>
>>> Peter, does this seem sane with the tty locking rules?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> No.
>>
>> Fix for this is right here:
>> http://lkml.iu.edu/hypermail/linux/kernel/1511.3/03045.html
> 
> Ah, I thought I had seen this before, thanks for the reminder :)
> 
> But, I didn't apply that series for some reason, was it waiting on
> something before that?  Or did I just miss it with the holliday break?

That series is good to go, and in fact fixes a rash of crash
reports which occur on kernels since 3.10, but just started showing
up now (triggered by some interaction between consoles over terminal servers
and systemd).

I didn't bother you about it because it seemed like you were busy.
Do you need me to resend this series (and the other 6 series' plus
misc fixes)?

Regards,
Peter Hurley

PS - I meant to get to plumbers' last summer but overdue work
kept me from going. This would have been easier with a tree to pull from.

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

[Greg Kroah-Hartman]

On Thu, Jan 07, 2016 at 09:38:56AM -0800, Peter Hurley wrote:
> On 01/07/2016 09:08 AM, Greg Kroah-Hartman wrote:
> > On Thu, Jan 07, 2016 at 08:38:04AM -0800, Peter Hurley wrote:
> >> On 01/07/2016 08:14 AM, Greg Kroah-Hartman wrote:
> >>> Adding Peter and linux-serial to the list here, as Peter has been doing
> >>> a ton of work in this area...
> >>>
> >>> Peter, does this seem sane with the tty locking rules?
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >>
> >> No.
> >>
> >> Fix for this is right here:
> >> http://lkml.iu.edu/hypermail/linux/kernel/1511.3/03045.html
> > 
> > Ah, I thought I had seen this before, thanks for the reminder :)
> > 
> > But, I didn't apply that series for some reason, was it waiting on
> > something before that?  Or did I just miss it with the holliday break?
> 
> That series is good to go, and in fact fixes a rash of crash
> reports which occur on kernels since 3.10, but just started showing
> up now (triggered by some interaction between consoles over terminal servers
> and systemd).
> 
> I didn't bother you about it because it seemed like you were busy.
> Do you need me to resend this series (and the other 6 series' plus
> misc fixes)?

Sure, a resend would be good to have, if you can do that.

thanks,

greg k-h