diff mbox series

[SRU,Trusty,1/1] n_tty: fix EXTPROC vs ICANON interaction with TIOCINQ (aka FIONREAD)

Message ID 20181109170426.6350-2-kleber.souza@canonical.com
State New
Headers show
Series Fix for CVE-2018-18386 | expand

Commit Message

Kleber Sacilotto de Souza Nov. 9, 2018, 5:04 p.m. UTC 
From: Linus Torvalds <torvalds@linux-foundation.org>

We added support for EXTPROC back in 2010 in commit 26df6d13406d ("tty:
Add EXTPROC support for LINEMODE") and the intent was to allow it to
override some (all?) ICANON behavior.  Quoting from that original commit
message:

         There is a new bit in the termios local flag word, EXTPROC.
         When this bit is set, several aspects of the terminal driver
         are disabled.  Input line editing, character echo, and mapping
         of signals are all disabled.  This allows the telnetd to turn
         off these functions when in linemode, but still keep track of
         what state the user wants the terminal to be in.

but the problem turns out that "several aspects of the terminal driver
are disabled" is a bit ambiguous, and you can really confuse the n_tty
layer by setting EXTPROC and then causing some of the ICANON invariants
to no longer be maintained.

This fixes at least one such case (TIOCINQ) becoming unhappy because of
the confusion over whether ICANON really means ICANON when EXTPROC is set.

This basically makes TIOCINQ match the case of read: if EXTPROC is set,
we ignore ICANON.  Also, make sure to reset the ICANON state ie EXTPROC
changes, not just if ICANON changes.

Fixes: 26df6d13406d ("tty: Add EXTPROC support for LINEMODE")
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-by: syzkaller <syzkaller@googlegroups.com>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

CVE-2018-18386
(cherry picked from commit 966031f340185eddd05affcf72b740549f056348)
Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 drivers/tty/n_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Colin Ian King Nov. 9, 2018, 5:07 p.m. UTC | #1 
On 09/11/2018 17:04, Kleber Sacilotto de Souza wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> We added support for EXTPROC back in 2010 in commit 26df6d13406d ("tty:
> Add EXTPROC support for LINEMODE") and the intent was to allow it to
> override some (all?) ICANON behavior.  Quoting from that original commit
> message:
> 
>          There is a new bit in the termios local flag word, EXTPROC.
>          When this bit is set, several aspects of the terminal driver
>          are disabled.  Input line editing, character echo, and mapping
>          of signals are all disabled.  This allows the telnetd to turn
>          off these functions when in linemode, but still keep track of
>          what state the user wants the terminal to be in.
> 
> but the problem turns out that "several aspects of the terminal driver
> are disabled" is a bit ambiguous, and you can really confuse the n_tty
> layer by setting EXTPROC and then causing some of the ICANON invariants
> to no longer be maintained.
> 
> This fixes at least one such case (TIOCINQ) becoming unhappy because of
> the confusion over whether ICANON really means ICANON when EXTPROC is set.
> 
> This basically makes TIOCINQ match the case of read: if EXTPROC is set,
> we ignore ICANON.  Also, make sure to reset the ICANON state ie EXTPROC
> changes, not just if ICANON changes.
> 
> Fixes: 26df6d13406d ("tty: Add EXTPROC support for LINEMODE")
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Cc: Jiri Slaby <jslaby@suse.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> CVE-2018-18386
> (cherry picked from commit 966031f340185eddd05affcf72b740549f056348)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
>  drivers/tty/n_tty.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 8a13b3372804..38bf1d5230d0 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1811,7 +1811,7 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
>  {
>  	struct n_tty_data *ldata = tty->disc_data;
>  
> -	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
> +	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & (ICANON | EXTPROC)) {
>  		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
>  		ldata->line_start = ldata->read_tail;
>  		if (!L_ICANON(tty) || !read_cnt(ldata)) {
> @@ -2526,7 +2526,7 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
>  		return put_user(tty_chars_in_buffer(tty), (int __user *) arg);
>  	case TIOCINQ:
>  		down_write(&tty->termios_rwsem);
> -		if (L_ICANON(tty))
> +		if (L_ICANON(tty) && !L_EXTPROC(tty))
>  			retval = inq_canon(ldata);
>  		else
>  			retval = read_cnt(ldata);
> 

Clean cherry pick. Been upstream for a while, so..

Acked-by: Colin Ian King <colin.king@canonical.com>
Tyler Hicks Nov. 9, 2018, 11:48 p.m. UTC | #2 
On 2018-11-09 18:04:26, Kleber Sacilotto de Souza wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> We added support for EXTPROC back in 2010 in commit 26df6d13406d ("tty:
> Add EXTPROC support for LINEMODE") and the intent was to allow it to
> override some (all?) ICANON behavior.  Quoting from that original commit
> message:
> 
>          There is a new bit in the termios local flag word, EXTPROC.
>          When this bit is set, several aspects of the terminal driver
>          are disabled.  Input line editing, character echo, and mapping
>          of signals are all disabled.  This allows the telnetd to turn
>          off these functions when in linemode, but still keep track of
>          what state the user wants the terminal to be in.
> 
> but the problem turns out that "several aspects of the terminal driver
> are disabled" is a bit ambiguous, and you can really confuse the n_tty
> layer by setting EXTPROC and then causing some of the ICANON invariants
> to no longer be maintained.
> 
> This fixes at least one such case (TIOCINQ) becoming unhappy because of
> the confusion over whether ICANON really means ICANON when EXTPROC is set.
> 
> This basically makes TIOCINQ match the case of read: if EXTPROC is set,
> we ignore ICANON.  Also, make sure to reset the ICANON state ie EXTPROC
> changes, not just if ICANON changes.
> 
> Fixes: 26df6d13406d ("tty: Add EXTPROC support for LINEMODE")
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Cc: Jiri Slaby <jslaby@suse.com>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> CVE-2018-18386
> (cherry picked from commit 966031f340185eddd05affcf72b740549f056348)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---

Acked-by: Tyler Hicks <tyhicks@canonical.com>

Tyler
diff mbox series

Patch

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 8a13b3372804..38bf1d5230d0 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -1811,7 +1811,7 @@  static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old)
 {
 	struct n_tty_data *ldata = tty->disc_data;
 
-	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) {
+	if (!old || (old->c_lflag ^ tty->termios.c_lflag) & (ICANON | EXTPROC)) {
 		bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE);
 		ldata->line_start = ldata->read_tail;
 		if (!L_ICANON(tty) || !read_cnt(ldata)) {
@@ -2526,7 +2526,7 @@  static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
 		return put_user(tty_chars_in_buffer(tty), (int __user *) arg);
 	case TIOCINQ:
 		down_write(&tty->termios_rwsem);
-		if (L_ICANON(tty))
+		if (L_ICANON(tty) && !L_EXTPROC(tty))
 			retval = inq_canon(ldata);
 		else
 			retval = read_cnt(ldata);