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 <[email protected]>
---
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);
+ }
}
-static int tty_write_lock(struct tty_struct *tty, int ndelay)
+int tty_write_lock(struct tty_struct *tty, int ndelay)
{
if (!mutex_trylock(&tty->atomic_write_lock)) {
if (ndelay)
@@ -973,7 +975,7 @@ static inline ssize_t do_tty_write(
ret = written;
}
out:
- tty_write_unlock(tty);
+ tty_write_unlock(tty, 1);
return ret;
}
@@ -997,7 +999,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
if (tty->ops->write && tty->count > 0)
tty->ops->write(tty, msg, strlen(msg));
tty_unlock(tty);
- tty_write_unlock(tty);
+ tty_write_unlock(tty, 1);
}
return;
}
@@ -1092,7 +1094,7 @@ int tty_send_xchar(struct tty_struct *tty, char ch)
if (was_stopped)
stop_tty(tty);
up_read(&tty->termios_rwsem);
- tty_write_unlock(tty);
+ tty_write_unlock(tty, 1);
return 0;
}
@@ -2395,7 +2397,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
msleep_interruptible(duration);
retval = tty->ops->break_ctl(tty, 0);
out:
- tty_write_unlock(tty);
+ tty_write_unlock(tty, 1);
if (signal_pending(current))
retval = -EINTR;
}
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index d9b561d89432..a54ab91aec90 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
spin_unlock_irq(&tty->flow_lock);
break;
case TCOON:
+ if (tty_write_lock(tty, 0) < 0)
+ return -ERESTARTSYS;
+
spin_lock_irq(&tty->flow_lock);
if (tty->flow_stopped) {
tty->flow_stopped = 0;
__start_tty(tty);
}
spin_unlock_irq(&tty->flow_lock);
+
+ tty_write_unlock(tty, 0);
break;
case TCIOFF:
if (STOP_CHAR(tty) != __DISABLED_CHAR)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 47f8af22f216..9b18393d2f95 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -458,6 +458,8 @@ static inline struct tty_struct *tty_kref_get(struct tty_struct *tty)
return tty;
}
+void tty_write_unlock(struct tty_struct *tty, int wakeup);
+int tty_write_lock(struct tty_struct *tty, int ndelay);
extern const char *tty_driver_name(const struct tty_struct *tty);
extern void tty_wait_until_sent(struct tty_struct *tty, long timeout);
extern int __tty_check_change(struct tty_struct *tty, int sig);
--
2.14.3
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 <[email protected]>
> ---
> 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 :)
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?
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?
thanks,
greg k-h
On Wed, 25 Apr 2018 22:20:50 +0900
DaeRyong Jeong <[email protected]> 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.
The tty input layer does not work if it can be executed concurrently. If
that is happening in the pty code then the pty code is buggy and that
needs serializing on the inbound path.
> -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);
> + }
And this may cause deadlocks.
You don't actually need any of the wakeup changes in your code
> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index d9b561d89432..a54ab91aec90 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
> spin_unlock_irq(&tty->flow_lock);
> break;
> case TCOON:
> + if (tty_write_lock(tty, 0) < 0)
> + return -ERESTARTSYS;
> +
> spin_lock_irq(&tty->flow_lock);
> if (tty->flow_stopped) {
> tty->flow_stopped = 0;
> __start_tty(tty);
> }
> spin_unlock_irq(&tty->flow_lock);
> +
> + tty_write_unlock(tty, 0);
If you just used these unmodified it would be simpler and as good,
however it won't actually fix anything. The pty layer is broken not this
code.
The tty layer rule for all the input buffer handling is that you may not
call *any* of it from multiple threads at once. This works fine for
normal serial because the IRQ layer or the polling logic has that
property.
The bug looks real, your diagnosis looks right, your fix sort of works
but isn't sufficient.
So NAK.
Alan
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 <[email protected]>
> > ---
> > 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
On Wed, Apr 25, 2018 at 03:39:48PM +0100, Alan Cox wrote:
> On Wed, 25 Apr 2018 22:20:50 +0900
> DaeRyong Jeong <[email protected]> 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.
>
> The tty input layer does not work if it can be executed concurrently. If
> that is happening in the pty code then the pty code is buggy and that
> needs serializing on the inbound path.
>
>
> > -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);
> > + }
>
> And this may cause deadlocks.
>
> You don't actually need any of the wakeup changes in your code
>
> > diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> > index d9b561d89432..a54ab91aec90 100644
> > --- a/drivers/tty/tty_ioctl.c
> > +++ b/drivers/tty/tty_ioctl.c
> > @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
> > spin_unlock_irq(&tty->flow_lock);
> > break;
> > case TCOON:
> > + if (tty_write_lock(tty, 0) < 0)
> > + return -ERESTARTSYS;
> > +
> > spin_lock_irq(&tty->flow_lock);
> > if (tty->flow_stopped) {
> > tty->flow_stopped = 0;
> > __start_tty(tty);
> > }
> > spin_unlock_irq(&tty->flow_lock);
> > +
> > + tty_write_unlock(tty, 0);
>
> If you just used these unmodified it would be simpler and as good,
> however it won't actually fix anything. The pty layer is broken not this
> code.
Yes. I thought the wrong way to resolve the issue.
>
> The tty layer rule for all the input buffer handling is that you may not
> call *any* of it from multiple threads at once. This works fine for
> normal serial because the IRQ layer or the polling logic has that
> property.
>
> The bug looks real, your diagnosis looks right, your fix sort of works
> but isn't sufficient.
>
> So NAK.
>
> Alan
With your comments, now I know that this patch is incorrect.
Since the bug is real, I will send a new patch with considering all your
comments above.
Thank you a lot for all your comments.
DaeRyong Jeong