| From ebec3f8f5271139df618ebdf8427e24ba102ba94 Mon Sep 17 00:00:00 2001 |
| From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
| Date: Sat, 26 May 2018 09:53:14 +0900 |
| Subject: n_tty: Access echo_* variables carefully. |
| |
| From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
| |
| commit ebec3f8f5271139df618ebdf8427e24ba102ba94 upstream. |
| |
| syzbot is reporting stalls at __process_echoes() [1]. This is because |
| since ldata->echo_commit < ldata->echo_tail becomes true for some reason, |
| the discard loop is serving as almost infinite loop. This patch tries to |
| avoid falling into ldata->echo_commit < ldata->echo_tail situation by |
| making access to echo_* variables more carefully. |
| |
| Since reset_buffer_flags() is called without output_lock held, it should |
| not touch echo_* variables. And omit a call to reset_buffer_flags() from |
| n_tty_open() by using vzalloc(). |
| |
| Since add_echo_byte() is called without output_lock held, it needs memory |
| barrier between storing into echo_buf[] and incrementing echo_head counter. |
| echo_buf() needs corresponding memory barrier before reading echo_buf[]. |
| Lack of handling the possibility of not-yet-stored multi-byte operation |
| might be the reason of falling into ldata->echo_commit < ldata->echo_tail |
| situation, for if I do WARN_ON(ldata->echo_commit == tail + 1) prior to |
| echo_buf(ldata, tail + 1), the WARN_ON() fires. |
| |
| Also, explicitly masking with buffer for the former "while" loop, and |
| use ldata->echo_commit > tail for the latter "while" loop. |
| |
| [1] https://syzkaller.appspot.com/bug?id=17f23b094cd80df750e5b0f8982c521ee6bcbf40 |
| |
| Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
| Reported-by: syzbot <syzbot+108696293d7a21ab688f@syzkaller.appspotmail.com> |
| Cc: Peter Hurley <peter@hurleysoftware.com> |
| Cc: stable <stable@vger.kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/tty/n_tty.c | 42 ++++++++++++++++++++++++------------------ |
| 1 file changed, 24 insertions(+), 18 deletions(-) |
| |
| --- a/drivers/tty/n_tty.c |
| +++ b/drivers/tty/n_tty.c |
| @@ -147,6 +147,7 @@ static inline unsigned char *read_buf_ad |
| |
| static inline unsigned char echo_buf(struct n_tty_data *ldata, size_t i) |
| { |
| + smp_rmb(); /* Matches smp_wmb() in add_echo_byte(). */ |
| return ldata->echo_buf[i & (N_TTY_BUF_SIZE - 1)]; |
| } |
| |
| @@ -324,9 +325,7 @@ static inline void put_tty_queue(unsigne |
| static void reset_buffer_flags(struct n_tty_data *ldata) |
| { |
| ldata->read_head = ldata->canon_head = ldata->read_tail = 0; |
| - ldata->echo_head = ldata->echo_tail = ldata->echo_commit = 0; |
| ldata->commit_head = 0; |
| - ldata->echo_mark = 0; |
| ldata->line_start = 0; |
| |
| ldata->erasing = 0; |
| @@ -647,13 +646,20 @@ static size_t __process_echoes(struct tt |
| old_space = space = tty_write_room(tty); |
| |
| tail = ldata->echo_tail; |
| - while (ldata->echo_commit != tail) { |
| + while (MASK(ldata->echo_commit) != MASK(tail)) { |
| c = echo_buf(ldata, tail); |
| if (c == ECHO_OP_START) { |
| unsigned char op; |
| int no_space_left = 0; |
| |
| /* |
| + * Since add_echo_byte() is called without holding |
| + * output_lock, we might see only portion of multi-byte |
| + * operation. |
| + */ |
| + if (MASK(ldata->echo_commit) == MASK(tail + 1)) |
| + goto not_yet_stored; |
| + /* |
| * If the buffer byte is the start of a multi-byte |
| * operation, get the next byte, which is either the |
| * op code or a control character value. |
| @@ -664,6 +670,8 @@ static size_t __process_echoes(struct tt |
| unsigned int num_chars, num_bs; |
| |
| case ECHO_OP_ERASE_TAB: |
| + if (MASK(ldata->echo_commit) == MASK(tail + 2)) |
| + goto not_yet_stored; |
| num_chars = echo_buf(ldata, tail + 2); |
| |
| /* |
| @@ -758,7 +766,8 @@ static size_t __process_echoes(struct tt |
| /* If the echo buffer is nearly full (so that the possibility exists |
| * of echo overrun before the next commit), then discard enough |
| * data at the tail to prevent a subsequent overrun */ |
| - while (ldata->echo_commit - tail >= ECHO_DISCARD_WATERMARK) { |
| + while (ldata->echo_commit > tail && |
| + ldata->echo_commit - tail >= ECHO_DISCARD_WATERMARK) { |
| if (echo_buf(ldata, tail) == ECHO_OP_START) { |
| if (echo_buf(ldata, tail + 1) == ECHO_OP_ERASE_TAB) |
| tail += 3; |
| @@ -768,6 +777,7 @@ static size_t __process_echoes(struct tt |
| tail++; |
| } |
| |
| + not_yet_stored: |
| ldata->echo_tail = tail; |
| return old_space - space; |
| } |
| @@ -778,6 +788,7 @@ static void commit_echoes(struct tty_str |
| size_t nr, old, echoed; |
| size_t head; |
| |
| + mutex_lock(&ldata->output_lock); |
| head = ldata->echo_head; |
| ldata->echo_mark = head; |
| old = ldata->echo_commit - ldata->echo_tail; |
| @@ -786,10 +797,12 @@ static void commit_echoes(struct tty_str |
| * is over the threshold (and try again each time another |
| * block is accumulated) */ |
| nr = head - ldata->echo_tail; |
| - if (nr < ECHO_COMMIT_WATERMARK || (nr % ECHO_BLOCK > old % ECHO_BLOCK)) |
| + if (nr < ECHO_COMMIT_WATERMARK || |
| + (nr % ECHO_BLOCK > old % ECHO_BLOCK)) { |
| + mutex_unlock(&ldata->output_lock); |
| return; |
| + } |
| |
| - mutex_lock(&ldata->output_lock); |
| ldata->echo_commit = head; |
| echoed = __process_echoes(tty); |
| mutex_unlock(&ldata->output_lock); |
| @@ -840,7 +853,9 @@ static void flush_echoes(struct tty_stru |
| |
| static inline void add_echo_byte(unsigned char c, struct n_tty_data *ldata) |
| { |
| - *echo_buf_addr(ldata, ldata->echo_head++) = c; |
| + *echo_buf_addr(ldata, ldata->echo_head) = c; |
| + smp_wmb(); /* Matches smp_rmb() in echo_buf(). */ |
| + ldata->echo_head++; |
| } |
| |
| /** |
| @@ -1920,31 +1935,22 @@ static int n_tty_open(struct tty_struct |
| struct n_tty_data *ldata; |
| |
| /* Currently a malloc failure here can panic */ |
| - ldata = vmalloc(sizeof(*ldata)); |
| + ldata = vzalloc(sizeof(*ldata)); |
| if (!ldata) |
| - goto err; |
| + return -ENOMEM; |
| |
| ldata->overrun_time = jiffies; |
| mutex_init(&ldata->atomic_read_lock); |
| mutex_init(&ldata->output_lock); |
| |
| tty->disc_data = ldata; |
| - reset_buffer_flags(tty->disc_data); |
| - ldata->column = 0; |
| - ldata->canon_column = 0; |
| ldata->minimum_to_wake = 1; |
| - ldata->num_overrun = 0; |
| - ldata->no_room = 0; |
| - ldata->lnext = 0; |
| tty->closing = 0; |
| /* indicate buffer work may resume */ |
| clear_bit(TTY_LDISC_HALTED, &tty->flags); |
| n_tty_set_termios(tty, NULL); |
| tty_unthrottle(tty); |
| - |
| return 0; |
| -err: |
| - return -ENOMEM; |
| } |
| |
| static inline int input_available_p(struct tty_struct *tty, int poll) |