[lvc-project] [PATCH] media: rc: Fix use-after-free when racing vfd_write() with disconnect

[Larshin Sergey]

Syzbot reports a KASAN issue as below:
BUG: KASAN: use-after-free in __create_pipe include/linux/usb.h:1945 [inline]
BUG: KASAN: use-after-free in send_packet+0xa2d/0xbc0 drivers/media/rc/imon.c:627
Read of size 4 at addr ffff8880256fb000 by task syz-executor314/4465

CPU: 2 PID: 4465 Comm: syz-executor314 Not tainted 6.0.0-rc1-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
Call Trace:
 <TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:317 [inline]
print_report.cold+0x2ba/0x6e9 mm/kasan/report.c:433
kasan_report+0xb1/0x1e0 mm/kasan/report.c:495
__create_pipe include/linux/usb.h:1945 [inline]
send_packet+0xa2d/0xbc0 drivers/media/rc/imon.c:627
vfd_write+0x2d9/0x550 drivers/media/rc/imon.c:991
vfs_write+0x2d7/0xdd0 fs/read_write.c:576
ksys_write+0x127/0x250 fs/read_write.c:631
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd

`imon_disconnect()` races with `vfd_write()`, `imon_disconnect()`
may complete the pending transfer and free `ictx`
while `vfd_write()` is blocked in `wait_for_completion_interruptible()`.
When `vfd_write()` wakes up, it accesses freed `ictx`,
causing a use-after-free.

Thread 1 vfd_write()                       Thread 2 imon_disconnect()
...
mutex_lock_interruptible(&ictx->lock)
...
while
  send_packet(ictx)
    ictx->tx.busy = true // UAF
    usb_submit_urb()
    wait_for_completion_interruptible()
    ...
                                          ...
                                          if (ictx->tx.busy)
                                            usb_kill_urb(ictx->tx_urb)
                                            complete(&ictx->tx.finished)

                                          ...
                                          if (refcount_dec_and_test
                                                         (&ictx->users))
                                            free_imon_context(ictx)

    ictx->tx.busy = false // UAF
    retval = ictx->tx.status

Set and read `ictx->disconnected` atomically.
Acquire `ictx->lock` in `imon_disconnect()` after setting
the flag to synchronize with writers holding the lock.
This ensures writers exit safely before `disconnect()`
proceeds with cleanup.

Reported-by: syzbot+f1a69784f6efe748c3bf at syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f1a69784f6efe748c3bf
Signed-off-by: Larshin Sergey <Sergey.Larshin at kaspersky.com>
---
 drivers/media/rc/imon.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index e5590a708f1c..8dcc953689fd 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -526,7 +526,8 @@ static int display_open(struct inode *inode, struct file *file)
 
 	rcu_read_lock();
 	ictx = usb_get_intfdata(interface);
-	if (!ictx || ictx->disconnected || !refcount_inc_not_zero(&ictx->users)) {
+	/* Ensure we see device connect before proceeding */
+	if (!ictx || smp_load_acquire(&ictx->disconnected) || !refcount_inc_not_zero(&ictx->users)) {
 		rcu_read_unlock();
 		pr_err("no context found for minor %d\n", subminor);
 		retval = -ENODEV;
@@ -536,7 +537,8 @@ static int display_open(struct inode *inode, struct file *file)
 
 	mutex_lock(&ictx->lock);
 
-	if (!ictx->display_supported) {
+	/* Ensure we see device connect before proceeding */
+	if (smp_load_acquire(&ictx->disconnected) || !ictx->display_supported) {
 		pr_err("display not supported by device\n");
 		retval = -ENODEV;
 	} else if (ictx->display_isopen) {
@@ -598,6 +600,10 @@ static int send_packet(struct imon_context *ictx)
 	int retval = 0;
 	struct usb_ctrlrequest *control_req = NULL;
 
+	/* Ensure we see device connect before proceeding */
+	if (smp_load_acquire(&ictx->disconnected))
+		return -ENODEV;
+
 	/* Check if we need to use control or interrupt urb */
 	if (!ictx->tx_control) {
 		pipe = usb_sndintpipe(ictx->usbdev_intf0,
@@ -949,7 +955,8 @@ static ssize_t vfd_write(struct file *file, const char __user *buf,
 	static const unsigned char vfd_packet6[] = {
 		0x01, 0x00, 0x00, 0x00, 0x00, 0xFF, 0xFF };
 
-	if (ictx->disconnected)
+	/* Ensure we see device connect before proceeding */
+	if (smp_load_acquire(&ictx->disconnected))
 		return -ENODEV;
 
 	if (mutex_lock_interruptible(&ictx->lock))
@@ -1029,7 +1036,8 @@ static ssize_t lcd_write(struct file *file, const char __user *buf,
 	int retval = 0;
 	struct imon_context *ictx = file->private_data;
 
-	if (ictx->disconnected)
+	/* Ensure we see device connect before proceeding */
+	if (smp_load_acquire(&ictx->disconnected))
 		return -ENODEV;
 
 	mutex_lock(&ictx->lock);
@@ -2499,7 +2507,11 @@ static void imon_disconnect(struct usb_interface *interface)
 	int ifnum;
 
 	ictx = usb_get_intfdata(interface);
-	ictx->disconnected = true;
+	/* Ensure disconnect is visible to readers */
+	smp_store_release(&ictx->disconnected, true);
+	mutex_lock(&ictx->lock);
+	mutex_unlock(&ictx->lock);
+
 	dev = ictx->dev;
 	ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
 
@@ -2513,7 +2525,7 @@ static void imon_disconnect(struct usb_interface *interface)
 	usb_set_intfdata(interface, NULL);
 
 	/* Abort ongoing write */
-	if (ictx->tx.busy) {
+	if (READ_ONCE(ictx->tx.busy)) {
 		usb_kill_urb(ictx->tx_urb);
 		complete(&ictx->tx.finished);
 	}
-- 
2.39.5