Re: [PATCH 1/2] n_tty: Fix stall at n_tty_receive_char_special().

[Tetsuo Handa]

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 <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Reported-by: syzbot <syzbot+18df353d7540aa6b5467@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Fixes: bc5a5e3f45d04784 ("n_tty: Don't wrap input buffer indices at buffer size")
> Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> ---
>  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 <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Reported-by: syzbot <syzbot+108696293d7a21ab688f@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/tty/n_tty.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)