diff mbox series

vc_screen: reload & check struct vc_data pointer in vcs_read() to avoid UAF

Message ID 1674514099-17696-1-git-send-email-george.kennedy@oracle.com
State New
Headers
Series vc_screen: reload & check struct vc_data pointer in vcs_read() to avoid UAF |

Commit Message

George Kennedy Jan. 23, 2023, 10:48 p.m. UTC 
  After a call to console_unlock() in vcs_read() the vc_data struct can be
freed by vc_deallocate(). Because of that, the struct vc_data pointer needs
to be reloaded and checked for NULL to avoid the UAF below when vcs_size()
is called.

Syzkaller reported a UAF in vcs_size().

BUG: KASAN: use-after-free in vcs_size (drivers/tty/vt/vc_screen.c:215)
Read of size 4 at addr ffff8881137479a8 by task 4a005ed81e27e65/1537

CPU: 0 PID: 1537 Comm: 4a005ed81e27e65 Not tainted 6.2.0-rc5 #1
Hardware name: Red Hat KVM, BIOS 1.15.0-2.module
Call Trace:
 <TASK>
dump_stack_lvl (lib/dump_stack.c:107)
print_report (mm/kasan/report.c:307 mm/kasan/report.c:417)
kasan_report (mm/kasan/report.c:184 mm/kasan/report.c:519)
__asan_report_load4_noabort (mm/kasan/report_generic.c:350)
vcs_size (drivers/tty/vt/vc_screen.c:215)
vcs_read (drivers/tty/vt/vc_screen.c:415)
vfs_read (fs/read_write.c:468 fs/read_write.c:450)
ksys_read (fs/read_write.c:614)
__x64_sys_read (fs/read_write.c:621)
do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
 </TASK>

Allocated by task 1191:
kasan_save_stack (mm/kasan/common.c:46)
kasan_set_track (mm/kasan/common.c:52)
kasan_save_alloc_info (mm/kasan/generic.c:508)
__kasan_kmalloc (mm/kasan/common.c:381)
kmalloc_trace (mm/slab_common.c:1069)
vc_allocate (./include/linux/slab.h:580 ./include/linux/slab.h:720
    drivers/tty/vt/vt.c:1128 drivers/tty/vt/vt.c:1108)
con_install (drivers/tty/vt/vt.c:3383)
tty_init_dev (drivers/tty/tty_io.c:1301 drivers/tty/tty_io.c:1413
    drivers/tty/tty_io.c:1390)
tty_open (drivers/tty/tty_io.c:2080 drivers/tty/tty_io.c:2126)
chrdev_open (fs/char_dev.c:415)
do_dentry_open (fs/open.c:883)
vfs_open (fs/open.c:1014)
path_openat (fs/namei.c:3558 fs/namei.c:3714)
do_filp_open (fs/namei.c:3741)
do_sys_openat2 (fs/open.c:1311)
do_sys_open (fs/open.c:1324)
__x64_sys_openat (fs/open.c:1337)
do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)

Freed by task 1548:
kasan_save_stack (mm/kasan/common.c:46)
kasan_set_track (mm/kasan/common.c:52)
kasan_save_free_info (mm/kasan/generic.c:520)
__kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200
    mm/kasan/common.c:244)
__kmem_cache_free (mm/slub.c:1807 mm/slub.c:3787 mm/slub.c:3800)
kfree (mm/slab_common.c:1021)
vc_port_destruct (drivers/tty/vt/vt.c:1094)
tty_port_destructor (drivers/tty/tty_port.c:296)
tty_port_put (drivers/tty/tty_port.c:312)
vt_disallocate_all (drivers/tty/vt/vt_ioctl.c:662 (discriminator 2))
vt_ioctl (drivers/tty/vt/vt_ioctl.c:903)
tty_ioctl (drivers/tty/tty_io.c:2776)
__x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:870 fs/ioctl.c:856
    fs/ioctl.c:856)
do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)

The buggy address belongs to the object at ffff888113747800
 which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 424 bytes inside of
 1024-byte region [ffff888113747800, ffff888113747c00)

The buggy address belongs to the physical page:
page:00000000b3fe6c7c refcount:1 mapcount:0 mapping:0000000000000000
    index:0x0 pfn:0x113740
head:00000000b3fe6c7c order:3 compound_mapcount:0 subpages_mapcount:0
    compound_pincount:0
anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
raw: 0017ffffc0010200 ffff888100042dc0 0000000000000000 dead000000000001
raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888113747880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888113747900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff888113747980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                  ^
 ffff888113747a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888113747a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
Disabling lock debugging due to kernel taint

Fixes: ac751efa6a0d ("console: rename acquire/release_console_sem() to console_lock/unlock()")
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: George Kennedy <george.kennedy@oracle.com>
---
 drivers/tty/vt/vc_screen.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Jiri Slaby Jan. 24, 2023, 10:07 a.m. UTC | #1 
On 23. 01. 23, 23:48, George Kennedy wrote:
> After a call to console_unlock() in vcs_read() the vc_data struct can be
> freed by vc_deallocate(). Because of that, the struct vc_data pointer needs
> to be reloaded and checked for NULL to avoid the UAF below when vcs_size()
> is called.
> 
> Syzkaller reported a UAF in vcs_size().
> 
> BUG: KASAN: use-after-free in vcs_size (drivers/tty/vt/vc_screen.c:215)
> Read of size 4 at addr ffff8881137479a8 by task 4a005ed81e27e65/1537

Could you trim the stack traces a bit, for example as follows?

> CPU: 0 PID: 1537 Comm: 4a005ed81e27e65 Not tainted 6.2.0-rc5 #1
> Hardware name: Red Hat KVM, BIOS 1.15.0-2.module
> Call Trace:
>   <TASK>
> __asan_report_load4_noabort (mm/kasan/report_generic.c:350)
> vcs_size (drivers/tty/vt/vc_screen.c:215)
> vcs_read (drivers/tty/vt/vc_screen.c:415)
> vfs_read (fs/read_write.c:468 fs/read_write.c:450)
> ...
>   </TASK>
> 
> Allocated by task 1191:
 > ...
 > kmalloc_trace (mm/slab_common.c:1069)
> vc_allocate (./include/linux/slab.h:580 ./include/linux/slab.h:720
>      drivers/tty/vt/vt.c:1128 drivers/tty/vt/vt.c:1108)
> con_install (drivers/tty/vt/vt.c:3383)
> tty_init_dev (drivers/tty/tty_io.c:1301 drivers/tty/tty_io.c:1413
>      drivers/tty/tty_io.c:1390)
> tty_open (drivers/tty/tty_io.c:2080 drivers/tty/tty_io.c:2126)
> chrdev_open (fs/char_dev.c:415)
> do_dentry_open (fs/open.c:883)
> vfs_open (fs/open.c:1014)
> ...
> 
> Freed by task 1548:
 > ...
> kfree (mm/slab_common.c:1021)
> vc_port_destruct (drivers/tty/vt/vt.c:1094)
> tty_port_destructor (drivers/tty/tty_port.c:296)
> tty_port_put (drivers/tty/tty_port.c:312)
> vt_disallocate_all (drivers/tty/vt/vt_ioctl.c:662 (discriminator 2))
> vt_ioctl (drivers/tty/vt/vt_ioctl.c:903)
> tty_ioctl (drivers/tty/tty_io.c:2776)
> ...
> 
> The buggy address belongs to the object at ffff888113747800
>   which belongs to the cache kmalloc-1k of size 1024
> The buggy address is located 424 bytes inside of
>   1024-byte region [ffff888113747800, ffff888113747c00)
> 
> The buggy address belongs to the physical page:
> page:00000000b3fe6c7c refcount:1 mapcount:0 mapping:0000000000000000
>      index:0x0 pfn:0x113740
> head:00000000b3fe6c7c order:3 compound_mapcount:0 subpages_mapcount:0
>      compound_pincount:0
> anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> raw: 0017ffffc0010200 ffff888100042dc0 0000000000000000 dead000000000001
> raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> 
> Memory state around the buggy address:
>   ffff888113747880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>   ffff888113747900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> ffff888113747980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>                                    ^
>   ffff888113747a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>   ffff888113747a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
> Disabling lock debugging due to kernel taint
> 
> Fixes: ac751efa6a0d ("console: rename acquire/release_console_sem() to console_lock/unlock()")
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> ---
>   drivers/tty/vt/vc_screen.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
> index 1850bacdb5b0..efa4f14fc9c0 100644
> --- a/drivers/tty/vt/vc_screen.c
> +++ b/drivers/tty/vt/vc_screen.c
> @@ -403,7 +403,7 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   		poll->event = 0;
>   	read = 0;
>   	ret = 0;
> -	while (count) {
> +	while (vc && count) {
>   		unsigned int this_round, skip = 0;
>   		int size;
>   
> @@ -465,9 +465,17 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   		pos += this_round;
>   		read += this_round;
>   		count -= this_round;
> +
> +		/* Reload if more to do. May have been freed by vc_deallocate()
> +		 * after console_unlock()
> +		 */
> +		if (count)
> +			vc = vcs_vc(inode, &viewed);
>   	}
>   	*ppos += read;
> -	if (read)
> +	if (!vc)
> +		ret = -ENXIO;

Wouldn't it make more sense to simply return from the loop immediately 
(goto unlock_out)? So that you wouldn't need to add vc to the while 
condition and the added 'if (count)' would contain also this 'if (!vc)?

> +	else if (read)
>   		ret = read;
>   unlock_out:
>   	console_unlock();

thanks,
diff mbox series

Patch

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 1850bacdb5b0..efa4f14fc9c0 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -403,7 +403,7 @@  vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		poll->event = 0;
 	read = 0;
 	ret = 0;
-	while (count) {
+	while (vc && count) {
 		unsigned int this_round, skip = 0;
 		int size;
 
@@ -465,9 +465,17 @@  vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		pos += this_round;
 		read += this_round;
 		count -= this_round;
+
+		/* Reload if more to do. May have been freed by vc_deallocate()
+		 * after console_unlock()
+		 */
+		if (count)
+			vc = vcs_vc(inode, &viewed);
 	}
 	*ppos += read;
-	if (read)
+	if (!vc)
+		ret = -ENXIO;
+	else if (read)
 		ret = read;
 unlock_out:
 	console_unlock();