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

[YuanZhe Liu]

dvb_remove_device() clears dvb_minors[] and drops the minors-table
reference while holding 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() happens to drop the last 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.

The reference accounting itself is fine: dvb_unregister_device()
pairs the put() with its own one for the caller, and direct callers
such as dvb_unregister_frontend() and dvb_ca_en50221_release() rely
on dvb_remove_device() consuming the minors-table reference only.
The problem is the ordering, so move the put() after the last use of
the object. The entry is already gone from dvb_minors[], hence no
concurrent open() can acquire a new reference in between.

dvb_register_device() has a related problem in the other direction.
It installs the new device in dvb_minors[] before calling
dvb_register_media_device() and device_create(), and when one of them
fails it frees the file operations with kfree() while
dvb_device_open() may just have looked the entry up.  Clearing the
array entry from the error path narrows the window but does not close
it: dvb_device_open() releases minor_rwsem only after it has fetched
dvbdev->fops, installed it in file->f_op and called into it.

Publish the file operations only after every step that can fail has
completed.  dvb_minors[minor] is still filled in before
dvb_register_media_device() so that the minor stays reserved and
concurrent open() calls reach a stable decision, but dvbdev->fops
remains NULL until device_create() has worked, and dvb_device_open()
turns such an entry into -ENODEV without taking a reference.  The
error paths can then release the object and its file operations with
no concurrent user in sight.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Bin Yu <byu@xxxxxxxxxxxxx>
Signed-off-by: Bin Yu <byu@xxxxxxxxxxxxx>
Co-developed-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
 drivers/media/dvb-core/dvbdev.c | 37 ++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..7988d01ebe99 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -518,7 +518,6 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 	dvbdev->id = id;
 	dvbdev->adapter = adap;
 	dvbdev->priv = priv;
-	dvbdev->fops = dvbdevfops;
 	init_waitqueue_head(&dvbdev->wait_queue);
 	dvbdevfops->owner = adap->module;
 	list_add_tail(&dvbdev->list_head, &adap->device_list);
@@ -537,10 +536,10 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 			kfree(new_node);
 		}
 		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
 		*pdvbdev = NULL;
 		up_write(&minor_rwsem);
 		mutex_unlock(&dvbdev_register_lock);
+		dvb_device_put(dvbdev);
 		return -EINVAL;
 	}
 
@@ -556,11 +555,16 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 			kfree(dvbdevfops);
 			kfree(new_node);
 		}
+		down_write(&minor_rwsem);
+		dvb_minors[minor] = NULL;
+		up_write(&minor_rwsem);
 		dvb_media_device_free(dvbdev);
 		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
 		*pdvbdev = NULL;
 		mutex_unlock(&dvbdev_register_lock);
+		/* drop the minors-table and the caller's references */
+		dvb_device_put(dvbdev);
+		dvb_device_put(dvbdev);
 		return ret;
 	}
 
@@ -575,14 +579,31 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 			kfree(dvbdevfops);
 			kfree(new_node);
 		}
+		down_write(&minor_rwsem);
+		dvb_minors[minor] = NULL;
+		up_write(&minor_rwsem);
 		dvb_media_device_free(dvbdev);
 		list_del(&dvbdev->list_head);
-		kfree(dvbdev);
 		*pdvbdev = NULL;
 		mutex_unlock(&dvbdev_register_lock);
+		/* drop the minors-table and the caller's references */
+		dvb_device_put(dvbdev);
+		dvb_device_put(dvbdev);
 		return PTR_ERR(clsdev);
 	}
 
+	/*
+	 * Publish the file operations only once every step that can fail
+	 * has completed.  Until then dvb_minors[minor] is just a
+	 * reservation: dvb_device_open() turns an entry whose fops is
+	 * still NULL into -ENODEV without taking a reference on it, so
+	 * the error paths above never free file operations that a
+	 * concurrent open() is about to use.
+	 */
+	down_write(&minor_rwsem);
+	dvbdev->fops = dvbdevfops;
+	up_write(&minor_rwsem);
+
 	dprintk("DVB: register adapter%d/%s%d @ minor: %i (0x%02x)\n",
 		adap->num, dnames[type], id, minor, minor);
 
@@ -598,7 +619,6 @@ void dvb_remove_device(struct dvb_device *dvbdev)
 
 	down_write(&minor_rwsem);
 	dvb_minors[dvbdev->minor] = NULL;
-	dvb_device_put(dvbdev);
 	up_write(&minor_rwsem);
 
 	dvb_media_device_free(dvbdev);
@@ -606,6 +626,13 @@ void dvb_remove_device(struct dvb_device *dvbdev)
 	device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor));
 
 	list_del(&dvbdev->list_head);
+
+	/*
+	 * Drop the minors-table reference only after the last use of the
+	 * object above: should this be the last reference, freeing it any
+	 * earlier would turn those statements into a use-after-free.
+	 */
+	dvb_device_put(dvbdev);
 }
 EXPORT_SYMBOL(dvb_remove_device);
 
-- 
2.34.1