Messages in this thread | | | Date | Thu, 26 Apr 2018 02:42:46 +0900 | From | DaeRyong Jeong <> | Subject | Re: [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
| |