[PATCH 0/2] media: dvbdev: Prevent a dangling pointer in dvb_minors

[PATCH 0/2] media: dvbdev: Prevent a dangling pointer in dvb_minors

[Petr Pavlu]

Prevent a dangling pointer in dvb_minors and simplify error paths in
dvb_register_device().

The main patch attempts to address a syzbot report in try_module_get() [1].
The report provides limited information so I'm not entirely sure if the
patch resolves the specific issue. Nonetheless, the bug fixed in
dvb_register_device() is valid and can be reproduced by manually injecting
errors into the function.

Note that I don't have any DVB device available. I tested the code only by
writing a dummy module that calls dvb_register_adapter() and
dvb_register_device(), and by injecting errors into the latter function.

[1] https://syzkaller.appspot.com/bug?extid=71d5d861d9adc6905054

Petr Pavlu (2):
  media: dvbdev: Prevent a dangling pointer in dvb_minors
  media: dvbdev: Simplify error paths in dvb_register_device()

 drivers/media/dvb-core/dvbdev.c | 83 ++++++++++++++-------------------
 1 file changed, 35 insertions(+), 48 deletions(-)


base-commit: cee73b1e840c154f64ace682cb477c1ae2e29cc4
-- 
2.52.0

[PATCH 1/2] media: dvbdev: Prevent a dangling pointer in dvb_minors

[Petr Pavlu]

Syzbot reports a slab-out-of-bounds write in try_module_get():

==================================================================
BUG: KASAN: slab-out-of-bounds in instrument_atomic_read_write include/linux/instrumented.h:96 [inline]
BUG: KASAN: slab-out-of-bounds in atomic_inc_not_zero include/linux/atomic/atomic-instrumented.h:1536 [inline]
BUG: KASAN: slab-out-of-bounds in try_module_get+0x46/0xc0 kernel/module/main.c:913
Write of size 4 at addr ffff888142f67108 by task syz.3.2706/18556

CPU: 1 UID: 0 PID: 18556 Comm: syz.3.2706 Tainted: G             L      syzkaller #0 PREEMPT(full)-
Tainted: [L]=SOFTLOCKUP
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/24/2026
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0xba/0x230 mm/kasan/report.c:482
 kasan_report+0x117/0x150 mm/kasan/report.c:595
 check_region_inline mm/kasan/generic.c:-1 [inline]
 kasan_check_range+0x264/0x2c0 mm/kasan/generic.c:200
 instrument_atomic_read_write include/linux/instrumented.h:96 [inline]
 atomic_inc_not_zero include/linux/atomic/atomic-instrumented.h:1536 [inline]
 try_module_get+0x46/0xc0 kernel/module/main.c:913
 dvb_device_open+0xef/0x350 drivers/media/dvb-core/dvbdev.c:103
 chrdev_open+0x4cd/0x5e0 fs/char_dev.c:414
 do_dentry_open+0x7ce/0x1420 fs/open.c:962
 vfs_open+0x3b/0x340 fs/open.c:1094
 do_open fs/namei.c:4637 [inline]
 path_openat+0x3486/0x3e20 fs/namei.c:4796
 do_filp_open+0x22d/0x490 fs/namei.c:4823
 do_sys_openat2+0x12f/0x220 fs/open.c:1430
 do_sys_open fs/open.c:1436 [inline]
 __do_sys_openat fs/open.c:1452 [inline]
 __se_sys_openat fs/open.c:1447 [inline]
 __x64_sys_openat+0x138/0x170 fs/open.c:1447
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xe2/0xf80 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7feefad5b78e
Code: 08 0f 85 a5 a8 ff ff 49 89 fb 48 89 f0 48 89 d7 48 89 ce 4c 89 c2 4d 89 ca 4c 8b 44 24 08 4c 8b 4c 24 10 4c 89 5c 24 08 0f 05 <c3> 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 80 00 00 00 00 48 83 ec 08
RSP: 002b:00007feefbc79b28 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 00007feefbc7a6c0 RCX: 00007feefad5b78e
RDX: 0000000000000002 RSI: 00007feefbc79c00 RDI: ffffffffffffff9c
RBP: 00007feefbc79c00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: cccccccccccccccd
R13: 00007feefb016128 R14: 00007feefb016090 R15: 00007ffc341cef58
 </TASK>
==================================================================

The dvb_device_open() function performs 'dvbdev = dvb_minors[minor]' and
then calls fops_get(dvbdev->fops). The code in fops_get() invokes
try_module_get(fops->owner) but this operation fails because the module is
apparently gone.

The issue seems to stem from dvb_register_device() which assigns
dvb_minors[minor] and then continues with the device creation. However, if
a subsequent step fails, the value in dvb_minors[minor] is not cleared,
leaving a dangling dvbdev pointer in the array.

Fix the issue by extending the duration for which minor_rwsem is held in
dvb_register_device() and assign dvb_minors[minor] only after the device
creation is fully completed. This is similar to the approach taken in
usb_register_dev(), where usb_minors and its minor_rwsem are handled.

Reported-by: syzbot+e993e01b15c8eefd9cd4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=71d5d861d9adc6905054
Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 drivers/media/dvb-core/dvbdev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 8b980d371a45..c067bbeb9461 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -548,8 +548,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	}
 
 	dvbdev->minor = minor;
-	dvb_minors[minor] = dvb_device_get(dvbdev);
-	up_write(&minor_rwsem);
+
 	ret = dvb_register_media_device(dvbdev, type, minor, demux_sink_pads);
 	if (ret) {
 		pr_err("%s: dvb_register_media_device failed to create the mediagraph\n",
@@ -563,6 +562,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 		list_del(&dvbdev->list_head);
 		kfree(dvbdev);
 		*pdvbdev = NULL;
+		up_write(&minor_rwsem);
 		mutex_unlock(&dvbdev_register_lock);
 		return ret;
 	}
@@ -582,10 +582,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 		list_del(&dvbdev->list_head);
 		kfree(dvbdev);
 		*pdvbdev = NULL;
+		up_write(&minor_rwsem);
 		mutex_unlock(&dvbdev_register_lock);
 		return PTR_ERR(clsdev);
 	}
 
+	dvb_minors[minor] = dvb_device_get(dvbdev);
+	up_write(&minor_rwsem);
+
 	dprintk("DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
 		adap->num, dnames[type], id, minor, minor);
 
-- 
2.52.0

[PATCH 2/2] media: dvbdev: Simplify error paths in dvb_register_device()

[Petr Pavlu]

The error paths in dvb_register_device() each handle necessary cleanup
operations individually. Since the function performs a number of
operations, this leads to duplicated and lengthy code.

Unify the cleanup process under error labels.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 drivers/media/dvb-core/dvbdev.c | 79 +++++++++++++--------------------
 1 file changed, 31 insertions(+), 48 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index c067bbeb9461..94591f8a7afa 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -466,16 +466,15 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 
 	id = dvbdev_get_free_id(adap, type);
 	if (id < 0) {
-		mutex_unlock(&dvbdev_register_lock);
-		*pdvbdev = NULL;
 		pr_err("%s: couldn't find free device id\n", __func__);
-		return -ENFILE;
+		ret = -ENFILE;
+		goto error_set_pdvbdev;
 	}
 
 	*pdvbdev = dvbdev = kzalloc(sizeof(*dvbdev), GFP_KERNEL);
 	if (!dvbdev) {
-		mutex_unlock(&dvbdev_register_lock);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto error_unlock;
 	}
 
 	/*
@@ -494,19 +493,15 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	if (!dvbdevfops) {
 		dvbdevfops = kmemdup(template->fops, sizeof(*dvbdevfops), GFP_KERNEL);
 		if (!dvbdevfops) {
-			kfree(dvbdev);
-			*pdvbdev = NULL;
-			mutex_unlock(&dvbdev_register_lock);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error_free_dvbdev;
 		}
 
 		new_node = kzalloc(sizeof(*new_node), GFP_KERNEL);
 		if (!new_node) {
 			kfree(dvbdevfops);
-			kfree(dvbdev);
-			*pdvbdev = NULL;
-			mutex_unlock(&dvbdev_register_lock);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error_free_dvbdev;
 		}
 
 		new_node->fops = dvbdevfops;
@@ -534,17 +529,8 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	minor = nums2minor(adap->num, type, id);
 #endif
 	if (minor >= MAX_DVB_MINORS) {
-		if (new_node) {
-			list_del(&new_node->list_head);
-			kfree(dvbdevfops);
-			kfree(new_node);
-		}
-		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
-		*pdvbdev = NULL;
-		up_write(&minor_rwsem);
-		mutex_unlock(&dvbdev_register_lock);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto error_dvbdev_cleanup;
 	}
 
 	dvbdev->minor = minor;
@@ -553,18 +539,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	if (ret) {
 		pr_err("%s: dvb_register_media_device failed to create the mediagraph\n",
 		       __func__);
-		if (new_node) {
-			list_del(&new_node->list_head);
-			kfree(dvbdevfops);
-			kfree(new_node);
-		}
-		dvb_media_device_free(dvbdev);
-		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
-		*pdvbdev = NULL;
-		up_write(&minor_rwsem);
-		mutex_unlock(&dvbdev_register_lock);
-		return ret;
+		goto error_free_media;
 	}
 
 	clsdev = device_create(dvb_class, adap->device,
@@ -573,18 +548,8 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	if (IS_ERR(clsdev)) {
 		pr_err("%s: failed to create device dvb%d.%s%d (%pe)\n",
 		       __func__, adap->num, dnames[type], id, clsdev);
-		if (new_node) {
-			list_del(&new_node->list_head);
-			kfree(dvbdevfops);
-			kfree(new_node);
-		}
-		dvb_media_device_free(dvbdev);
-		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
-		*pdvbdev = NULL;
-		up_write(&minor_rwsem);
-		mutex_unlock(&dvbdev_register_lock);
-		return PTR_ERR(clsdev);
+		ret = PTR_ERR(clsdev);
+		goto error_free_media;
 	}
 
 	dvb_minors[minor] = dvb_device_get(dvbdev);
@@ -595,6 +560,24 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 
 	mutex_unlock(&dvbdev_register_lock);
 	return 0;
+
+error_free_media:
+	dvb_media_device_free(dvbdev);
+error_dvbdev_cleanup:
+	if (new_node) {
+		list_del(&new_node->list_head);
+		kfree(dvbdevfops);
+		kfree(new_node);
+	}
+	list_del(&dvbdev->list_head);
+	up_write(&minor_rwsem);
+error_free_dvbdev:
+	kfree(dvbdev);
+error_set_pdvbdev:
+	*pdvbdev = NULL;
+error_unlock:
+	mutex_unlock(&dvbdev_register_lock);
+	return ret;
 }
 EXPORT_SYMBOL(dvb_register_device);
 
-- 
2.52.0