[PATCH 06/19] tty: Fix unsafe ldisc reference via ioctl(TIOCGETD)

[PATCH 06/19] tty: Fix unsafe ldisc reference via ioctl(TIOCGETD)

[Peter Hurley]

ioctl(TIOCGETD) retrieves the line discipline id directly from the
ldisc because the line discipline id (c_line) in termios is untrustworthy;
userspace may have set termios via ioctl(TCSETS*) without actually
changing the line discipline via ioctl(TIOCSETD).

However, directly accessing the current ldisc via tty->ldisc is
unsafe; the ldisc ptr dereferenced may be stale if the line discipline
is changing via ioctl(TIOCSETD) or hangup.

Wait for the line discipline reference (just like read() or write())
to retrieve the "current" line discipline id.

Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 892c923..56d3a6b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2649,6 +2649,28 @@ static int tiocsetd(struct tty_struct *tty, int __user *p)
 }
 
 /**
+ *	tiocgetd	-	get line discipline
+ *	@tty: tty device
+ *	@p: pointer to user data
+ *
+ *	Retrieves the line discipline id directly from the ldisc.
+ *
+ *	Locking: waits for ldisc reference (in case the line discipline
+ *		is changing or the tty is being hungup)
+ */
+
+static int tiocgetd(struct tty_struct *tty, int __user *p)
+{
+	struct tty_ldisc *ld;
+	int ret;
+
+	ld = tty_ldisc_ref_wait(tty);
+	ret = put_user(ld->ops->num, p);
+	tty_ldisc_deref(ld);
+	return ret;
+}
+
+/**
  *	send_break	-	performed time break
  *	@tty: device to break on
  *	@duration: timeout in mS
@@ -2874,7 +2896,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:
-- 
2.6.3

[PATCH 07/19] n_tty: Fix unsafe reference to "other" ldisc

[Peter Hurley]

Although n_tty_check_unthrottle() has a valid ldisc reference (since
the tty core gets the ldisc ref in tty_read() before calling the line
discipline read() method), it does not have a valid ldisc reference to
the "other" pty of a pty pair. Since getting an ldisc reference for
tty->link essentially open-codes tty_wakeup(), just replace with the
equivalent tty_wakeup().

Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 2dddc72..703b219 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -270,16 +270,13 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 
 static void n_tty_check_unthrottle(struct tty_struct *tty)
 {
-	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-	    tty->link->ldisc->ops->write_wakeup == n_tty_write_wakeup) {
+	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			return;
 		if (!tty->count)
 			return;
 		n_tty_kick_worker(tty);
-		n_tty_write_wakeup(tty->link);
-		if (waitqueue_active(&tty->link->write_wait))
-			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
+		tty_wakeup(tty->link);
 		return;
 	}
 
-- 
2.6.3

[PATCH 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Peter Hurley]

As the function documentation for tty_ldisc_ref_wait() notes, it is
only callable from a tty file_operations routine; otherwise there
is no guarantee the ref won't be NULL.

The key difference with the VT's paste_selection() is that is an ioctl,
where __speakup_paste_selection() is completely asynch kworker, kicked
off from interrupt context.

Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
       tty (ab)usage to match vt")
Cc: <stable@vger.kernel.org>

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/staging/speakup/selection.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index aa5ab6c..86c0b9a 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
 	struct tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	ld = tty_ldisc_ref_wait(tty);
+	ld = tty_ldisc_ref(tty);
+	if (!ld)
+		return;
 	tty_buffer_lock_exclusive(&vc->port);
 
 	add_wait_queue(&vc->paste_wait, &wait);
-- 
2.6.3

[PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Peter Hurley]

As the function documentation for tty_ldisc_ref_wait() notes, it is
only callable from a tty file_operations routine; otherwise there
is no guarantee the ref won't be NULL.

The key difference with the VT's paste_selection() is that is an ioctl,
where __speakup_paste_selection() is completely asynch kworker, kicked
off from interrupt context.

Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
       tty (ab)usage to match vt")
Cc: <stable@vger.kernel.org>

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/staging/speakup/selection.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index aa5ab6c..86c0b9a 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
 	struct tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	ld = tty_ldisc_ref_wait(tty);
+	ld = tty_ldisc_ref(tty);
+	if (!ld)
+		return;
 	tty_buffer_lock_exclusive(&vc->port);
 
 	add_wait_queue(&vc->paste_wait, &wait);
-- 
2.7.0

Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
> As the function documentation for tty_ldisc_ref_wait() notes, it is
> only callable from a tty file_operations routine; otherwise there
> is no guarantee the ref won't be NULL.
> 
> The key difference with the VT's paste_selection() is that is an ioctl,
> where __speakup_paste_selection() is completely asynch kworker, kicked
> off from interrupt context.
> 
> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>        tty (ab)usage to match vt")
> Cc: <stable@vger.kernel.org>
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> ---
>  drivers/staging/speakup/selection.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
> index aa5ab6c..86c0b9a 100644
> --- a/drivers/staging/speakup/selection.c
> +++ b/drivers/staging/speakup/selection.c
> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>  	struct tty_ldisc *ld;
>  	DECLARE_WAITQUEUE(wait, current);
>  
> -	ld = tty_ldisc_ref_wait(tty);
> +	ld = tty_ldisc_ref(tty);
> +	if (!ld)
> +		return;
>  	tty_buffer_lock_exclusive(&vc->port);
>  
>  	add_wait_queue(&vc->paste_wait, &wait);

This leaks a reference to the tty.  Instead of returning directly, I
think you need to add a label and goto the tty_kref_put() at the bottom
of the function.

Ben.

-- 
Ben Hutchings
Power corrupts.  Absolute power is kind of neat.
                           - John Lehman, Secretary of the US Navy 1981-1987

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Peter Hurley]

On 01/10/2016 03:16 PM, Ben Hutchings wrote:
> On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
>> As the function documentation for tty_ldisc_ref_wait() notes, it is
>> only callable from a tty file_operations routine; otherwise there
>> is no guarantee the ref won't be NULL.
>>
>> The key difference with the VT's paste_selection() is that is an ioctl,
>> where __speakup_paste_selection() is completely asynch kworker, kicked
>> off from interrupt context.
>>
>> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>>        tty (ab)usage to match vt")
>> Cc: <stable@vger.kernel.org>
>>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> ---
>>  drivers/staging/speakup/selection.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
>> index aa5ab6c..86c0b9a 100644
>> --- a/drivers/staging/speakup/selection.c
>> +++ b/drivers/staging/speakup/selection.c
>> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>>  	struct tty_ldisc *ld;
>>  	DECLARE_WAITQUEUE(wait, current);
>>  
>> -	ld = tty_ldisc_ref_wait(tty);
>> +	ld = tty_ldisc_ref(tty);
>> +	if (!ld)
>> +		return;
>>  	tty_buffer_lock_exclusive(&vc->port);
>>  
>>  	add_wait_queue(&vc->paste_wait, &wait);
> 
> This leaks a reference to the tty.  Instead of returning directly, I
> think you need to add a label and goto the tty_kref_put() at the bottom
> of the function.

Ugh, speakup_paste_selection() is a worse hack than I thought it was.

Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Peter Hurley]

On 01/10/2016 04:25 PM, Peter Hurley wrote:
> On 01/10/2016 03:16 PM, Ben Hutchings wrote:
>> On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
>>> As the function documentation for tty_ldisc_ref_wait() notes, it is
>>> only callable from a tty file_operations routine; otherwise there
>>> is no guarantee the ref won't be NULL.
>>>
>>> The key difference with the VT's paste_selection() is that is an ioctl,
>>> where __speakup_paste_selection() is completely asynch kworker, kicked
>>> off from interrupt context.
>>>
>>> Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
>>>        tty (ab)usage to match vt")
>>> Cc: <stable@vger.kernel.org>
>>>
>>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>>> ---
>>>  drivers/staging/speakup/selection.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
>>> index aa5ab6c..86c0b9a 100644
>>> --- a/drivers/staging/speakup/selection.c
>>> +++ b/drivers/staging/speakup/selection.c
>>> @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
>>>  	struct tty_ldisc *ld;
>>>  	DECLARE_WAITQUEUE(wait, current);
>>>  
>>> -	ld = tty_ldisc_ref_wait(tty);
>>> +	ld = tty_ldisc_ref(tty);
>>> +	if (!ld)
>>> +		return;
>>>  	tty_buffer_lock_exclusive(&vc->port);
>>>  
>>>  	add_wait_queue(&vc->paste_wait, &wait);
>>
>> This leaks a reference to the tty.  Instead of returning directly, I
>> think you need to add a label and goto the tty_kref_put() at the bottom
>> of the function.
> 
> Ugh, speakup_paste_selection() is a worse hack than I thought it was.

What if the kworker has already been scheduled but not run? Leaky reference
anyway.

What guarantees that the kref is gettable to begin with and isn't incrementing
from 0?

This isn't how tty krefs work.

I'll fix the patch to drop the kref but this is broken anyway.

Regards,
Peter Hurley

[PATCH v3 06/19] tty: Fix unsafe ldisc reference via ioctl(TIOCGETD)

[Peter Hurley]

ioctl(TIOCGETD) retrieves the line discipline id directly from the
ldisc because the line discipline id (c_line) in termios is untrustworthy;
userspace may have set termios via ioctl(TCSETS*) without actually
changing the line discipline via ioctl(TIOCSETD).

However, directly accessing the current ldisc via tty->ldisc is
unsafe; the ldisc ptr dereferenced may be stale if the line discipline
is changing via ioctl(TIOCSETD) or hangup.

Wait for the line discipline reference (just like read() or write())
to retrieve the "current" line discipline id.

Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/tty_io.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 892c923..56d3a6b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2649,6 +2649,28 @@ static int tiocsetd(struct tty_struct *tty, int __user *p)
 }
 
 /**
+ *	tiocgetd	-	get line discipline
+ *	@tty: tty device
+ *	@p: pointer to user data
+ *
+ *	Retrieves the line discipline id directly from the ldisc.
+ *
+ *	Locking: waits for ldisc reference (in case the line discipline
+ *		is changing or the tty is being hungup)
+ */
+
+static int tiocgetd(struct tty_struct *tty, int __user *p)
+{
+	struct tty_ldisc *ld;
+	int ret;
+
+	ld = tty_ldisc_ref_wait(tty);
+	ret = put_user(ld->ops->num, p);
+	tty_ldisc_deref(ld);
+	return ret;
+}
+
+/**
  *	send_break	-	performed time break
  *	@tty: device to break on
  *	@duration: timeout in mS
@@ -2874,7 +2896,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:
-- 
2.7.0

[PATCH v3 07/19] n_tty: Fix unsafe reference to "other" ldisc

[Peter Hurley]

Although n_tty_check_unthrottle() has a valid ldisc reference (since
the tty core gets the ldisc ref in tty_read() before calling the line
discipline read() method), it does not have a valid ldisc reference to
the "other" pty of a pty pair. Since getting an ldisc reference for
tty->link essentially open-codes tty_wakeup(), just replace with the
equivalent tty_wakeup().

Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index e09fd58..90eca26 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -269,16 +269,13 @@ static void n_tty_check_throttle(struct tty_struct *tty)
 
 static void n_tty_check_unthrottle(struct tty_struct *tty)
 {
-	if (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
-	    tty->link->ldisc->ops->write_wakeup == n_tty_write_wakeup) {
+	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
 		if (chars_in_buffer(tty) > TTY_THRESHOLD_UNTHROTTLE)
 			return;
 		if (!tty->count)
 			return;
 		n_tty_kick_worker(tty);
-		n_tty_write_wakeup(tty->link);
-		if (waitqueue_active(&tty->link->write_wait))
-			wake_up_interruptible_poll(&tty->link->write_wait, POLLOUT);
+		tty_wakeup(tty->link);
 		return;
 	}
 
-- 
2.7.0

[PATCH v3 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Peter Hurley]

As the function documentation for tty_ldisc_ref_wait() notes, it is
only callable from a tty file_operations routine; otherwise there
is no guarantee the ref won't be NULL.

The key difference with the VT's paste_selection() is that is an ioctl,
where __speakup_paste_selection() is completely async kworker, kicked
off from interrupt context.

Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
       tty (ab)usage to match vt")
Cc: <stable@vger.kernel.org>

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/staging/speakup/selection.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index aa5ab6c..41ef099 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
 	struct tty_ldisc *ld;
 	DECLARE_WAITQUEUE(wait, current);
 
-	ld = tty_ldisc_ref_wait(tty);
+	ld = tty_ldisc_ref(tty);
+	if (!ld)
+		goto tty_unref;
 	tty_buffer_lock_exclusive(&vc->port);
 
 	add_wait_queue(&vc->paste_wait, &wait);
@@ -162,6 +164,7 @@ static void __speakup_paste_selection(struct work_struct *work)
 
 	tty_buffer_unlock_exclusive(&vc->port);
 	tty_ldisc_deref(ld);
+tty_unref:
 	tty_kref_put(tty);
 }
 
-- 
2.7.0

Re: [PATCH v2 09/19] staging/speakup: Use tty_ldisc_ref() for paste kworker

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2600 bytes --]

On Sun, 2016-01-10 at 21:40 -0800, Peter Hurley wrote:
> On 01/10/2016 04:25 PM, Peter Hurley wrote:
> > On 01/10/2016 03:16 PM, Ben Hutchings wrote:
> > > On Sat, 2016-01-09 at 20:41 -0800, Peter Hurley wrote:
> > > > As the function documentation for tty_ldisc_ref_wait() notes, it is
> > > > only callable from a tty file_operations routine; otherwise there
> > > > is no guarantee the ref won't be NULL.
> > > > 
> > > > The key difference with the VT's paste_selection() is that is an ioctl,
> > > > where __speakup_paste_selection() is completely asynch kworker, kicked
> > > > off from interrupt context.
> > > > 
> > > > Fixes: 28a821c30688 ("Staging: speakup: Update __speakup_paste_selection()
> > > >        tty (ab)usage to match vt")
> > > > Cc: <stable@vger.kernel.org>
> > > > 
> > > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> > > > ---
> > > >  drivers/staging/speakup/selection.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
> > > > index aa5ab6c..86c0b9a 100644
> > > > --- a/drivers/staging/speakup/selection.c
> > > > +++ b/drivers/staging/speakup/selection.c
> > > > @@ -142,7 +142,9 @@ static void __speakup_paste_selection(struct work_struct *work)
> > > >  	struct tty_ldisc *ld;
> > > >  	DECLARE_WAITQUEUE(wait, current);
> > > >  
> > > > -	ld = tty_ldisc_ref_wait(tty);
> > > > +	ld = tty_ldisc_ref(tty);
> > > > +	if (!ld)
> > > > +		return;
> > > >  	tty_buffer_lock_exclusive(&vc->port);
> > > >  
> > > >  	add_wait_queue(&vc->paste_wait, &wait);
> > > 
> > > This leaks a reference to the tty.  Instead of returning directly, I
> > > think you need to add a label and goto the tty_kref_put() at the bottom
> > > of the function.
> > 
> > Ugh, speakup_paste_selection() is a worse hack than I thought it was.
> 
> What if the kworker has already been scheduled but not run? Leaky reference
> anyway.

I don't think so - the cmpxchg() should ensure that only one paste is
outstanding.

> What guarantees that the kref is gettable to begin with and isn't incrementing
> from 0?

Surely it's a bug in the caller of speakup_paste_selection() if it
doesn't hold a reference?

Ben.

> This isn't how tty krefs work.
> 
> I'll fix the patch to drop the kref but this is broken anyway.
> 
> Regards,
> Peter Hurley
> 
-- 
Ben Hutchings
Q.  Which is the greater problem in the world today, ignorance or apathy?
A.  I don't know and I couldn't care less.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]