[PATCH 1/3] media: dvb-core: fix use-after-free in dvb_remove_device() and dvb_device_open()

[Yuanzhe Liu]

dvb_remove_device() clears dvb_minors[] and drops the minors-table
reference under minor_rwsem, but then keeps using "dvbdev":

	down_write(&minor_rwsem);
	dvb_minors[dvbdev->minor] = NULL;
	dvb_device_put(dvbdev);          /* may kfree(dvbdev) */
	up_write(&minor_rwsem);

	dvb_media_device_free(dvbdev);
	device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor));
	list_del(&dvbdev->list_head);

If the put() frees the object (e.g. USB disconnect racing with an
in-flight open() that still holds a reference of its own, or an open()
error path having already dropped the last open() reference), the
remaining three statements are use-after-free. KASAN reports the read
of dvbdev->minor inside device_destroy(), and the list_del() then
writes list poison into the freed object.

dvb_unregister_device() has the same shape: it calls
dvb_remove_device() and then dvb_device_put(), i.e. it relies on
dvb_remove_device() not consuming the caller's reference.

Fix both by making the reference accounting symmetric:

- dvb_remove_device() no longer touches the kref at all. It removes
  the minors entry, frees the media resources, destroys the class
  device and unlinks the object from the adapter list. Everything it
  needs from "dvbdev" is done while the object is still alive; the
  caller still owns its reference.
- dvb_unregister_device() now drops both references the device
  actually holds: the minors-table one and the caller's initial one.
  All in-tree callers that want the object gone already call
  dvb_unregister_device(), so the extra put() restores the previous
  net refcount balance while keeping the object alive until after the
  last use.

While at it, close the dvb_device_open() race that supplied the stale
pointer in the first place: it fetched dvb_minors[minor] under
down_read(&minor_rwsem), but only called dvb_device_get() after a
sequence that a concurrent dvb_remove_device() could preempt to free
the object. KASAN caught this as a read of ->fops on a freed
kmalloc-128 object, together with "refcount_t: addition on 0"
warnings on the same path.

Do the whole lookup + pin atomically under minor_rwsem: take the kref
while still holding the read lock (which excludes the only writer,
dvb_remove_device()), and only then call file->f_op->open().

Cc: stable@xxxxxxxxxxxxxxx
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Yuanzhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
 drivers/media/dvb-core/dvbdev.c | 58 +++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d32..9eb8862 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -87,6 +87,8 @@ static int dvb_device_open(struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev;
 	unsigned int minor = iminor(inode);
+	const struct file_operations *new_fops;
+	int err = 0;
 
 	if (minor >= MAX_DVB_MINORS)
 		return -ENODEV;
@@ -94,25 +96,33 @@ static int dvb_device_open(struct inode *inode, struct file *file)
 	mutex_lock(&dvbdev_mutex);
 	down_read(&minor_rwsem);
 
+	/*
+	 * Look the device up and pin it while still holding minor_rwsem.
+	 * dvb_remove_device() takes the write side to clear the entry, so
+	 * once the kref has been taken the object cannot be freed until
+	 * we drop it again.
+	 */
 	dvbdev = dvb_minors[minor];
 
-	if (dvbdev && dvbdev->fops) {
-		int err = 0;
-		const struct file_operations *new_fops;
-
-		new_fops = fops_get(dvbdev->fops);
-		if (!new_fops)
-			goto fail;
-		file->private_data = dvb_device_get(dvbdev);
-		replace_fops(file, new_fops);
-		if (file->f_op->open)
-			err = file->f_op->open(inode, file);
-		up_read(&minor_rwsem);
-		mutex_unlock(&dvbdev_mutex);
-		if (err)
-			dvb_device_put(dvbdev);
-		return err;
+	if (!dvbdev || !dvbdev->fops)
+		goto fail;
+
+	/* Pin the device before dropping minor_rwsem. */
+	file->private_data = dvb_device_get(dvbdev);
+	new_fops = fops_get(dvbdev->fops);
+	up_read(&minor_rwsem);
+	mutex_unlock(&dvbdev_mutex);
+
+	if (!new_fops) {
+		dvb_device_put(dvbdev);
+		return -ENODEV;
 	}
+	replace_fops(file, new_fops);
+	if (file->f_op->open)
+		err = file->f_op->open(inode, file);
+	if (err)
+		dvb_device_put(dvbdev);
+	return err;
 fail:
 	up_read(&minor_rwsem);
 	mutex_unlock(&dvbdev_mutex);
@@ -596,9 +606,16 @@ void dvb_remove_device(struct dvb_device *dvbdev)
 	if (!dvbdev)
 		return;
 
+	/*
+	 * Stop new opens first, then tear down everything that still
+	 * needs the object.  We must not drop the kref here: our caller
+	 * still owns a reference and keeps using the object after this
+	 * function returns.  The minors-table reference is dropped in
+	 * dvb_unregister_device() instead, pairing the dvb_device_get()
+	 * that installed the entry in dvb_register_device().
+	 */
 	down_write(&minor_rwsem);
 	dvb_minors[dvbdev->minor] = NULL;
-	dvb_device_put(dvbdev);
 	up_write(&minor_rwsem);
 
 	dvb_media_device_free(dvbdev);
@@ -632,6 +649,13 @@ void dvb_device_put(struct dvb_device *dvbdev)
 void dvb_unregister_device(struct dvb_device *dvbdev)
 {
 	dvb_remove_device(dvbdev);
+
+	/*
+	 * Drop both the minors-table reference and the caller's own one.
+	 * The object is only freed once every open() fd has dropped its
+	 * reference too.
+	 */
+	dvb_device_put(dvbdev);
 	dvb_device_put(dvbdev);
 }
 EXPORT_SYMBOL(dvb_unregister_device);
-- 
2.45.1.windows.1