Hello,
syzbot found the following crash on:
HEAD commit: 596cf45c Merge branch 'akpm' (patches from Andrew)
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13dc7aeae00000
kernel config: https://syzkaller.appspot.com/x/.config?x=7d8ab2e0e09c2a82
dashboard link: https://syzkaller.appspot.com/bug?extid=522643ab5729b0421998
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15f5d59ce00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1214042ae00000
Bisection is inconclusive: the bug happens on the oldest tested release.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=17b01edae00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=14701edae00000
console output: https://syzkaller.appspot.com/x/log.txt?x=10701edae00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: [email protected]
==================================================================
BUG: KASAN: use-after-free in con_shutdown+0x85/0x90
drivers/tty/vt/vt.c:3278
Write of size 8 at addr ffff88809b797108 by task syz-executor613/9470
CPU: 0 PID: 9470 Comm: syz-executor613 Not tainted 5.4.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x197/0x210 lib/dump_stack.c:118
print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
__kasan_report.cold+0x1b/0x41 mm/kasan/report.c:506
kasan_report+0x12/0x20 mm/kasan/common.c:638
__asan_report_store8_noabort+0x17/0x20 mm/kasan/generic_report.c:140
con_shutdown+0x85/0x90 drivers/tty/vt/vt.c:3278
release_tty+0xd3/0x470 drivers/tty/tty_io.c:1511
tty_release_struct+0x3c/0x50 drivers/tty/tty_io.c:1626
tty_release+0xbcb/0xe90 drivers/tty/tty_io.c:1786
__fput+0x2ff/0x890 fs/file_table.c:280
____fput+0x16/0x20 fs/file_table.c:313
task_work_run+0x145/0x1c0 kernel/task_work.c:113
exit_task_work include/linux/task_work.h:22 [inline]
do_exit+0x8e7/0x2ef0 kernel/exit.c:797
do_group_exit+0x135/0x360 kernel/exit.c:895
__do_sys_exit_group kernel/exit.c:906 [inline]
__se_sys_exit_group kernel/exit.c:904 [inline]
__x64_sys_exit_group+0x44/0x50 kernel/exit.c:904
do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x443c38
Code: 00 00 be 3c 00 00 00 eb 19 66 0f 1f 84 00 00 00 00 00 48 89 d7 89 f0
0f 05 48 3d 00 f0 ff ff 77 21 f4 48 89 d7 44 89 c0 0f 05 <48> 3d 00 f0 ff
ff 76 e0 f7 d8 64 41 89 01 eb d8 0f 1f 84 00 00 00
RSP: 002b:00007ffd7eba55f8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000443c38
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004c37b0 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 000000000000000f R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d5180 R14: 0000000000000000 R15: 0000000000000000
Allocated by task 9465:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_kmalloc mm/kasan/common.c:512 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:485
kasan_kmalloc+0x9/0x10 mm/kasan/common.c:526
kmem_cache_alloc_trace+0x158/0x790 mm/slab.c:3551
kmalloc include/linux/slab.h:556 [inline]
kzalloc include/linux/slab.h:670 [inline]
vc_allocate drivers/tty/vt/vt.c:1085 [inline]
vc_allocate+0x1fc/0x760 drivers/tty/vt/vt.c:1066
con_install+0x52/0x410 drivers/tty/vt/vt.c:3229
tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
tty_init_dev drivers/tty/tty_io.c:1341 [inline]
tty_init_dev+0xf7/0x460 drivers/tty/tty_io.c:1318
tty_open_by_driver drivers/tty/tty_io.c:1985 [inline]
tty_open+0x4a5/0xbb0 drivers/tty/tty_io.c:2033
chrdev_open+0x245/0x6b0 fs/char_dev.c:414
do_dentry_open+0x4e6/0x1380 fs/open.c:797
vfs_open+0xa0/0xd0 fs/open.c:914
do_last fs/namei.c:3412 [inline]
path_openat+0x10e4/0x4710 fs/namei.c:3529
do_filp_open+0x1a1/0x280 fs/namei.c:3559
do_sys_open+0x3fe/0x5d0 fs/open.c:1097
__do_sys_open fs/open.c:1115 [inline]
__se_sys_open fs/open.c:1110 [inline]
__x64_sys_open+0x7e/0xc0 fs/open.c:1110
do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 9471:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
kasan_set_free_info mm/kasan/common.c:334 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/common.c:473
kasan_slab_free+0xe/0x10 mm/kasan/common.c:482
__cache_free mm/slab.c:3426 [inline]
kfree+0x10a/0x2c0 mm/slab.c:3757
vt_disallocate_all+0x2bd/0x3e0 drivers/tty/vt/vt_ioctl.c:323
vt_ioctl+0xc38/0x26d0 drivers/tty/vt/vt_ioctl.c:816
tty_ioctl+0xa37/0x14f0 drivers/tty/tty_io.c:2658
vfs_ioctl fs/ioctl.c:47 [inline]
file_ioctl fs/ioctl.c:539 [inline]
do_vfs_ioctl+0xdb6/0x13e0 fs/ioctl.c:726
ksys_ioctl+0xab/0xd0 fs/ioctl.c:743
__do_sys_ioctl fs/ioctl.c:750 [inline]
__se_sys_ioctl fs/ioctl.c:748 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:748
do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff88809b797000
which belongs to the cache kmalloc-2k of size 2048
The buggy address is located 264 bytes inside of
2048-byte region [ffff88809b797000, ffff88809b797800)
The buggy address belongs to the page:
page:ffffea00026de5c0 refcount:1 mapcount:0 mapping:ffff8880aa400e00
index:0x0
raw: 00fffe0000000200 ffffea00025c8248 ffffea00023a9a88 ffff8880aa400e00
raw: 0000000000000000 ffff88809b797000 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88809b797000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88809b797080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff88809b797100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88809b797180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88809b797200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
From: Eric Biggers <[email protected]>
The VT_DISALLOCATE ioctl can free a virtual console while tty_release()
is still running, causing a use-after-free in con_shutdown(). This
occurs because VT_DISALLOCATE only considers a virtual console to be
in-use if it has a tty_struct with count > 0. But actually when
count == 0, the tty is still in the process of being closed.
Fix this by treating a virtual console as in-use if it has a tty_struct
at all, even with zero count; and by making VT_DISALLOCATE take the
tty_mutex in order to provide synchronization with release_tty().
Reproducer:
#include <fcntl.h>
#include <linux/vt.h>
#include <sys/ioctl.h>
#include <unistd.h>
int main()
{
if (fork()) {
for (;;)
close(open("/dev/tty5", O_RDWR));
} else {
int fd = open("/dev/tty10", O_RDWR);
for (;;)
ioctl(fd, VT_DISALLOCATE, 5);
}
}
KASAN report:
BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
Call Trace:
[...]
con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
[...]
Allocated by task 129:
[...]
kzalloc include/linux/slab.h:669 [inline]
vc_allocate drivers/tty/vt/vt.c:1085 [inline]
vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
[...]
Freed by task 130:
[...]
kfree+0xbf/0x1e0 mm/slab.c:3757
vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
[...]
Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <[email protected]> # v3.4+
Reported-by: [email protected]
Signed-off-by: Eric Biggers <[email protected]>
---
drivers/tty/vt/vt_ioctl.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index ee6c91ef1f6cf..57d681706fa85 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -42,7 +42,7 @@
char vt_dont_switch;
extern struct tty_driver *console_driver;
-#define VT_IS_IN_USE(i) (console_driver->ttys[i] && console_driver->ttys[i]->count)
+#define VT_IS_IN_USE(i) (console_driver->ttys[i] != NULL)
#define VT_BUSY(i) (VT_IS_IN_USE(i) || i == fg_console || vc_cons[i].d == sel_cons)
/*
@@ -288,12 +288,14 @@ static int vt_disallocate(unsigned int vc_num)
struct vc_data *vc = NULL;
int ret = 0;
+ mutex_lock(&tty_mutex); /* synchronize with release_tty() */
console_lock();
if (VT_BUSY(vc_num))
ret = -EBUSY;
else if (vc_num)
vc = vc_deallocate(vc_num);
console_unlock();
+ mutex_unlock(&tty_mutex);
if (vc && vc_num >= MIN_NR_CONSOLES) {
tty_port_destroy(&vc->port);
@@ -309,6 +311,7 @@ static void vt_disallocate_all(void)
struct vc_data *vc[MAX_NR_CONSOLES];
int i;
+ mutex_lock(&tty_mutex); /* synchronize with release_tty() */
console_lock();
for (i = 1; i < MAX_NR_CONSOLES; i++)
if (!VT_BUSY(i))
@@ -316,6 +319,7 @@ static void vt_disallocate_all(void)
else
vc[i] = NULL;
console_unlock();
+ mutex_unlock(&tty_mutex);
for (i = 1; i < MAX_NR_CONSOLES; i++) {
if (vc[i] && i >= MIN_NR_CONSOLES) {
--
2.25.1
On 24. 02. 20, 8:12, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> The VT_DISALLOCATE ioctl can free a virtual console while tty_release()
> is still running, causing a use-after-free in con_shutdown(). This
> occurs because VT_DISALLOCATE only considers a virtual console to be
> in-use if it has a tty_struct with count > 0. But actually when
> count == 0, the tty is still in the process of being closed.
>
> Fix this by treating a virtual console as in-use if it has a tty_struct
> at all, even with zero count; and by making VT_DISALLOCATE take the
> tty_mutex in order to provide synchronization with release_tty().
>
> Reproducer:
> #include <fcntl.h>
> #include <linux/vt.h>
> #include <sys/ioctl.h>
> #include <unistd.h>
>
> int main()
> {
> if (fork()) {
> for (;;)
> close(open("/dev/tty5", O_RDWR));
> } else {
> int fd = open("/dev/tty10", O_RDWR);
>
> for (;;)
> ioctl(fd, VT_DISALLOCATE, 5);
> }
> }
>
> KASAN report:
> BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
>
> CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> Call Trace:
> [...]
> con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
> tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
> tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
> [...]
>
> Allocated by task 129:
> [...]
> kzalloc include/linux/slab.h:669 [inline]
> vc_allocate drivers/tty/vt/vt.c:1085 [inline]
> vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
> con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
> tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
> tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
> tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> [...]
>
> Freed by task 130:
> [...]
> kfree+0xbf/0x1e0 mm/slab.c:3757
> vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
> vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
> tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
That means the associated tty_port is destroyed while the tty layer
still has a tty on the top of it. That is a BUG anyway.
> Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
> Cc: <[email protected]> # v3.4+
> Reported-by: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> drivers/tty/vt/vt_ioctl.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index ee6c91ef1f6cf..57d681706fa85 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -42,7 +42,7 @@
> char vt_dont_switch;
> extern struct tty_driver *console_driver;
>
> -#define VT_IS_IN_USE(i) (console_driver->ttys[i] && console_driver->ttys[i]->count)
> +#define VT_IS_IN_USE(i) (console_driver->ttys[i] != NULL)
> #define VT_BUSY(i) (VT_IS_IN_USE(i) || i == fg_console || vc_cons[i].d == sel_cons)
>
> /*
> @@ -288,12 +288,14 @@ static int vt_disallocate(unsigned int vc_num)
> struct vc_data *vc = NULL;
> int ret = 0;
>
> + mutex_lock(&tty_mutex); /* synchronize with release_tty() */
> console_lock();
Is this lock dependency new or pre-existing?
Locking tty_mutex here does not sound quite right. What about switching
vc_data to proper refcounting based on tty_port? (Instead of doing
tty_port_destroy and kfree in vt_disallocate*.)
> if (VT_BUSY(vc_num))
> ret = -EBUSY;
> else if (vc_num)
> vc = vc_deallocate(vc_num);
> console_unlock();
> + mutex_unlock(&tty_mutex);
>
> if (vc && vc_num >= MIN_NR_CONSOLES) {
> tty_port_destroy(&vc->port);
thanks,
--
js
suse labs
On Mon, Feb 24, 2020 at 09:04:33AM +0100, Jiri Slaby wrote:
> > KASAN report:
> > BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
> >
> > CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> > Call Trace:
> > [...]
> > con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
> > tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
> > tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
> > [...]
> >
> > Allocated by task 129:
> > [...]
> > kzalloc include/linux/slab.h:669 [inline]
> > vc_allocate drivers/tty/vt/vt.c:1085 [inline]
> > vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
> > con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
> > tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
> > tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
> > tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> > tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> > [...]
> >
> > Freed by task 130:
> > [...]
> > kfree+0xbf/0x1e0 mm/slab.c:3757
> > vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
> > vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
> > tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
>
> That means the associated tty_port is destroyed while the tty layer
> still has a tty on the top of it. That is a BUG anyway.
>
> > Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
> > Cc: <[email protected]> # v3.4+
> > Reported-by: [email protected]
> > Signed-off-by: Eric Biggers <[email protected]>
> > ---
> > drivers/tty/vt/vt_ioctl.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > index ee6c91ef1f6cf..57d681706fa85 100644
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -42,7 +42,7 @@
> > char vt_dont_switch;
> > extern struct tty_driver *console_driver;
> >
> > -#define VT_IS_IN_USE(i) (console_driver->ttys[i] && console_driver->ttys[i]->count)
> > +#define VT_IS_IN_USE(i) (console_driver->ttys[i] != NULL)
> > #define VT_BUSY(i) (VT_IS_IN_USE(i) || i == fg_console || vc_cons[i].d == sel_cons)
> >
> > /*
> > @@ -288,12 +288,14 @@ static int vt_disallocate(unsigned int vc_num)
> > struct vc_data *vc = NULL;
> > int ret = 0;
> >
> > + mutex_lock(&tty_mutex); /* synchronize with release_tty() */
> > console_lock();
>
> Is this lock dependency new or pre-existing?
It's the same locking order used during release_tty().
>
> Locking tty_mutex here does not sound quite right. What about switching
> vc_data to proper refcounting based on tty_port? (Instead of doing
> tty_port_destroy and kfree in vt_disallocate*.)
>
How would that work? We could make struct vc_data refcounted such that
VT_DISALLOCATE doesn't free it right away but rather it's freed in the next
con_shutdown(). But release_tty() still accesses tty->port afterwards, which is
part of the 'struct vc_data' that would have just been freed.
- Eric
On Mon, Feb 24, 2020 at 12:19:13AM -0800, Eric Biggers wrote:
> On Mon, Feb 24, 2020 at 09:04:33AM +0100, Jiri Slaby wrote:
> > > KASAN report:
> > > BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > > Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
> > >
> > > CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
> > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> > > Call Trace:
> > > [...]
> > > con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > > release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
> > > tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
> > > tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
> > > [...]
> > >
> > > Allocated by task 129:
> > > [...]
> > > kzalloc include/linux/slab.h:669 [inline]
> > > vc_allocate drivers/tty/vt/vt.c:1085 [inline]
> > > vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
> > > con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
> > > tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
> > > tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
> > > tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> > > tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> > > [...]
> > >
> > > Freed by task 130:
> > > [...]
> > > kfree+0xbf/0x1e0 mm/slab.c:3757
> > > vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
> > > vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
> > > tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
> >
> > That means the associated tty_port is destroyed while the tty layer
> > still has a tty on the top of it. That is a BUG anyway.
> >
> > > Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
> > > Cc: <[email protected]> # v3.4+
> > > Reported-by: [email protected]
> > > Signed-off-by: Eric Biggers <[email protected]>
> > > ---
> > > drivers/tty/vt/vt_ioctl.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > > index ee6c91ef1f6cf..57d681706fa85 100644
> > > --- a/drivers/tty/vt/vt_ioctl.c
> > > +++ b/drivers/tty/vt/vt_ioctl.c
> > > @@ -42,7 +42,7 @@
> > > char vt_dont_switch;
> > > extern struct tty_driver *console_driver;
> > >
> > > -#define VT_IS_IN_USE(i) (console_driver->ttys[i] && console_driver->ttys[i]->count)
> > > +#define VT_IS_IN_USE(i) (console_driver->ttys[i] != NULL)
> > > #define VT_BUSY(i) (VT_IS_IN_USE(i) || i == fg_console || vc_cons[i].d == sel_cons)
> > >
> > > /*
> > > @@ -288,12 +288,14 @@ static int vt_disallocate(unsigned int vc_num)
> > > struct vc_data *vc = NULL;
> > > int ret = 0;
> > >
> > > + mutex_lock(&tty_mutex); /* synchronize with release_tty() */
> > > console_lock();
> >
> > Is this lock dependency new or pre-existing?
>
> It's the same locking order used during release_tty().
>
> >
> > Locking tty_mutex here does not sound quite right. What about switching
> > vc_data to proper refcounting based on tty_port? (Instead of doing
> > tty_port_destroy and kfree in vt_disallocate*.)
> >
>
> How would that work? We could make struct vc_data refcounted such that
> VT_DISALLOCATE doesn't free it right away but rather it's freed in the next
> con_shutdown(). But release_tty() still accesses tty->port afterwards, which is
> part of the 'struct vc_data' that would have just been freed.
>
Jiri, can you explain what you meant here? I don't see how your suggestion
would solve the problem.
Greg, any opinion?
- Eric
On Mon, Mar 02, 2020 at 01:23:06PM -0800, Eric Biggers wrote:
> On Mon, Feb 24, 2020 at 12:19:13AM -0800, Eric Biggers wrote:
> > On Mon, Feb 24, 2020 at 09:04:33AM +0100, Jiri Slaby wrote:
> > > > KASAN report:
> > > > BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > > > Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
> > > >
> > > > CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
> > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> > > > Call Trace:
> > > > [...]
> > > > con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> > > > release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
> > > > tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
> > > > tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
> > > > [...]
> > > >
> > > > Allocated by task 129:
> > > > [...]
> > > > kzalloc include/linux/slab.h:669 [inline]
> > > > vc_allocate drivers/tty/vt/vt.c:1085 [inline]
> > > > vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
> > > > con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
> > > > tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
> > > > tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
> > > > tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> > > > tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> > > > [...]
> > > >
> > > > Freed by task 130:
> > > > [...]
> > > > kfree+0xbf/0x1e0 mm/slab.c:3757
> > > > vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
> > > > vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
> > > > tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
> > >
> > > That means the associated tty_port is destroyed while the tty layer
> > > still has a tty on the top of it. That is a BUG anyway.
> > >
> > > > Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
> > > > Cc: <[email protected]> # v3.4+
> > > > Reported-by: [email protected]
> > > > Signed-off-by: Eric Biggers <[email protected]>
> > > > ---
> > > > drivers/tty/vt/vt_ioctl.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> > > > index ee6c91ef1f6cf..57d681706fa85 100644
> > > > --- a/drivers/tty/vt/vt_ioctl.c
> > > > +++ b/drivers/tty/vt/vt_ioctl.c
> > > > @@ -42,7 +42,7 @@
> > > > char vt_dont_switch;
> > > > extern struct tty_driver *console_driver;
> > > >
> > > > -#define VT_IS_IN_USE(i) (console_driver->ttys[i] && console_driver->ttys[i]->count)
> > > > +#define VT_IS_IN_USE(i) (console_driver->ttys[i] != NULL)
> > > > #define VT_BUSY(i) (VT_IS_IN_USE(i) || i == fg_console || vc_cons[i].d == sel_cons)
> > > >
> > > > /*
> > > > @@ -288,12 +288,14 @@ static int vt_disallocate(unsigned int vc_num)
> > > > struct vc_data *vc = NULL;
> > > > int ret = 0;
> > > >
> > > > + mutex_lock(&tty_mutex); /* synchronize with release_tty() */
> > > > console_lock();
> > >
> > > Is this lock dependency new or pre-existing?
> >
> > It's the same locking order used during release_tty().
> >
> > >
> > > Locking tty_mutex here does not sound quite right. What about switching
> > > vc_data to proper refcounting based on tty_port? (Instead of doing
> > > tty_port_destroy and kfree in vt_disallocate*.)
> > >
> >
> > How would that work? We could make struct vc_data refcounted such that
> > VT_DISALLOCATE doesn't free it right away but rather it's freed in the next
> > con_shutdown(). But release_tty() still accesses tty->port afterwards, which is
> > part of the 'struct vc_data' that would have just been freed.
> >
>
> Jiri, can you explain what you meant here? I don't see how your suggestion
> would solve the problem.
>
> Greg, any opinion?
I'll defer to Jiri here :)
On 18. 03. 20, 11:06, Greg Kroah-Hartman wrote:
>> Jiri, can you explain what you meant here? I don't see how your suggestion
>> would solve the problem.
>>
>> Greg, any opinion?
>
> I'll defer to Jiri here :)
Heh, thanks.
I started looking into this yesterday, but was interrupted by other
tasks. Stay tuned.
regards,
--
js
suse labs
On 24. 02. 20, 9:19, Eric Biggers wrote:
> On Mon, Feb 24, 2020 at 09:04:33AM +0100, Jiri Slaby wrote:
>>> KASAN report:
>>> BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
>>> Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
>>>
>>> CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
>>> Call Trace:
>>> [...]
>>> con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
>>> release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
>>> tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
>>> tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
>>> [...]
>>>
>>> Allocated by task 129:
>>> [...]
>>> kzalloc include/linux/slab.h:669 [inline]
>>> vc_allocate drivers/tty/vt/vt.c:1085 [inline]
>>> vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
>>> con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
>>> tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
>>> tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
>>> tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
>>> tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
>>> [...]
>>>
>>> Freed by task 130:
>>> [...]
>>> kfree+0xbf/0x1e0 mm/slab.c:3757
>>> vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
>>> vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
>>> tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
>>
>> That means the associated tty_port is destroyed while the tty layer
>> still has a tty on the top of it. That is a BUG anyway.
...
>> Locking tty_mutex here does not sound quite right. What about switching
>> vc_data to proper refcounting based on tty_port? (Instead of doing
>> tty_port_destroy and kfree in vt_disallocate*.)
>>
>
> How would that work? We could make struct vc_data refcounted such that
> VT_DISALLOCATE doesn't free it right away but rather it's freed in the next
> con_shutdown(). But release_tty() still accesses tty->port afterwards, which is
> part of the 'struct vc_data' that would have just been freed.
Yes, but if it does the same as pty, i.e. throwing tty_port in
->cleanup, not ->shutdown, that would work, right?
The initial requirement from tty_port is that it outlives tty. This was
later lifted by pty to live at least till ->cleanup.
thanks,
--
js
suse labs
On Wed, Mar 18, 2020 at 02:15:00PM +0100, Jiri Slaby wrote:
> On 24. 02. 20, 9:19, Eric Biggers wrote:
> > On Mon, Feb 24, 2020 at 09:04:33AM +0100, Jiri Slaby wrote:
> >>> KASAN report:
> >>> BUG: KASAN: use-after-free in con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> >>> Write of size 8 at addr ffff88806a4ec108 by task syz_vt/129
> >>>
> >>> CPU: 0 PID: 129 Comm: syz_vt Not tainted 5.6.0-rc2 #11
> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> >>> Call Trace:
> >>> [...]
> >>> con_shutdown+0x76/0x80 drivers/tty/vt/vt.c:3278
> >>> release_tty+0xa8/0x410 drivers/tty/tty_io.c:1514
> >>> tty_release_struct+0x34/0x50 drivers/tty/tty_io.c:1629
> >>> tty_release+0x984/0xed0 drivers/tty/tty_io.c:1789
> >>> [...]
> >>>
> >>> Allocated by task 129:
> >>> [...]
> >>> kzalloc include/linux/slab.h:669 [inline]
> >>> vc_allocate drivers/tty/vt/vt.c:1085 [inline]
> >>> vc_allocate+0x1ac/0x680 drivers/tty/vt/vt.c:1066
> >>> con_install+0x4d/0x3f0 drivers/tty/vt/vt.c:3229
> >>> tty_driver_install_tty drivers/tty/tty_io.c:1228 [inline]
> >>> tty_init_dev+0x94/0x350 drivers/tty/tty_io.c:1341
> >>> tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> >>> tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> >>> [...]
> >>>
> >>> Freed by task 130:
> >>> [...]
> >>> kfree+0xbf/0x1e0 mm/slab.c:3757
> >>> vt_disallocate drivers/tty/vt/vt_ioctl.c:300 [inline]
> >>> vt_ioctl+0x16dc/0x1e30 drivers/tty/vt/vt_ioctl.c:818
> >>> tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
> >>
> >> That means the associated tty_port is destroyed while the tty layer
> >> still has a tty on the top of it. That is a BUG anyway.
>
> ...
>
> >> Locking tty_mutex here does not sound quite right. What about switching
> >> vc_data to proper refcounting based on tty_port? (Instead of doing
> >> tty_port_destroy and kfree in vt_disallocate*.)
> >>
> >
> > How would that work? We could make struct vc_data refcounted such that
> > VT_DISALLOCATE doesn't free it right away but rather it's freed in the next
> > con_shutdown(). But release_tty() still accesses tty->port afterwards, which is
> > part of the 'struct vc_data' that would have just been freed.
>
> Yes, but if it does the same as pty, i.e. throwing tty_port in
> ->cleanup, not ->shutdown, that would work, right?
>
> The initial requirement from tty_port is that it outlives tty. This was
> later lifted by pty to live at least till ->cleanup.
>
Yes, it looks like that will work. I didn't know about
tty_operations::cleanup(). I'll update the patch.
- Eric
Fix VT_DISALLOCATE freeing an in-use virtual console, and fix a
use-after-free in vt_in_use().
Changed since v1:
- Made the vc_data be freed via tty_port refcounting.
- Added patch to fix a use-after-free in vt_in_use().
Eric Biggers (2):
vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console
vt: vt_ioctl: fix use-after-free in vt_in_use()
drivers/tty/vt/vt.c | 14 +++++++++++++-
drivers/tty/vt/vt_ioctl.c | 24 ++++++++++++------------
2 files changed, 25 insertions(+), 13 deletions(-)
--
2.25.1