Re: [PATCH 1/2] n_tty: Fix stall at n_tty_receive_char_special().
On Mon, Jun 04, 2018 at 07:49:40PM +0900, Tetsuo Handa wrote: > Greg, Jiri, will you pick up these patches? Sorry, I missed these last week. They are still in my queue but have to wait until after 4.18-rc1 is out. They are not lost, don't worry... greg k-h
Re: [PATCH 1/2] n_tty: Fix stall at n_tty_receive_char_special().
Greg, Jiri, will you pick up these patches? On 2018/05/26 1:12, Greg KH wrote: > On Thu, May 17, 2018 at 07:11:01AM -0700, syzbot wrote: >> Hello, >> >> syzbot has tested the proposed patch and the reproducer did not trigger >> crash: > > Great! Is the patch going to be submitted "properly" so that I can > queue it up? :) > > thanks, > > greg k-h > On 2018/05/26 9:53, Tetsuo Handa wrote: > syzbot is reporting stalls at n_tty_receive_char_special() [1]. This is > because comparison is not working as expected since ldata->read_head can > change at any moment. Mitigate this by explicitly masking with buffer size > when checking condition for "while" loops. > > [1] > https://syzkaller.appspot.com/bug?id=3d7481a346958d9469bebbeb0537d5f056bdd6e8 > > Signed-off-by: Tetsuo Handa > Reported-by: syzbot > Fixes: bc5a5e3f45d04784 ("n_tty: Don't wrap input buffer indices at buffer > size") > Cc: Peter Hurley > --- > drivers/tty/n_tty.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > On 2018/05/26 9:53, Tetsuo Handa wrote: > 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 > Reported-by: syzbot > Cc: Peter Hurley > --- > drivers/tty/n_tty.c | 42 -- > 1 file changed, 24 insertions(+), 18 deletions(-)
[PATCH 1/2] n_tty: Fix stall at n_tty_receive_char_special().
syzbot is reporting stalls at n_tty_receive_char_special() [1]. This is because comparison is not working as expected since ldata->read_head can change at any moment. Mitigate this by explicitly masking with buffer size when checking condition for "while" loops. [1] https://syzkaller.appspot.com/bug?id=3d7481a346958d9469bebbeb0537d5f056bdd6e8 Signed-off-by: Tetsuo Handa Reported-by: syzbot Fixes: bc5a5e3f45d04784 ("n_tty: Don't wrap input buffer indices at buffer size") Cc: Peter Hurley --- drivers/tty/n_tty.c | 13 - 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index cbe98bc..b279f873 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -124,6 +124,8 @@ struct n_tty_data { struct mutex output_lock; }; +#define MASK(x) ((x) & (N_TTY_BUF_SIZE - 1)) + static inline size_t read_cnt(struct n_tty_data *ldata) { return ldata->read_head - ldata->read_tail; @@ -978,14 +980,15 @@ static void eraser(unsigned char c, struct tty_struct *tty) } seen_alnums = 0; - while (ldata->read_head != ldata->canon_head) { + while (MASK(ldata->read_head) != MASK(ldata->canon_head)) { head = ldata->read_head; /* erase a single possibly multibyte character */ do { head--; c = read_buf(ldata, head); - } while (is_continuation(c, tty) && head != ldata->canon_head); + } while (is_continuation(c, tty) && +MASK(head) != MASK(ldata->canon_head)); /* do not partially erase */ if (is_continuation(c, tty)) @@ -1027,7 +1030,7 @@ static void eraser(unsigned char c, struct tty_struct *tty) * This info is used to go back the correct * number of columns. */ - while (tail != ldata->canon_head) { + while (MASK(tail) != MASK(ldata->canon_head)) { tail--; c = read_buf(ldata, tail); if (c == '\t') { @@ -1302,7 +1305,7 @@ static void n_tty_receive_parity_error(struct tty_struct *tty, unsigned char c) finish_erasing(ldata); echo_char(c, tty); echo_char_raw('\n', ldata); - while (tail != ldata->read_head) { + while (MASK(tail) != MASK(ldata->read_head)) { echo_char(read_buf(ldata, tail), tty); tail++; } @@ -2411,7 +2414,7 @@ static unsigned long inq_canon(struct n_tty_data *ldata) tail = ldata->read_tail; nr = head - tail; /* Skip EOF-chars.. */ - while (head != tail) { + while (MASK(head) != MASK(tail)) { if (test_bit(tail & (N_TTY_BUF_SIZE - 1), ldata->read_flags) && read_buf(ldata, tail) == __DISABLED_CHAR) nr--; -- 1.8.3.1