KASAN: use-after-free Write in release_tty

KASAN: use-after-free Write in release_tty

[syzbot]

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: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com

==================================================================
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 syzkaller@googlegroups.com.

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

[PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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: <stable@vger.kernel.org> # v3.4+
Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Jiri Slaby]

On 24. 02. 20, 8:12, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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: <stable@vger.kernel.org> # v3.4+
> Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

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: <stable@vger.kernel.org> # v3.4+
> > Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

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: <stable@vger.kernel.org> # v3.4+
> > > Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > >  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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Greg Kroah-Hartman]

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: <stable@vger.kernel.org> # v3.4+
> > > > Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > > ---
> > > >  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 :)

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Jiri Slaby]

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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Jiri Slaby]

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

Re: [PATCH] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

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

[PATCH v2 0/2] vt: fix some vt_ioctl races

[Eric Biggers]

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

[PATCH v2 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 considers a virtual console's
'struct vc_data' to be unused as soon as the corresponding tty's
refcount hits 0.  But actually it may be still being closed.

Fix this by making vc_data be reference-counted via the embedded
'struct tty_port'.  A newly allocated virtual console has refcount 1.
Opening it for the first time increments the refcount to 2.  Closing it
for the last time decrements the refcount (in tty_operations::cleanup()
so that it happens late enough), as does VT_DISALLOCATE.

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: <stable@vger.kernel.org> # v3.4+
Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/vt/vt.c       | 14 +++++++++++++-
 drivers/tty/vt/vt_ioctl.c | 12 ++++--------
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index bbc26d73209a4..ec34f1f5f3bb5 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1102,6 +1102,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
 	tty_port_init(&vc->port);
 	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
 
+	/* if this wasn't the case, we'd have to implement port->ops.destruct */
+	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
+
 	visual_init(vc, currcons, 1);
 
 	if (!*vc->vc_uni_pagedir_loc)
@@ -3250,6 +3253,7 @@ static int con_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	tty->driver_data = vc;
 	vc->port.tty = tty;
+	tty_port_get(&vc->port);
 
 	if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 		tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
@@ -3285,6 +3289,13 @@ static void con_shutdown(struct tty_struct *tty)
 	console_unlock();
 }
 
+static void con_cleanup(struct tty_struct *tty)
+{
+	struct vc_data *vc = tty->driver_data;
+
+	tty_port_put(&vc->port);
+}
+
 static int default_color           = 7; /* white */
 static int default_italic_color    = 2; // green (ASCII)
 static int default_underline_color = 3; // cyan (ASCII)
@@ -3410,7 +3421,8 @@ static const struct tty_operations con_ops = {
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
 	.resize = vt_resize,
-	.shutdown = con_shutdown
+	.shutdown = con_shutdown,
+	.cleanup = con_cleanup,
 };
 
 static struct cdev vc0_cdev;
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 7297997fcf04c..f62f498f63c05 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -310,10 +310,8 @@ static int vt_disallocate(unsigned int vc_num)
 		vc = vc_deallocate(vc_num);
 	console_unlock();
 
-	if (vc && vc_num >= MIN_NR_CONSOLES) {
-		tty_port_destroy(&vc->port);
-		kfree(vc);
-	}
+	if (vc && vc_num >= MIN_NR_CONSOLES)
+		tty_port_put(&vc->port);
 
 	return ret;
 }
@@ -333,10 +331,8 @@ static void vt_disallocate_all(void)
 	console_unlock();
 
 	for (i = 1; i < MAX_NR_CONSOLES; i++) {
-		if (vc[i] && i >= MIN_NR_CONSOLES) {
-			tty_port_destroy(&vc[i]->port);
-			kfree(vc[i]);
-		}
+		if (vc[i] && i >= MIN_NR_CONSOLES)
+			tty_port_put(&vc[i]->port);
 	}
 }
 
-- 
2.25.1

[PATCH v2 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

vt_in_use() dereferences console_driver->ttys[i] without proper locking.
This is broken because the tty can be closed and freed concurrently.

We could fix this by using 'READ_ONCE(console_driver->ttys[i]) != NULL'
and skipping the check of tty_struct::count.  But, looking at
console_driver->ttys[i] isn't really appropriate anyway because even if
it is NULL the tty can still be in the process of being closed.

Instead, fix it by making vt_in_use() require console_lock() and check
whether the vt is allocated and has port refcount > 1.  This works since
following the patch "vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use
virtual console" the port refcount is incremented while the vt is open.

Reproducer (very unreliable, but it worked for me after a few minutes):

	#include <fcntl.h>
	#include <linux/vt.h>

	int main()
	{
		int fd, nproc;
		struct vt_stat state;
		char ttyname[16];

		fd = open("/dev/tty10", O_RDONLY);
		for (nproc = 1; nproc < 8; nproc *= 2)
			fork();
		for (;;) {
			sprintf(ttyname, "/dev/tty%d", rand() % 8);
			close(open(ttyname, O_RDONLY));
			ioctl(fd, VT_GETSTATE, &state);
		}
	}

KASAN report:

	BUG: KASAN: use-after-free in vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	BUG: KASAN: use-after-free in vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	Read of size 4 at addr ffff888065722468 by task syz-vt2/132

	CPU: 0 PID: 132 Comm: syz-vt2 Not tainted 5.6.0-rc5-00130-g089b6d3654916 #13
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	Call Trace:
	 [...]
	 vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	 vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	 tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
	 [...]

	Allocated by task 136:
	 [...]
	 kzalloc include/linux/slab.h:669 [inline]
	 alloc_tty_struct+0x96/0x8a0 drivers/tty/tty_io.c:2982
	 tty_init_dev+0x23/0x350 drivers/tty/tty_io.c:1334
	 tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
	 tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
	 [...]

	Freed by task 41:
	 [...]
	 kfree+0xbf/0x200 mm/slab.c:3757
	 free_tty_struct+0x8d/0xb0 drivers/tty/tty_io.c:177
	 release_one_tty+0x22d/0x2f0 drivers/tty/tty_io.c:1468
	 process_one_work+0x7f1/0x14b0 kernel/workqueue.c:2264
	 worker_thread+0x8b/0xc80 kernel/workqueue.c:2410
	 [...]

Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <stable@vger.kernel.org> # v3.4+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/vt/vt_ioctl.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index f62f498f63c05..ca0e57b1c0b43 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -43,9 +43,11 @@ bool vt_dont_switch;
 
 static inline bool vt_in_use(unsigned int i)
 {
-	extern struct tty_driver *console_driver;
+	const struct vc_data *vc = vc_cons[i].d;
 
-	return console_driver->ttys[i] && console_driver->ttys[i]->count;
+	WARN_CONSOLE_UNLOCKED();
+
+	return vc && kref_read(&vc->port.kref) > 1;
 }
 
 static inline bool vt_busy(int i)
@@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_stat __user *vtstat = up;
 		unsigned short state, mask;
 
-		/* Review: FIXME: Console lock ? */
 		if (put_user(fg_console + 1, &vtstat->v_active))
 			ret = -EFAULT;
 		else {
 			state = 1;	/* /dev/tty0 is always open */
+			console_lock();
 			for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
 							++i, mask <<= 1)
 				if (vt_in_use(i))
 					state |= mask;
+			console_unlock();
 			ret = put_user(state, &vtstat->v_state);
 		}
 		break;
@@ -661,10 +664,11 @@ int vt_ioctl(struct tty_struct *tty,
 	 * Returns the first available (non-opened) console.
 	 */
 	case VT_OPENQRY:
-		/* FIXME: locking ? - but then this is a stupid API */
+		console_lock();
 		for (i = 0; i < MAX_NR_CONSOLES; ++i)
 			if (!vt_in_use(i))
 				break;
+		console_unlock();
 		uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
 		goto setint;		 
 
-- 
2.25.1

Re: [PATCH v2 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Jiri Slaby]

On 18. 03. 20, 23:38, Eric Biggers wrote:
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1102,6 +1102,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  	tty_port_init(&vc->port);
>  	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>  
> +	/* if this wasn't the case, we'd have to implement port->ops.destruct */
> +	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
> +

This is 3 lines, implementing destruct would be like 4-5 :)? Please
implement destruct instead.

Otherwise looks good.

thanks,
-- 
js
suse labs

Re: [PATCH v2 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

On Thu, Mar 19, 2020 at 08:36:28AM +0100, Jiri Slaby wrote:
> On 18. 03. 20, 23:38, Eric Biggers wrote:
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -1102,6 +1102,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
> >  	tty_port_init(&vc->port);
> >  	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> >  
> > +	/* if this wasn't the case, we'd have to implement port->ops.destruct */
> > +	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
> > +
> 
> This is 3 lines, implementing destruct would be like 4-5 :)? Please
> implement destruct instead.
> 
> Otherwise looks good.
> 

Actually implementing destruct would be 12 lines, see below.  Remember there is
no tty_port_operations defined yet so we'd have to define it just for destruct.

Do you still prefer it?

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index ec34f1f5f3bb5..309a39197be0a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1075,6 +1075,17 @@ static void visual_deinit(struct vc_data *vc)
 	module_put(vc->vc_sw->owner);
 }
 
+static void vc_port_destruct(struct tty_port *port)
+{
+	struct vc_data *vc = container_of(port, struct vc_data, port);
+
+	kfree(vc);
+}
+
+static const struct tty_port_operations vc_port_ops = {
+	.destruct = vc_port_destruct,
+};
+
 int vc_allocate(unsigned int currcons)	/* return 0 on success */
 {
 	struct vt_notifier_param param;
@@ -1100,11 +1111,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
 
 	vc_cons[currcons].d = vc;
 	tty_port_init(&vc->port);
+	vc->port.ops = &vc_port_ops;
 	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
 
-	/* if this wasn't the case, we'd have to implement port->ops.destruct */
-	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
-
 	visual_init(vc, currcons, 1);
 
 	if (!*vc->vc_uni_pagedir_loc)

Re: [PATCH v2 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Greg Kroah-Hartman]

On Thu, Mar 19, 2020 at 10:10:49PM -0700, Eric Biggers wrote:
> On Thu, Mar 19, 2020 at 08:36:28AM +0100, Jiri Slaby wrote:
> > On 18. 03. 20, 23:38, Eric Biggers wrote:
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -1102,6 +1102,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
> > >  	tty_port_init(&vc->port);
> > >  	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > >  
> > > +	/* if this wasn't the case, we'd have to implement port->ops.destruct */
> > > +	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
> > > +
> > 
> > This is 3 lines, implementing destruct would be like 4-5 :)? Please
> > implement destruct instead.
> > 
> > Otherwise looks good.
> > 
> 
> Actually implementing destruct would be 12 lines, see below.  Remember there is
> no tty_port_operations defined yet so we'd have to define it just for destruct.
> 
> Do you still prefer it?
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index ec34f1f5f3bb5..309a39197be0a 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1075,6 +1075,17 @@ static void visual_deinit(struct vc_data *vc)
>  	module_put(vc->vc_sw->owner);
>  }
>  
> +static void vc_port_destruct(struct tty_port *port)
> +{
> +	struct vc_data *vc = container_of(port, struct vc_data, port);
> +
> +	kfree(vc);
> +}
> +
> +static const struct tty_port_operations vc_port_ops = {
> +	.destruct = vc_port_destruct,
> +};
> +
>  int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  {
>  	struct vt_notifier_param param;
> @@ -1100,11 +1111,9 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  
>  	vc_cons[currcons].d = vc;
>  	tty_port_init(&vc->port);
> +	vc->port.ops = &vc_port_ops;
>  	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>  
> -	/* if this wasn't the case, we'd have to implement port->ops.destruct */
> -	BUILD_BUG_ON(offsetof(struct vc_data, port) != 0);
> -
>  	visual_init(vc, currcons, 1);
>  
>  	if (!*vc->vc_uni_pagedir_loc)


Yes, this is good to have, thanks for doing this.

greg k-h

Re: [PATCH v2 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Jiri Slaby]

On 18. 03. 20, 23:38, Eric Biggers wrote:
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -43,9 +43,11 @@ bool vt_dont_switch;
>  
>  static inline bool vt_in_use(unsigned int i)
>  {
> -	extern struct tty_driver *console_driver;
> +	const struct vc_data *vc = vc_cons[i].d;
>  
> -	return console_driver->ttys[i] && console_driver->ttys[i]->count;
> +	WARN_CONSOLE_UNLOCKED();
> +
> +	return vc && kref_read(&vc->port.kref) > 1;
>  }
>  
>  static inline bool vt_busy(int i)
> @@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
>  		struct vt_stat __user *vtstat = up;
>  		unsigned short state, mask;
>  
> -		/* Review: FIXME: Console lock ? */
>  		if (put_user(fg_console + 1, &vtstat->v_active))
>  			ret = -EFAULT;
>  		else {
>  			state = 1;	/* /dev/tty0 is always open */
> +			console_lock();

Could you comment on this one and the lock below why you added it here?

To me, it seems, we should rather introduce a vt alloc/dealloc lock
protecting cases like this, not console lock. But not now, some time
later. So a comment would help when/once we/I get into it...

The interface (ie. the ioctls) also look weird and racy. Both of them.
Like the "OK, I give you this number, but it might not be correct by
now." kind of thing.

This let me think, who could use this? The answer is many 8-/. openpt,
systemd, sysvinit, didn't check others.

Perhaps we should provide openvt -- analogy of openpty and deprecate
VT_OPENQRY?

With VT_GETSTATE, the situation is more complicated:
sysvinit uses VT_GETSTATE only if TIOCGDEV is not available, so
VT_GETSTATE is actually unneeded there.

systemd uses it to find the current console (vtstat->v_active) and
systemd-logind uses it for spawning autovt on free consoles. That sort
of makes sense...

>  			for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
>  							++i, mask <<= 1)
>  				if (vt_in_use(i))
>  					state |= mask;
> +			console_unlock();
>  			ret = put_user(state, &vtstat->v_state);
>  		}
>  		break;
> @@ -661,10 +664,11 @@ int vt_ioctl(struct tty_struct *tty,
>  	 * Returns the first available (non-opened) console.
>  	 */
>  	case VT_OPENQRY:
> -		/* FIXME: locking ? - but then this is a stupid API */
> +		console_lock();
>  		for (i = 0; i < MAX_NR_CONSOLES; ++i)
>  			if (!vt_in_use(i))
>  				break;
> +		console_unlock();
>  		uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
>  		goto setint;		 
>  
> 

thanks,
-- 
js
suse labs

Re: [PATCH v2 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Eric Biggers]

On Fri, Mar 20, 2020 at 02:42:12PM +0100, Jiri Slaby wrote:
> On 18. 03. 20, 23:38, Eric Biggers wrote:
> > --- a/drivers/tty/vt/vt_ioctl.c
> > +++ b/drivers/tty/vt/vt_ioctl.c
> > @@ -43,9 +43,11 @@ bool vt_dont_switch;
> >  
> >  static inline bool vt_in_use(unsigned int i)
> >  {
> > -	extern struct tty_driver *console_driver;
> > +	const struct vc_data *vc = vc_cons[i].d;
> >  
> > -	return console_driver->ttys[i] && console_driver->ttys[i]->count;
> > +	WARN_CONSOLE_UNLOCKED();
> > +
> > +	return vc && kref_read(&vc->port.kref) > 1;
> >  }
> >  
> >  static inline bool vt_busy(int i)
> > @@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
> >  		struct vt_stat __user *vtstat = up;
> >  		unsigned short state, mask;
> >  
> > -		/* Review: FIXME: Console lock ? */
> >  		if (put_user(fg_console + 1, &vtstat->v_active))
> >  			ret = -EFAULT;
> >  		else {
> >  			state = 1;	/* /dev/tty0 is always open */
> > +			console_lock();
> 
> Could you comment on this one and the lock below why you added it here?
> 
> To me, it seems, we should rather introduce a vt alloc/dealloc lock
> protecting cases like this, not console lock. But not now, some time
> later. So a comment would help when/once we/I get into it...

I think the locking I added to VT_GETSTATE and VT_OPENQRY is pretty
self-explanatory: it's needed because they call vt_in_use() which now requires
console_lock.  So I'm not sure what you'd like me to add there?

As for vt_in_use() itself, I already added WARN_CONSOLE_UNLOCKED() to it.
But I can add a comment to it too if it would be useful, like:

static inline bool vt_in_use(unsigned int i)
{
        const struct vc_data *vc = vc_cons[i].d;

        /*
         * console_lock must be held to prevent the vc from being deallocated
         * while we're checking whether it's in-use.
         */
        WARN_CONSOLE_UNLOCKED();

        return vc && kref_read(&vc->port.kref) > 1;
}


> The interface (ie. the ioctls) also look weird and racy. Both of them.
> Like the "OK, I give you this number, but it might not be correct by
> now." kind of thing.
> 
> This let me think, who could use this? The answer is many 8-/. openpt,
> systemd, sysvinit, didn't check others.
> 
> Perhaps we should provide openvt -- analogy of openpty and deprecate
> VT_OPENQRY?
> 
> With VT_GETSTATE, the situation is more complicated:
> sysvinit uses VT_GETSTATE only if TIOCGDEV is not available, so
> VT_GETSTATE is actually unneeded there.
> 
> systemd uses it to find the current console (vtstat->v_active) and
> systemd-logind uses it for spawning autovt on free consoles. That sort
> of makes sense...
> 

Yes, these are bad APIs.

Once I did remove a buggy ioctl elsewhere in the kernel rather than fixing it.
But you have to be very, very confident that nothing is using it.  That doesn't
seem to be the case for VT_GETSTATE and VT_OPENQRY as it's not hard to find code
using them.  E.g. here's another user of both of them:
https://android.googlesource.com/platform/system/core/+/ccecf1425412beb2bc3bb38d470293fdc244d6f1/toolbox/setconsole.c

So we're probably stuck with them for now.  If you'd like to explore adding a
better API and trying to get all users to use it, you're certainly welcome to.
But it would be orthogonal to fixing this bug.

- Eric

[PATCH v3 0/2] vt: fix some vt_ioctl races

[Eric Biggers]

Fix VT_DISALLOCATE freeing an in-use virtual console, and fix a
use-after-free in vt_in_use().

Changed since v2:
    - Implemented tty_port_operations::destruct().
    - Added comments regarding vt_in_use() locking.

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       | 23 ++++++++++++++++++++++-
 drivers/tty/vt/vt_ioctl.c | 28 ++++++++++++++++------------
 2 files changed, 38 insertions(+), 13 deletions(-)

-- 
2.25.2

[PATCH v3 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 considers a virtual console's
'struct vc_data' to be unused as soon as the corresponding tty's
refcount hits 0.  But actually it may be still being closed.

Fix this by making vc_data be reference-counted via the embedded
'struct tty_port'.  A newly allocated virtual console has refcount 1.
Opening it for the first time increments the refcount to 2.  Closing it
for the last time decrements the refcount (in tty_operations::cleanup()
so that it happens late enough), as does VT_DISALLOCATE.

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: <stable@vger.kernel.org> # v3.4+
Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/vt/vt.c       | 23 ++++++++++++++++++++++-
 drivers/tty/vt/vt_ioctl.c | 12 ++++--------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index bbc26d73209a4..309a39197be0a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1075,6 +1075,17 @@ static void visual_deinit(struct vc_data *vc)
 	module_put(vc->vc_sw->owner);
 }
 
+static void vc_port_destruct(struct tty_port *port)
+{
+	struct vc_data *vc = container_of(port, struct vc_data, port);
+
+	kfree(vc);
+}
+
+static const struct tty_port_operations vc_port_ops = {
+	.destruct = vc_port_destruct,
+};
+
 int vc_allocate(unsigned int currcons)	/* return 0 on success */
 {
 	struct vt_notifier_param param;
@@ -1100,6 +1111,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
 
 	vc_cons[currcons].d = vc;
 	tty_port_init(&vc->port);
+	vc->port.ops = &vc_port_ops;
 	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
 
 	visual_init(vc, currcons, 1);
@@ -3250,6 +3262,7 @@ static int con_install(struct tty_driver *driver, struct tty_struct *tty)
 
 	tty->driver_data = vc;
 	vc->port.tty = tty;
+	tty_port_get(&vc->port);
 
 	if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
 		tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
@@ -3285,6 +3298,13 @@ static void con_shutdown(struct tty_struct *tty)
 	console_unlock();
 }
 
+static void con_cleanup(struct tty_struct *tty)
+{
+	struct vc_data *vc = tty->driver_data;
+
+	tty_port_put(&vc->port);
+}
+
 static int default_color           = 7; /* white */
 static int default_italic_color    = 2; // green (ASCII)
 static int default_underline_color = 3; // cyan (ASCII)
@@ -3410,7 +3430,8 @@ static const struct tty_operations con_ops = {
 	.throttle = con_throttle,
 	.unthrottle = con_unthrottle,
 	.resize = vt_resize,
-	.shutdown = con_shutdown
+	.shutdown = con_shutdown,
+	.cleanup = con_cleanup,
 };
 
 static struct cdev vc0_cdev;
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 7297997fcf04c..f62f498f63c05 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -310,10 +310,8 @@ static int vt_disallocate(unsigned int vc_num)
 		vc = vc_deallocate(vc_num);
 	console_unlock();
 
-	if (vc && vc_num >= MIN_NR_CONSOLES) {
-		tty_port_destroy(&vc->port);
-		kfree(vc);
-	}
+	if (vc && vc_num >= MIN_NR_CONSOLES)
+		tty_port_put(&vc->port);
 
 	return ret;
 }
@@ -333,10 +331,8 @@ static void vt_disallocate_all(void)
 	console_unlock();
 
 	for (i = 1; i < MAX_NR_CONSOLES; i++) {
-		if (vc[i] && i >= MIN_NR_CONSOLES) {
-			tty_port_destroy(&vc[i]->port);
-			kfree(vc[i]);
-		}
+		if (vc[i] && i >= MIN_NR_CONSOLES)
+			tty_port_put(&vc[i]->port);
 	}
 }
 
-- 
2.25.2

[PATCH v3 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

vt_in_use() dereferences console_driver->ttys[i] without proper locking.
This is broken because the tty can be closed and freed concurrently.

We could fix this by using 'READ_ONCE(console_driver->ttys[i]) != NULL'
and skipping the check of tty_struct::count.  But, looking at
console_driver->ttys[i] isn't really appropriate anyway because even if
it is NULL the tty can still be in the process of being closed.

Instead, fix it by making vt_in_use() require console_lock() and check
whether the vt is allocated and has port refcount > 1.  This works since
following the patch "vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use
virtual console" the port refcount is incremented while the vt is open.

Reproducer (very unreliable, but it worked for me after a few minutes):

	#include <fcntl.h>
	#include <linux/vt.h>

	int main()
	{
		int fd, nproc;
		struct vt_stat state;
		char ttyname[16];

		fd = open("/dev/tty10", O_RDONLY);
		for (nproc = 1; nproc < 8; nproc *= 2)
			fork();
		for (;;) {
			sprintf(ttyname, "/dev/tty%d", rand() % 8);
			close(open(ttyname, O_RDONLY));
			ioctl(fd, VT_GETSTATE, &state);
		}
	}

KASAN report:

	BUG: KASAN: use-after-free in vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	BUG: KASAN: use-after-free in vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	Read of size 4 at addr ffff888065722468 by task syz-vt2/132

	CPU: 0 PID: 132 Comm: syz-vt2 Not tainted 5.6.0-rc5-00130-g089b6d3654916 #13
	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
	Call Trace:
	 [...]
	 vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
	 vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
	 tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
	 [...]

	Allocated by task 136:
	 [...]
	 kzalloc include/linux/slab.h:669 [inline]
	 alloc_tty_struct+0x96/0x8a0 drivers/tty/tty_io.c:2982
	 tty_init_dev+0x23/0x350 drivers/tty/tty_io.c:1334
	 tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
	 tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
	 [...]

	Freed by task 41:
	 [...]
	 kfree+0xbf/0x200 mm/slab.c:3757
	 free_tty_struct+0x8d/0xb0 drivers/tty/tty_io.c:177
	 release_one_tty+0x22d/0x2f0 drivers/tty/tty_io.c:1468
	 process_one_work+0x7f1/0x14b0 kernel/workqueue.c:2264
	 worker_thread+0x8b/0xc80 kernel/workqueue.c:2410
	 [...]

Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
Cc: <stable@vger.kernel.org> # v3.4+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 drivers/tty/vt/vt_ioctl.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index f62f498f63c05..daf61c28ba766 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -43,9 +43,15 @@ bool vt_dont_switch;
 
 static inline bool vt_in_use(unsigned int i)
 {
-	extern struct tty_driver *console_driver;
+	const struct vc_data *vc = vc_cons[i].d;
 
-	return console_driver->ttys[i] && console_driver->ttys[i]->count;
+	/*
+	 * console_lock must be held to prevent the vc from being deallocated
+	 * while we're checking whether it's in-use.
+	 */
+	WARN_CONSOLE_UNLOCKED();
+
+	return vc && kref_read(&vc->port.kref) > 1;
 }
 
 static inline bool vt_busy(int i)
@@ -643,15 +649,16 @@ int vt_ioctl(struct tty_struct *tty,
 		struct vt_stat __user *vtstat = up;
 		unsigned short state, mask;
 
-		/* Review: FIXME: Console lock ? */
 		if (put_user(fg_console + 1, &vtstat->v_active))
 			ret = -EFAULT;
 		else {
 			state = 1;	/* /dev/tty0 is always open */
+			console_lock(); /* required by vt_in_use() */
 			for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
 							++i, mask <<= 1)
 				if (vt_in_use(i))
 					state |= mask;
+			console_unlock();
 			ret = put_user(state, &vtstat->v_state);
 		}
 		break;
@@ -661,10 +668,11 @@ int vt_ioctl(struct tty_struct *tty,
 	 * Returns the first available (non-opened) console.
 	 */
 	case VT_OPENQRY:
-		/* FIXME: locking ? - but then this is a stupid API */
+		console_lock(); /* required by vt_in_use() */
 		for (i = 0; i < MAX_NR_CONSOLES; ++i)
 			if (!vt_in_use(i))
 				break;
+		console_unlock();
 		uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
 		goto setint;		 
 
-- 
2.25.2

Re: [PATCH v3 0/2] vt: fix some vt_ioctl races

[Greg Kroah-Hartman]

On Sat, Mar 21, 2020 at 08:43:03PM -0700, Eric Biggers wrote:
> Fix VT_DISALLOCATE freeing an in-use virtual console, and fix a
> use-after-free in vt_in_use().
> 
> Changed since v2:
>     - Implemented tty_port_operations::destruct().
>     - Added comments regarding vt_in_use() locking.
> 
> 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       | 23 ++++++++++++++++++++++-
>  drivers/tty/vt/vt_ioctl.c | 28 ++++++++++++++++------------
>  2 files changed, 38 insertions(+), 13 deletions(-)


Jiri, any objection to these?

thanks,

greg k-h

Re: [PATCH v3 1/2] vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use virtual console

[Jiri Slaby]

On 22. 03. 20, 4:43, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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 considers a virtual console's
> 'struct vc_data' to be unused as soon as the corresponding tty's
> refcount hits 0.  But actually it may be still being closed.
> 
> Fix this by making vc_data be reference-counted via the embedded
> 'struct tty_port'.  A newly allocated virtual console has refcount 1.
> Opening it for the first time increments the refcount to 2.  Closing it
> for the last time decrements the refcount (in tty_operations::cleanup()
> so that it happens late enough), as does VT_DISALLOCATE.
> 
> 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: <stable@vger.kernel.org> # v3.4+
> Reported-by: syzbot+522643ab5729b0421998@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Jiri Slaby <jslaby@suse.cz>

> ---
>  drivers/tty/vt/vt.c       | 23 ++++++++++++++++++++++-
>  drivers/tty/vt/vt_ioctl.c | 12 ++++--------
>  2 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index bbc26d73209a4..309a39197be0a 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1075,6 +1075,17 @@ static void visual_deinit(struct vc_data *vc)
>  	module_put(vc->vc_sw->owner);
>  }
>  
> +static void vc_port_destruct(struct tty_port *port)
> +{
> +	struct vc_data *vc = container_of(port, struct vc_data, port);
> +
> +	kfree(vc);
> +}
> +
> +static const struct tty_port_operations vc_port_ops = {
> +	.destruct = vc_port_destruct,
> +};
> +
>  int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  {
>  	struct vt_notifier_param param;
> @@ -1100,6 +1111,7 @@ int vc_allocate(unsigned int currcons)	/* return 0 on success */
>  
>  	vc_cons[currcons].d = vc;
>  	tty_port_init(&vc->port);
> +	vc->port.ops = &vc_port_ops;
>  	INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>  
>  	visual_init(vc, currcons, 1);
> @@ -3250,6 +3262,7 @@ static int con_install(struct tty_driver *driver, struct tty_struct *tty)
>  
>  	tty->driver_data = vc;
>  	vc->port.tty = tty;
> +	tty_port_get(&vc->port);
>  
>  	if (!tty->winsize.ws_row && !tty->winsize.ws_col) {
>  		tty->winsize.ws_row = vc_cons[currcons].d->vc_rows;
> @@ -3285,6 +3298,13 @@ static void con_shutdown(struct tty_struct *tty)
>  	console_unlock();
>  }
>  
> +static void con_cleanup(struct tty_struct *tty)
> +{
> +	struct vc_data *vc = tty->driver_data;
> +
> +	tty_port_put(&vc->port);
> +}
> +
>  static int default_color           = 7; /* white */
>  static int default_italic_color    = 2; // green (ASCII)
>  static int default_underline_color = 3; // cyan (ASCII)
> @@ -3410,7 +3430,8 @@ static const struct tty_operations con_ops = {
>  	.throttle = con_throttle,
>  	.unthrottle = con_unthrottle,
>  	.resize = vt_resize,
> -	.shutdown = con_shutdown
> +	.shutdown = con_shutdown,
> +	.cleanup = con_cleanup,
>  };
>  
>  static struct cdev vc0_cdev;
> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
> index 7297997fcf04c..f62f498f63c05 100644
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -310,10 +310,8 @@ static int vt_disallocate(unsigned int vc_num)
>  		vc = vc_deallocate(vc_num);
>  	console_unlock();
>  
> -	if (vc && vc_num >= MIN_NR_CONSOLES) {
> -		tty_port_destroy(&vc->port);
> -		kfree(vc);
> -	}
> +	if (vc && vc_num >= MIN_NR_CONSOLES)
> +		tty_port_put(&vc->port);
>  
>  	return ret;
>  }
> @@ -333,10 +331,8 @@ static void vt_disallocate_all(void)
>  	console_unlock();
>  
>  	for (i = 1; i < MAX_NR_CONSOLES; i++) {
> -		if (vc[i] && i >= MIN_NR_CONSOLES) {
> -			tty_port_destroy(&vc[i]->port);
> -			kfree(vc[i]);
> -		}
> +		if (vc[i] && i >= MIN_NR_CONSOLES)
> +			tty_port_put(&vc[i]->port);
>  	}
>  }
>  
> 


-- 
js
suse labs

Re: [PATCH v3 2/2] vt: vt_ioctl: fix use-after-free in vt_in_use()

[Jiri Slaby]

On 22. 03. 20, 4:43, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> vt_in_use() dereferences console_driver->ttys[i] without proper locking.
> This is broken because the tty can be closed and freed concurrently.
> 
> We could fix this by using 'READ_ONCE(console_driver->ttys[i]) != NULL'
> and skipping the check of tty_struct::count.  But, looking at
> console_driver->ttys[i] isn't really appropriate anyway because even if
> it is NULL the tty can still be in the process of being closed.
> 
> Instead, fix it by making vt_in_use() require console_lock() and check
> whether the vt is allocated and has port refcount > 1.  This works since
> following the patch "vt: vt_ioctl: fix VT_DISALLOCATE freeing in-use
> virtual console" the port refcount is incremented while the vt is open.
> 
> Reproducer (very unreliable, but it worked for me after a few minutes):
> 
> 	#include <fcntl.h>
> 	#include <linux/vt.h>
> 
> 	int main()
> 	{
> 		int fd, nproc;
> 		struct vt_stat state;
> 		char ttyname[16];
> 
> 		fd = open("/dev/tty10", O_RDONLY);
> 		for (nproc = 1; nproc < 8; nproc *= 2)
> 			fork();
> 		for (;;) {
> 			sprintf(ttyname, "/dev/tty%d", rand() % 8);
> 			close(open(ttyname, O_RDONLY));
> 			ioctl(fd, VT_GETSTATE, &state);
> 		}
> 	}
> 
> KASAN report:
> 
> 	BUG: KASAN: use-after-free in vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
> 	BUG: KASAN: use-after-free in vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
> 	Read of size 4 at addr ffff888065722468 by task syz-vt2/132
> 
> 	CPU: 0 PID: 132 Comm: syz-vt2 Not tainted 5.6.0-rc5-00130-g089b6d3654916 #13
> 	Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20191223_100556-anatol 04/01/2014
> 	Call Trace:
> 	 [...]
> 	 vt_in_use drivers/tty/vt/vt_ioctl.c:48 [inline]
> 	 vt_ioctl+0x1ad3/0x1d70 drivers/tty/vt/vt_ioctl.c:657
> 	 tty_ioctl+0x9db/0x11b0 drivers/tty/tty_io.c:2660
> 	 [...]
> 
> 	Allocated by task 136:
> 	 [...]
> 	 kzalloc include/linux/slab.h:669 [inline]
> 	 alloc_tty_struct+0x96/0x8a0 drivers/tty/tty_io.c:2982
> 	 tty_init_dev+0x23/0x350 drivers/tty/tty_io.c:1334
> 	 tty_open_by_driver drivers/tty/tty_io.c:1987 [inline]
> 	 tty_open+0x3ca/0xb30 drivers/tty/tty_io.c:2035
> 	 [...]
> 
> 	Freed by task 41:
> 	 [...]
> 	 kfree+0xbf/0x200 mm/slab.c:3757
> 	 free_tty_struct+0x8d/0xb0 drivers/tty/tty_io.c:177
> 	 release_one_tty+0x22d/0x2f0 drivers/tty/tty_io.c:1468
> 	 process_one_work+0x7f1/0x14b0 kernel/workqueue.c:2264
> 	 worker_thread+0x8b/0xc80 kernel/workqueue.c:2410
> 	 [...]
> 
> Fixes: 4001d7b7fc27 ("vt: push down the tty lock so we can see what is left to tackle")
> Cc: <stable@vger.kernel.org> # v3.4+
> Signed-off-by: Eric Biggers <ebiggers@google.com>

I cannot think of anything better with the current poor code state, so:

Acked-by: Jiri Slaby <jslaby@suse.cz>

thanks,
-- 
-- 
js
suse labs