Re: [PATCH v3] media: dvb-core: fix use-after-free in dvb_frontend_open

[Guikun Yang]

In-Reply-To: <1c0aa133-b44b-4cca-91cd-82a80769b494@xxxxxxxxxxxxx>
References: <1c0aa133-b44b-4cca-91cd-82a80769b494@xxxxxxxxxxxxx>

Hi Yun, all,

Continuing this thread: the v3 series (and the related "avoid dropping
device ref" / "pin frontend device" attempts from June) has not been
merged as of v7.3-rc1 -- I confirmed dvb_frontend_open() still calls
dvb_generic_release() on its err2 path in mainline today, and the
underlying bug is still live (syzbot hit it again on 2026/08).

I'd like to propose an alternative fix that I believe is more robust,
with fault-injection reproduction on 7.2-rc7 / 7.3-rc1.

Root cause
----------
dvb_device_open() takes one device reference for the file and drops it
itself when ->open() fails (a2dd235df435).  dvb_frontend_open() undoes
the users/readers/writers accounting of dvb_generic_open() on its error
paths by calling dvb_generic_release().  But dvb_generic_release() also
drops the file's device reference, so on a frontend open failure the
same reference is dropped twice:

  dvb_device_open()                dvb_frontend_open()
  ------------------------------   ---------------------------------
  dvb_device_get()          ref+1
  file->f_op->open()
                                   dvb_generic_open()
                                   err3: dvb_frontend_start() fails
                                         (e.g. -ENOMEM)
                                   err2: dvb_generic_release()  ref-1
  err != 0
  dvb_device_put()          ref-1

The extra put can drop the last reference and free the device while it
is still registered in dvb_minors[], so a subsequent open() dereferences
freed memory (KASAN: slab-use-after-free in dvb_device_open, syzbot
extid 1eb177ecc3943b883f0a, first reported 2024/12).

Why a different approach
------------------------
Previous proposals (v1-v3 in this thread, plus the June patches)
balance the counters around dvb_generic_release() -- e.g. incrementing
the refcount before the call so the put inside it nets out, or restoring
users/writers/readers by hand.  That works, but it keeps the subtle
coupling: dvb_generic_release() still has the side effect of dropping
the device reference, which is only correct on a successful open.

This patch instead splits that single responsibility out: the counter
restore becomes dvb_generic_open_undo(), which does NOT touch the
device reference, and dvb_frontend_open() uses it on its error paths.
The reference remains owned and released by dvb_device_open().  This
makes the ownership explicit and removes the class of double-put bugs
for every future ->open implementation, not just the frontend.

Verification
------------
syzbot reproducer (open of /dev/dvb/adapter0/frontend0 with fail_nth: 8,
failing the kthread allocation in dvb_frontend_start()):
  * unpatched 7.2-rc7: reproduces the KASAN UAF within seconds
  * patched: survives 90s of continuous fault-injected opens, zero
    KASAN / refcount warnings; FAULT_INJECTION traces confirm the error
    path is exercised each time

Fixes: 0fc044b2b5e2 ("media: dvbdev: adopts refcnt to avoid UAF")
Cc: stable@xxxxxxxxxxxxxxx
Reported-by: syzbot+1eb177ecc3943b883f0a@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=1eb177ecc3943b883f0a
Tested-by: Guikun Yang <2855020996@xxxxxx>
Signed-off-by: Guikun Yang <2855020996@xxxxxx>
---
 drivers/media/dvb-core/dvb_frontend.c |  6 +++++-
 drivers/media/dvb-core/dvbdev.c       | 29 +++++++++++++++++++++++++++++
 include/media/dvbdev.h                | 11 +++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d082b6c57c76..5ef7756c3413 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2887,7 +2887,11 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 	mutex_unlock(&fe->dvb->mdev_lock);
 err2:
 #endif
-	dvb_generic_release(inode, file);
+	/*
+	 * Undo dvb_generic_open() only: the file's device reference is
+	 * dropped by dvb_device_open() when this open fails.
+	 */
+	dvb_generic_open_undo(file);
 err1:
 	if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
 		fe->ops.ts_bus_ctrl(fe, 0);
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..8623df718ea0 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -172,6 +172,35 @@ int dvb_generic_release(struct inode *inode, struct file *file)
 }
 EXPORT_SYMBOL(dvb_generic_release);
 
+/**
+ * dvb_generic_open_undo - undo dvb_generic_open() on a failed ->open
+ *
+ * @file: pointer to &struct file.
+ *
+ * Restores the users/readers/writers accounting done by
+ * dvb_generic_open(), without dropping the file's device reference:
+ * that reference is taken by dvb_device_open(), which also releases
+ * it when ->open fails.  Using dvb_generic_release() on an error path
+ * of an ->open implementation would therefore drop the reference
+ * twice and could free a device that is still registered in
+ * dvb_minors[].
+ */
+void dvb_generic_open_undo(struct file *file)
+{
+	struct dvb_device *dvbdev = file->private_data;
+
+	if (!dvbdev)
+		return;
+
+	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
+		dvbdev->readers++;
+	else
+		dvbdev->writers++;
+
+	dvbdev->users++;
+}
+EXPORT_SYMBOL(dvb_generic_open_undo);
+
 long dvb_generic_ioctl(struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index e5a00d126612..a32998afd804 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -354,6 +354,17 @@ int dvb_generic_open(struct inode *inode, struct file *file);
  */
 int dvb_generic_release(struct inode *inode, struct file *file);
 
+/**
+ * dvb_generic_open_undo - undo dvb_generic_open() on a failed ->open
+ *
+ * @file: pointer to &struct file.
+ *
+ * Restores the users/readers/writers accounting done by
+ * dvb_generic_open() without dropping the file's device reference,
+ * which dvb_device_open() releases itself when ->open fails.
+ */
+void dvb_generic_open_undo(struct file *file);
+
 /**
  * dvb_generic_ioctl - Digital TV close function, used by DVB devices
  *
-- 
2.0.0