[PATCH v2] media: dvb-core: prevent frontend thread resurrection after unregister

[YuanZhe Liu]

A use-after-free read in dvb_frontend_thread() was observed when a DVB USB
device was disconnected concurrently with userspace opening and closing the
frontend device node:

  BUG: KASAN: slab-use-after-free in dvb_frontend_thread+0x1380/0x1620
  Read of size 8 at addr ffff888008dd4320
  Comm: kdvb-ad-0-fe-0 Not tainted 7.1.2-dirty #81
  dvb_frontend_should_wakeup drivers/media/dvb-core/dvb_frontend.c:641
  dvb_frontend_thread drivers/media/dvb-core/dvb_frontend.c:682

  Allocated by the frontend attach path:
   cinergyt2_fe_attach+0x... (drivers/media/usb/dvb-usb/cinergyT2-fe.c:271)

  Freed by the final frontend release:
   dvb_frontend_put+0x... (drivers/media/dvb-core/dvb_frontend.c)
   dvb_frontend_release+0x... (drivers/media/dvb-core/dvb_frontend.c:2934)

On disconnect, dvb_unregister_frontend() stops the frontend kthread through
dvb_frontend_stop() and unregisters the dvb device, but it never marked the
frontend itself as removed: fe->exit was left at DVB_FE_NORMAL_EXIT, or at
DVB_FE_NO_EXIT when no thread was running.  dvb_frontend_open() does not
hold frontend_mutex, and dvb_frontend_start() did not check the removal
state at all, so an open racing with dvb_unregister_frontend() passed the
DVB_FE_DEVICE_REMOVED check in dvb_frontend_open(), unconditionally reset
fe->exit to DVB_FE_NO_EXIT and spawned a new kthread on a frontend that
was being removed.

dvb_unregister_frontend() and dvb_frontend_detach() then dropped both
registration references.  The resurrected kthread never holds a reference
on the frontend object itself, so once the last file descriptor was
closed, dvb_frontend_release() dropped the final reference and freed the
frontend state while the kthread was still running, resulting in the
use-after-free reported above.

Close that window by unregistering the device node before the thread is
stopped.  dvb_device_open() invokes the frontend open handler while
holding minor_rwsem for reading, and dvb_remove_device() takes the same
lock for writing, so once dvb_remove_device() has returned no open can
reach dvb_frontend_start() any more and the kthread cannot be restarted
after dvb_frontend_stop() has killed it.  Mark the frontend removed after
the thread has terminated, so that the marker is written only once the
thread is gone.

Make the removal state sticky in the frontend as well:

- do not overwrite DVB_FE_DEVICE_REMOVED when the thread notices the exit
  condition;
- do not derive DVB_FE_DEVICE_REMOVED from kthread_should_stop() when the
  thread returns.  Restarting an idle frontend stops the thread through
  kthread_stop() as well, and treating that as a removal left the device
  unusable;
- keep the DVB_FE_DEVICE_REMOVED check in dvb_frontend_start() and the
  reset in dvb_register_frontend() as a second line of defence, which
  also covers drivers that mark the frontend removed themselves.

em28xx already sets DVB_FE_DEVICE_REMOVED manually before unregistering
its frontends; the core now provides this protection for every dvb-core
user.

Fixes: 1f862a68df24 ("[media] dvb_frontend: move kref to struct dvb_frontend")
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>
---
Changes since v1:
 - remove the device node before the frontend thread is stopped and
   set DVB_FE_DEVICE_REMOVED only after dvb_frontend_stop() has
   returned.  Unregistering the device node takes minor_rwsem for
   writing, so no open can restart the kthread once it has returned,
   and the marker is written when the thread is already gone.
 - do not derive DVB_FE_DEVICE_REMOVED from kthread_should_stop()
   when the thread returns: restarting an idle frontend stops the
   thread the same way, which would leave the device unusable.
 - do not overwrite DVB_FE_DEVICE_REMOVED with DVB_FE_NORMAL_EXIT in
   the thread exit path.

 drivers/media/dvb-core/dvb_frontend.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d082b6c57c76..74c8042d84fc 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -688,7 +688,8 @@ static int dvb_frontend_thread(void *data)
 			/* got signal or quitting */
 			if (!down_interruptible(&fepriv->sem))
 				semheld = true;
-			fe->exit = DVB_FE_NORMAL_EXIT;
+			if (fe->exit != DVB_FE_DEVICE_REMOVED)
+				fe->exit = DVB_FE_NORMAL_EXIT;
 			break;
 		}
 
@@ -794,9 +795,7 @@ static int dvb_frontend_thread(void *data)
 	}
 
 	fepriv->thread = NULL;
-	if (kthread_should_stop())
-		fe->exit = DVB_FE_DEVICE_REMOVED;
-	else
+	if (fe->exit != DVB_FE_DEVICE_REMOVED)
 		fe->exit = DVB_FE_NO_EXIT;
 	mb();
 
@@ -873,6 +872,11 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 	if (down_interruptible(&fepriv->sem))
 		return -EINTR;
 
+	if (fe->exit == DVB_FE_DEVICE_REMOVED) {
+		up(&fepriv->sem);
+		return -ENODEV;
+	}
+
 	fepriv->state = FESTATE_IDLE;
 	fe->exit = DVB_FE_NO_EXIT;
 	fepriv->thread = NULL;
@@ -3042,6 +3046,7 @@ int dvb_register_frontend(struct dvb_adapter *dvb,
 	init_waitqueue_head(&fepriv->events.wait_queue);
 	mutex_init(&fepriv->events.mtx);
 	fe->dvb = dvb;
+	fe->exit = DVB_FE_NO_EXIT;
 	fepriv->inversion = INVERSION_OFF;
 
 	dev_info(fe->dvb->device,
@@ -3076,8 +3081,18 @@ int dvb_unregister_frontend(struct dvb_frontend *fe)
 	dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
 	mutex_lock(&frontend_mutex);
-	dvb_frontend_stop(fe);
+	/*
+	 * Remove the device node before stopping the thread.  New opens
+	 * cannot enter dvb_frontend_open() once dvb_remove_device() has
+	 * returned, because dvb_device_open() calls the frontend open
+	 * handler while holding minor_rwsem for reading and
+	 * dvb_remove_device() takes it for writing.  This guarantees that
+	 * dvb_frontend_start() cannot restart the kthread after it has
+	 * been stopped here.
+	 */
 	dvb_remove_device(fepriv->dvbdev);
+	dvb_frontend_stop(fe);
+	fe->exit = DVB_FE_DEVICE_REMOVED;
 
 	/* fe is invalid now */
 	mutex_unlock(&frontend_mutex);
-- 
2.34.1