[PATCH v6 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 the mirror problem.  It fills in
dvb_minors[minor] before calling dvb_register_media_device() and
device_create(), and when one of them fails it frees the object and
the file operations:

	dvbdev->minor = minor;
	dvb_minors[minor] = dvb_device_get(dvbdev);
	up_write(&minor_rwsem);
	ret = dvb_register_media_device(...);       /* may fail */
	if (ret) { ...; kfree(dvbdev); }            /* entry stays */
	clsdev = device_create(...);                /* may fail */
	if (IS_ERR(clsdev)) { ...; kfree(dvbdev); } /* entry stays */

An open() that arrives in that window follows the array entry, takes
a reference of its own and runs the file operations of a device whose
registration has not completed, while the error path frees the object
under it and the caller frees the private data behind it.  The array
entry is left behind as well, pointing at freed memory.

The minor number only has to be handed out once, and that is already
guaranteed by dvbdev_register_lock, which is held for the whole
function.  Filling in dvb_minors[] is what makes the device reachable,
so do that last: pick the minor as before, but publish the entry only
after dvb_register_media_device() and device_create() have succeeded.
Since nothing can have looked the device up, the error paths keep
releasing it with a plain kfree(), exactly as they do now, instead of
unpublishing an entry and dropping references that were never taken.

Publishing the entry late leaves a window between device_create() and
the publication in which an open() of the new node finds the entry
empty and fails with -ENODEV even though the node is complete.
dvb_device_open() takes minor_rwsem before it looks the entry up, so
hold the lock across device_create() and the publication: a racing
open() then waits, and sees a device whose entry is already there.
device_create() only fills in the new device and runs its devnode and
uevent callbacks, none of which takes minor_rwsem, and registration
does not take dvbdev_mutex, which dvb_device_open() acquires before
the lock; the lock order is unchanged.

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 | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..bd7f52718426 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -545,7 +545,6 @@ 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) {
@@ -564,6 +563,20 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 		return ret;
 	}
 
+	/*
+	 * dvb_minors[] is what dvb_device_open() follows, so publish the
+	 * entry only after device_create() has succeeded: an entry that is
+	 * reachable while it can still fail would let a concurrent open()
+	 * take a reference of its own to the object the error path is
+	 * about to free, together with the private data behind it.
+	 *
+	 * Take minor_rwsem before device_create() and hold it until the
+	 * entry is published.  dvb_device_open() acquires it before it
+	 * looks the minor up, so an open() that races with the
+	 * registration waits for the node to become usable instead of
+	 * finding it without an entry and failing with -ENODEV.
+	 */
+	down_write(&minor_rwsem);
 	clsdev = device_create(dvb_class, adap->device,
 			       MKDEV(DVB_MAJOR, minor),
 			       dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id);
@@ -579,10 +592,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);
 
@@ -598,7 +615,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 +622,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