[PATCH] video: fbdev: smscufx: Fix use-after-free in ufx_ops_open()

[Hyunwoo Kim <imv4bel@gmail.com>]

A race condition may occur if the user physically removes the
USB device while calling open() for this device node.

This is a race condition between the ufx_ops_open() function and
the ufx_usb_disconnect() function, which may eventually result in UAF.

So, add a mutex to the ufx_ops_open() and ufx_usb_disconnect() functions
to avoid race contidion of krefs.

Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 drivers/video/fbdev/smscufx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index d7aa5511c361..a4378a7241f7 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -137,6 +137,8 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len);
 static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size);
 static void ufx_free_urb_list(struct ufx_data *dev);
 
+static DEFINE_MUTEX(disconnect_mutex);
+
 /* reads a control register */
 static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data)
 {
@@ -1065,6 +1067,8 @@ static int ufx_ops_open(struct fb_info *info, int user)
 {
 	struct ufx_data *dev = info->par;
 
+	mutex_lock(&disconnect_mutex);
+
 	/* fbcon aggressively connects to first framebuffer it finds,
 	 * preventing other clients (X) from working properly. Usually
 	 * not what the user wants. Fail by default with option to enable. */
@@ -1097,6 +1101,8 @@ static int ufx_ops_open(struct fb_info *info, int user)
 	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d",
 		info->node, user, info, dev->fb_count);
 
+	mutex_unlock(&disconnect_mutex);
+
 	return 0;
 }
 
@@ -1741,6 +1747,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
 {
 	struct ufx_data *dev;
 
+	mutex_lock(&disconnect_mutex);
+
 	dev = usb_get_intfdata(interface);
 
 	pr_debug("USB disconnect starting\n");
@@ -1761,6 +1769,8 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
 	kref_put(&dev->kref, ufx_free);
 
 	/* consider ufx_data freed */
+
+	mutex_unlock(&disconnect_mutex);
 }
 
 static struct usb_driver ufx_driver = {
-- 
2.25.1


Dear all,


I found a race-condition-to-UAF vulnerability in "drivers/video/fbdev/smscufx.c".


# Introduction

This vulnerability is a race condition between the ufx_ops_open() and ufx_usb_disconnect() functions.

Because this driver uses kref to manage the "dev" structure, it looks like a race condition will not occur, 
but in reality, a race condition occurs because there is no lock between the above two functions:
```
                cpu0                                                cpu1
       1. open()
          ufx_ops_open()
          if (dev->virtualized)
                                                             2. ufx_usb_disconnect()
                                                                dev->virtualized = true;
                                                                atomic_set()
                                                                usb_set_intfdata()

                                                             3. if (dev->fb_count == 0)
                                                                schedule_delayed_work(&dev->free_framebuffer_work, 0)
                                                                kref_put()   <- kref count : 1
                                                                kref_put()   <- kref count : 0
                                                                ufx_free()
                                                                kfree(dev);

       4. dev->fb_count++;   <- UAF start
          kref_get()   <- refcount_t: addition on 0; use-after-free.
```
The detailed exploit flow is as follows:

1. open() the device node.
dev->virtualized set to "true" only in ufx_usb_disconnect(),
It passes "if (dev->virtualized)" because it hasn't been set yet.

2. ufx_usb_disconnect() is called by physically removing the usb device.
"dev->virtualized = true;" is executed. 
But checking this flag doesn't make any sense because step 1 has already passed.
1 ~ 2 are the first race conditions.

3. After "if (dev->fb_count == 0)" is passed, call kref_put() twice with delayed queue. 
Eventually "kfree(dev);" is executed.

4. "dev->fb_count++" is called. 
However, at this point "dev" is kfree()d, so it becomes a UAF.
3 ~ 4 are the second race conditions.
After this, many UAF scenarios can be configured.


However, if you actually check the code, the workload of 1 ~ 4 of open() is much shorter 
than the workload of 2 ~ 3 of disconnect().
As a result, it is almost impossible to trigger this race condition with a brute force.

Here, an attacker can successfully achieve a race condition using the technique introduced in this paper:
https://www.usenix.org/system/files/sec21-lee-yoochan.pdf

To briefly summarize the paper, 
the processing time between 1 and 4 of open() can be extended longer by using Reschedule IPI.
In other words, it can dramatically increase the success probability of the above race condition scenario.


# for debugging

To debug this race condition, it is convenient to modify the code as follows:
```
diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index d7aa5511c361..bbb42e9e768d 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -137,6 +137,10 @@ static int ufx_submit_urb(struct ufx_data *dev, struct urb * urb, size_t len);
 static int ufx_alloc_urb_list(struct ufx_data *dev, int count, size_t size);
 static void ufx_free_urb_list(struct ufx_data *dev);

+DECLARE_WAIT_QUEUE_HEAD(race_condition_debugq);
+static int race_check = 0;
+static struct ufx_data *open_dev;
+
 /* reads a control register */
 static int ufx_reg_read(struct ufx_data *dev, u32 index, u32 *data)
 {
@@ -1075,6 +1079,9 @@ static int ufx_ops_open(struct fb_info *info, int user)
        if (dev->virtualized)
                return -ENODEV;

+       open_dev = dev;
+       wait_event(race_condition_debugq, (race_check == 1));
+
        dev->fb_count++;

        kref_get(&dev->kref);
@@ -1760,6 +1767,12 @@ static void ufx_usb_disconnect(struct usb_interface *interface)
        /* release reference taken by kref_init in probe() */
        kref_put(&dev->kref, ufx_free);

+       if (open_dev == dev) {
+                usleep_range(1000000, 1000001);
+                race_check = 1;
+                wake_up(&race_condition_debugq);
+        }
+
        /* consider ufx_data freed */
 }
```


test code:
```
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <stdbool.h>
#include <unistd.h>
#include <string.h>
#include <fcntl.h>
#include <pthread.h>
#include <errno.h>
#include <sched.h>
#include <malloc.h>
#include <poll.h>
#include <pty.h>
#include <sys/syscall.h>
#include <sys/ioctl.h>
#include <sys/wait.h>
#include <sys/mman.h>
#include <sys/socket.h>
#include <sys/ipc.h>
#include <linux/userfaultfd.h>
#include <sys/time.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/un.h>
#include <linux/netlink.h>
#include <stddef.h>
#include <sys/param.h>
#include <sys/resource.h>
#include <linux/bpf.h>
#include <linux/ioctl.h>
#include <linux/types.h>


#define die() do { \
    fprintf(stderr, "died in %s: %u\n", __func__, __LINE__); \
    exit(EXIT_FAILURE); \
} while (0)


void *fb_open(void) {
	int fd;

	fd = open("/dev/fb1", O_RDONLY);
	if (fd > 0) {
		printf("[step 1] fb1 open() : %d  pid : %ld\n", fd, syscall(SYS_gettid));
	} else {
		perror("/dev/fb1 open() failed");
		die();
	}

	sleep(10);
}

void *usb_disconnect_for_debug(void) {
	int ret;
	char input[2];

	sleep(3);

	printf("Disconnect now (After disconnecting, type enter)\n");
	read(0, input, 1);
	printf("[step 2] disconnect fbdev usb\n");

	sleep(5);
}

int main() {
	int p1, p2;
	int status1, status2;
	pthread_t hdr1, hdr2;
	int ret;

	p1 = pthread_create(&hdr1, NULL, fb_open, (void *)NULL);
	if (p1 != 0) {
		perror("pthread_create 1");
		die();
	}

	p2 = pthread_create(&hdr2, NULL, usb_disconnect_for_debug, (void *)NULL);
	if (p2 != 0) {
		perror("pthread_create 2");
		die();
	}

	pthread_join(hdr1, (void **)&status1);
	pthread_join(hdr2, (void **)&status2);

	return 0;
}
```


KASAN log:
```
[  138.037981] BUG: KASAN: use-after-free in ufx_ops_open+0x3e2/0x4d0 [smscufx]
[  138.038010] Read of size 4 at addr ffff88812d6a305c by task smscufx/2763

[  138.038035] CPU: 12 PID: 2763 Comm: smscufx Tainted: G        W          6.0.0-rc6+ #1
[  138.038053] Hardware name: Gigabyte Technology Co., Ltd. B460MDS3H/B460M DS3H, BIOS F3 05/27/2020
[  138.038064] Call Trace:
[  138.038073]  <TASK>
[  138.038085]  dump_stack_lvl+0x49/0x63
[  138.038108]  print_report.cold+0x5e/0x5d9
[  138.038123]  ? __kasan_check_write+0x14/0x20
[  138.038146]  ? ufx_ops_open+0x3e2/0x4d0 [smscufx]
[  138.038165]  kasan_report+0xa0/0x120
[  138.038184]  ? ufx_ops_open+0x3e2/0x4d0 [smscufx]
[  138.038203]  __asan_report_load4_noabort+0x14/0x20
[  138.038221]  ufx_ops_open+0x3e2/0x4d0 [smscufx]
[  138.038238]  ? mutex_unlock+0x81/0xd0
[  138.038258]  ? ufx_ops_ioctl+0x3c0/0x3c0 [smscufx]
[  138.038277]  ? destroy_sched_domains_rcu+0x70/0x70
[  138.038300]  ? try_module_get.part.0+0xb8/0x1a0
[  138.038317]  fb_open+0x187/0x3b0
[  138.038335]  chrdev_open+0x230/0x6d0
[  138.038353]  ? cdev_device_add+0x1f0/0x1f0
[  138.038368]  ? fsnotify_perm.part.0+0x1d9/0x4e0
[  138.038390]  do_dentry_open+0x404/0xf80
[  138.038408]  ? inode_permission+0x125/0x560
[  138.038422]  ? cdev_device_add+0x1f0/0x1f0
[  138.038440]  vfs_open+0x9f/0xd0
[  138.038457]  path_openat+0xd58/0x3f60
[  138.038474]  ? kasan_save_stack+0x3a/0x50
[  138.038489]  ? kasan_save_stack+0x26/0x50
[  138.038503]  ? __kasan_slab_alloc+0x94/0xd0
[  138.038519]  ? getname_flags.part.0+0x52/0x490
[  138.038534]  ? getname+0x7a/0xb0
[  138.038550]  ? path_lookupat+0x660/0x660
[  138.038565]  ? get_partial_node.part.0+0xd2/0x330
[  138.038586]  do_filp_open+0x1b1/0x3e0
[  138.038600]  ? ___slab_alloc+0x52c/0xa50
[  138.038615]  ? may_open_dev+0xd0/0xd0
[  138.038635]  ? _raw_spin_lock_bh+0xe0/0xe0
[  138.038648]  ? __check_object_size+0x174/0x650
[  138.038671]  do_sys_openat2+0x132/0x450
[  138.038688]  ? _raw_spin_unlock_irq+0x1f/0x3b
[  138.038702]  ? build_open_flags+0x450/0x450
[  138.038719]  ? __ia32_sys_ssetmask+0x1d0/0x1d0
[  138.038735]  ? __kasan_check_write+0x14/0x20
[  138.038756]  __x64_sys_openat+0x128/0x210
[  138.038774]  ? __ia32_compat_sys_open+0x1b0/0x1b0
[  138.038792]  ? fpregs_assert_state_consistent+0x52/0xc0
[  138.038813]  ? exit_to_user_mode_prepare+0x49/0x1a0
[  138.038834]  do_syscall_64+0x59/0x90
[  138.038849]  ? fpregs_assert_state_consistent+0x52/0xc0
[  138.038866]  ? exit_to_user_mode_prepare+0x49/0x1a0
[  138.038885]  ? syscall_exit_to_user_mode+0x26/0x50
[  138.038902]  ? do_syscall_64+0x69/0x90
[  138.038915]  ? switch_fpu_return+0xe/0x20
[  138.038932]  ? exit_to_user_mode_prepare+0x16a/0x1a0
[  138.038951]  ? syscall_exit_to_user_mode+0x26/0x50
[  138.038967]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[  138.038981] RIP: 0033:0x4534a4
[  138.038998] Code: 24 20 eb 8f 66 90 44 89 54 24 0c e8 d6 ab 02 00 44 8b 54 24 0c 44 89 e2 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 34 44 89 c7 89 44 24 0c e8 18 ac 02 00 8b 44
[  138.039014] RSP: 002b:00007f3618d20140 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
[  138.039034] RAX: ffffffffffffffda RBX: 00007f3618d20640 RCX: 00000000004534a4
[  138.039048] RDX: 0000000000000000 RSI: 00000000004b1008 RDI: 00000000ffffff9c
[  138.039060] RBP: 00000000004b1008 R08: 0000000000000000 R09: 00007ffdf3d4a94f
[  138.039071] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000000
[  138.039082] R13: 0000000000000000 R14: 000000000041b260 R15: 00007f3618520000
[  138.039099]  </TASK>
```


Best Regards,
Hyunwoo Kim.