[PATCH v5 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.

A devtmpfs or udev open() that arrives between device_create() and the
publication finds dvb_minors[minor] still empty and gets -ENODEV, the
same answer it gets for a minor that does not exist yet.  Closing that
window would require holding minor_rwsem across device_create(), which
would serialize every DVB open() against the uevent handling of the
new node; the window is a few instructions wide and userspace retries.

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

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..ef88a6395442 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) {
@@ -583,6 +582,18 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
 		return PTR_ERR(clsdev);
 	}
 
+	/*
+	 * dvb_minors[] is what dvb_device_open() follows, so fill it in
+	 * only now that dvb_register_media_device() and device_create()
+	 * have succeeded: an entry that is reachable while those can still
+	 * fail would let a concurrent open() take a reference of its own to
+	 * the object the error paths are about to free, together with the
+	 * private data behind it.
+	 */
+	down_write(&minor_rwsem);
+	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 +609,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 +616,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