| [2521] | 1 | From 4291086b1f081b869c6d79e5b7441633dc3ace00 Mon Sep 17 00:00:00 2001 |
|---|
| 2 | From: Peter Hurley <peter@hurleysoftware.com> |
|---|
| 3 | Date: Sat, 3 May 2014 14:04:59 +0200 |
|---|
| 4 | Subject: [PATCH] n_tty: Fix n_tty_write crash when echoing in raw mode |
|---|
| 5 | |
|---|
| 6 | The tty atomic_write_lock does not provide an exclusion guarantee for |
|---|
| 7 | the tty driver if the termios settings are LECHO & !OPOST. And since |
|---|
| 8 | it is unexpected and not allowed to call TTY buffer helpers like |
|---|
| 9 | tty_insert_flip_string concurrently, this may lead to crashes when |
|---|
| 10 | concurrect writers call pty_write. In that case the following two |
|---|
| 11 | writers: |
|---|
| 12 | * the ECHOing from a workqueue and |
|---|
| 13 | * pty_write from the process |
|---|
| 14 | race and can overflow the corresponding TTY buffer like follows. |
|---|
| 15 | |
|---|
| 16 | If we look into tty_insert_flip_string_fixed_flag, there is: |
|---|
| 17 | int space = __tty_buffer_request_room(port, goal, flags); |
|---|
| 18 | struct tty_buffer *tb = port->buf.tail; |
|---|
| 19 | ... |
|---|
| 20 | memcpy(char_buf_ptr(tb, tb->used), chars, space); |
|---|
| 21 | ... |
|---|
| 22 | tb->used += space; |
|---|
| 23 | |
|---|
| 24 | so the race of the two can result in something like this: |
|---|
| 25 | A B |
|---|
| 26 | __tty_buffer_request_room |
|---|
| 27 | __tty_buffer_request_room |
|---|
| 28 | memcpy(buf(tb->used), ...) |
|---|
| 29 | tb->used += space; |
|---|
| 30 | memcpy(buf(tb->used), ...) ->BOOM |
|---|
| 31 | |
|---|
| 32 | B's memcpy is past the tty_buffer due to the previous A's tb->used |
|---|
| 33 | increment. |
|---|
| 34 | |
|---|
| 35 | Since the N_TTY line discipline input processing can output |
|---|
| 36 | concurrently with a tty write, obtain the N_TTY ldisc output_lock to |
|---|
| 37 | serialize echo output with normal tty writes. This ensures the tty |
|---|
| 38 | buffer helper tty_insert_flip_string is not called concurrently and |
|---|
| 39 | everything is fine. |
|---|
| 40 | |
|---|
| 41 | Note that this is nicely reproducible by an ordinary user using |
|---|
| 42 | forkpty and some setup around that (raw termios + ECHO). And it is |
|---|
| 43 | present in kernels at least after commit |
|---|
| 44 | d945cb9cce20ac7143c2de8d88b187f62db99bdc (pty: Rework the pty layer to |
|---|
| 45 | use the normal buffering logic) in 2.6.31-rc3. |
|---|
| 46 | |
|---|
| 47 | js: add more info to the commit log |
|---|
| 48 | js: switch to bool |
|---|
| 49 | js: lock unconditionally |
|---|
| 50 | js: lock only the tty->ops->write call |
|---|
| 51 | |
|---|
| 52 | References: CVE-2014-0196 |
|---|
| 53 | Reported-and-tested-by: Jiri Slaby <jslaby@suse.cz> |
|---|
| 54 | Signed-off-by: Peter Hurley <peter@hurleysoftware.com> |
|---|
| 55 | Signed-off-by: Jiri Slaby <jslaby@suse.cz> |
|---|
| 56 | Cc: Linus Torvalds <torvalds@linux-foundation.org> |
|---|
| 57 | Cc: Alan Cox <alan@lxorguk.ukuu.org.uk> |
|---|
| 58 | Cc: <stable@vger.kernel.org> |
|---|
| 59 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|
| 60 | --- |
|---|
| 61 | drivers/tty/n_tty.c | 4 ++++ |
|---|
| 62 | 1 file changed, 4 insertions(+) |
|---|
| 63 | |
|---|
| 64 | diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c |
|---|
| 65 | index 41fe8a0..fe9d129 100644 |
|---|
| 66 | --- a/drivers/tty/n_tty.c |
|---|
| 67 | +++ b/drivers/tty/n_tty.c |
|---|
| 68 | @@ -2353,8 +2353,12 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file, |
|---|
| 69 | if (tty->ops->flush_chars) |
|---|
| 70 | tty->ops->flush_chars(tty); |
|---|
| 71 | } else { |
|---|
| 72 | + struct n_tty_data *ldata = tty->disc_data; |
|---|
| 73 | + |
|---|
| 74 | while (nr > 0) { |
|---|
| 75 | + mutex_lock(&ldata->output_lock); |
|---|
| 76 | c = tty->ops->write(tty, b, nr); |
|---|
| 77 | + mutex_unlock(&ldata->output_lock); |
|---|
| 78 | if (c < 0) { |
|---|
| 79 | retval = c; |
|---|
| 80 | goto break_out; |
|---|
| 81 | -- |
|---|
| 82 | 1.7.10.4 |
|---|
| 83 | |
|---|
