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

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

[Mateusz Guzik]

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

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

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?

thanks,

greg k-h

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

[Mateusz Guzik]

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.


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

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

The race can be triggered in few seconds on bare metal:

[  428.520966] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
[  428.529728] IP: [<ffffffff813b04cb>] tty_ioctl+0x89b/0xbc0
[  428.535864] PGD 35a25067 PUD 36662067 PMD 0 
[  428.540659] Oops: 0000 [#1] SMP 
[  428.544277] Modules linked in: ppp_async ppp_generic slhc crc_ccitt intel_powerclamp coretemp intel_rapl kvm_intel kvm crc32_pclmul g
hash_clmulni_intel aesni_intel iTCO_wdt lrw gf128mul glue_helper ablk_helper ipmi_ssif cryptd iTCO_vendor_support sg ipmi_devintf dcdbas
 sb_edac lpc_ich edac_core pcspkr mfd_core ipmi_si ipmi_msghandler mei_me mei wmi shpchp acpi_power_meter nfsd auth_rpcgss nfs_acl lockd
 grace sunrpc ip_tables xfs sd_mod crc_t10dif crct10dif_generic mgag200 syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper tt
m bnx2x drm ahci mdio libahci crct10dif_pclmul crct10dif_common ptp libata i2c_core crc32c_intel megaraid_sas pps_core libcrc32c dm_mirr
or dm_region_hash dm_log dm_mod
[  428.614136] CPU: 29 PID: 3149 Comm: a.out Not tainted 3.10.0-327.el7.x86_64 #1
[  428.622201] Hardware name: Dell Inc. PowerEdge M630/0JXJPT, BIOS 1.2.5 05/04/2015
[  428.630557] task: ffff8804654aae00 ti: ffff880036720000 task.ti: ffff880036720000
[  428.638912] RIP: 0010:[<ffffffff813b04cb>]  [<ffffffff813b04cb>] tty_ioctl+0x89b/0xbc0
[  428.647762] RSP: 0018:ffff880036723e18  EFLAGS: 00010246
[  428.653690] RAX: 0000000000000000 RBX: 0000000000005424 RCX: 00007ffcafb2209c
[  428.661658] RDX: ffffffff818a4231 RSI: ffff8808663762a0 RDI: ffff88046804dc00
[  428.669625] RBP: ffff880036723eb0 R08: 0000000000000000 R09: 0000000000000000
[  428.677593] R10: 00007ffcafb21e00 R11: 0000000000000000 R12: ffff88046804dc00
[  428.685560] R13: ffff880461449700 R14: 00007ffcafb2209c R15: ffff88046804ec00
[  428.693529] FS:  00007fabcdefa740(0000) GS:ffff88086edc0000(0000) knlGS:0000000000000000
[  428.702563] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  428.708978] CR2: 0000000000000010 CR3: 0000000035b10000 CR4: 00000000001407e0
[  428.716945] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  428.724912] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  428.732880] Stack:
[  428.735122]  ffffffff812881d6 0000000000000010 0000000000000246 ffff880036723e40
[  428.743420]  0000000000000018 00007ffcafb21e00 0000000000000000 ffff880036723ec8
[  428.751718]  ffff880036723ec8 0007ffff00000001 fffdffef00000000 0000000000000001
[  428.760018] Call Trace:
[  428.762749]  [<ffffffff812881d6>] ? avc_has_perm_flags+0x96/0x1a0
[  428.769557]  [<ffffffff811f1ef5>] do_vfs_ioctl+0x2e5/0x4c0
[  428.775685]  [<ffffffff8128bc6e>] ? file_has_perm+0xae/0xc0
[  428.781907]  [<ffffffff811f2171>] SyS_ioctl+0xa1/0xc0
[  428.787550]  [<ffffffff81645909>] system_call_fastpath+0x16/0x1b
[  428.794256] Code: ff ff ff 84 d2 0f 84 05 fc ff ff 31 f6 4c 89 e7 e8 fb d3 ff ff 31 c0 e9 f4 fb ff ff 0f 1f 40 00 49 8b 44 24 50 4c 8
9 f1 48 8b 00 <8b> 40 10 e8 4d 12 f5 ff 48 98 e9 d6 fb ff ff 66 0f 1f 44 00 00 
[  428.815957] RIP  [<ffffffff813b04cb>] tty_ioctl+0x89b/0xbc0
[  428.822188]  RSP <ffff880036723e18>

To confirm the kernel really crashed while dereferncing tty->ldisc:

0xffffffff813b04bc <tty_ioctl+0x88c>:   nopl   0x0(%rax)
0xffffffff813b04c0 <tty_ioctl+0x890>:   mov    0x50(%r12),%rax
0xffffffff813b04c5 <tty_ioctl+0x895>:   mov    %r14,%rcx
0xffffffff813b04c8 <tty_ioctl+0x898>:   mov    (%rax),%rax
0xffffffff813b04cb <tty_ioctl+0x89b>:   mov    0x10(%rax),%eax

tty_ldisc is at offset 0x50 in tty_struct and ops is at offset 0x0 in
tty_ldisc.

-- 
Mateusz Guzik

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

[Greg Kroah-Hartman]

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

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