lkml.org 
[lkml]   [2018]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
On Wed, Apr 25, 2018 at 03:41:59PM +0200, Greg KH wrote:
> On Wed, Apr 25, 2018 at 10:20:50PM +0900, DaeRyong Jeong wrote:
> > tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> > by th->used and updates tb->used.
> > But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> > tb->used can be updated improperly.
> > It leads slab-out-of-bound write in tty_insert_flip_string_fixed_flag or
> > slab-out-of-bounds read in flush_to_ldisc
> >
> > BUG: KASAN: slab-out-of-bounds in tty_insert_flip_string_fixed_flag+0xb5/
> > 0x130 drivers/tty/tty_buffer.c:316 at addr ffff880114fcc121
> > Write of size 1792 by task syz-executor0/30017
> > CPU: 1 PID: 30017 Comm: syz-executor0 Not tainted 4.8.0 #1
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
> > 0000000000000000 ffff88011638f888 ffffffff81694cc3 ffff88007d802140
> > ffff880114fcb300 ffff880114fcc300 ffff880114fcb300 ffff88011638f8b0
> > ffffffff8130075c ffff88011638f940 ffff88007d802140 ffff880194fcc121
> > Call Trace:
> > __dump_stack lib/dump_stack.c:15 [inline]
> > dump_stack+0xb3/0x110 lib/dump_stack.c:51
> > kasan_object_err+0x1c/0x70 mm/kasan/report.c:156
> > print_address_description mm/kasan/report.c:194 [inline]
> > kasan_report_error+0x1f7/0x4e0 mm/kasan/report.c:283
> > kasan_report+0x36/0x40 mm/kasan/report.c:303
> > check_memory_region_inline mm/kasan/kasan.c:292 [inline]
> > check_memory_region+0x13e/0x1a0 mm/kasan/kasan.c:299
> > memcpy+0x37/0x50 mm/kasan/kasan.c:335
> > tty_insert_flip_string_fixed_flag+0xb5/0x130 drivers/tty/tty_buffer.c:316
> > tty_insert_flip_string include/linux/tty_flip.h:35 [inline]
> > pty_write+0x7f/0xc0 drivers/tty/pty.c:115
> > n_hdlc_send_frames+0x1d4/0x3b0 drivers/tty/n_hdlc.c:419
> > n_hdlc_tty_wakeup+0x73/0xa0 drivers/tty/n_hdlc.c:496
> > tty_wakeup+0x92/0xb0 drivers/tty/tty_io.c:601
> > __start_tty.part.26+0x66/0x70 drivers/tty/tty_io.c:1018
> > __start_tty+0x34/0x40 drivers/tty/tty_io.c:1013
> > n_tty_ioctl_helper+0x146/0x1e0 drivers/tty/tty_ioctl.c:1138
> > n_hdlc_tty_ioctl+0xb3/0x2b0 drivers/tty/n_hdlc.c:794
> > tty_ioctl+0xa85/0x16d0 drivers/tty/tty_io.c:2992
> > vfs_ioctl fs/ioctl.c:43 [inline]
> > do_vfs_ioctl+0x13e/0xba0 fs/ioctl.c:679
> > SYSC_ioctl fs/ioctl.c:694 [inline]
> > SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
> > entry_SYSCALL_64_fastpath+0x1f/0xbd
> >
> > Call sequences are as follows.
> > CPU0 CPU1
> > n_tty_ioctl_helper n_tty_ioctl_helper
> > __start_tty tty_send_xchar
> > tty_wakeup pty_write
> > n_hdlc_tty_wakeup tty_insert_flip_string
> > n_hdlc_send_frames tty_insert_flip_string_fixed_flag
> > pty_write
> > tty_insert_flip_string
> > tty_insert_flip_string_fixed_flag
> >
> > Acquire tty->atomic_write_lock by calling tty_write_lock() before
> > __start_tty() since __start_tty() can sends frames.
> >
> > Signed-off-by: DaeRyong Jeong <threeearcat@gmail.com>
> > ---
> > drivers/tty/tty_io.c | 16 +++++++++-------
> > drivers/tty/tty_ioctl.c | 5 +++++
> > include/linux/tty.h | 2 ++
> > 3 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 63114ea35ec1..41f83bd4cc40 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -873,13 +873,15 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
> > return i;
> > }
> >
> > -static void tty_write_unlock(struct tty_struct *tty)
> > +void tty_write_unlock(struct tty_struct *tty, int wakeup)
> > {
> > mutex_unlock(&tty->atomic_write_lock);
> > - wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > + if (wakeup) {
> > + wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > + }
>
> Always run scripts/checkpatch.pl before sending patches out so you do
> not get grumpy maintainers telling you to run scripts/checkpatch.pl :)

Yes, I will. I will never corrupt a patch again. I'm sorry..

>
> Anyway, these "bool" type options are horrid for trying to remember what
> is happening here. You have to go look up what 1 or 0 is, right?
>
> How about two functions:
> tty_write_unlock()
> tty_write_unlock_wakup()
>
> and the second one calls the first and then does the wakeup?

Yes. I agree that this has more readability and is much better to write a
code.

>
> But are you sure this is the correct fix? What is protecting the race
> from happening with this change? How do you know to call wakeup or not?
> What determines which is better to do?

As Alan cox commented, pty layer is broken, not this code. I thought
wrong way to fix the issue. I will look into tty/pty code and think
deeply. And then I will send a new patch.

Thank you a lot for your comment!

>
> thanks,
>
> greg k-h

\
 
 \ /
  Last update: 2018-04-25 19:43    [W:0.051 / U:2.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site