[PATCH v4] media: dvb-core: wait for a running frontend ioctl before device removal

[YuanZhe Liu]

dvb_frontend_stop() runs in the device removal path, when the
frontend is unregistered.  An ioctl that is already running at that
point has taken fepriv->sem and calls into the driver, but it checks
->exit only right after taking the semaphore, so one that started
before ->exit was set can still be inside the driver while the driver
state is being freed.

Two cases were not synchronised at all:

  - when the frontend device node was opened read-only, no frontend
    thread is started, so fepriv->thread is NULL and the function
    returned immediately.  Unlike the other legacy statistics ioctls,
    which return -EAGAIN, FE_READ_STATUS is allowed to run without the
    thread, so its callback can be running when the removal path frees
    the driver state;

  - when a thread exists, the thread takes fepriv->sem on its way out,
    but that acquisition is interruptible.  If it is interrupted, the
    sema_init() that followed discarded the hold of a running ioctl and
    left the semaphore with a surplus count, which also allowed two
    ioctls to call into the driver at the same time.

A use-after-free read was observed on a Twinhan USB2.0 DVB-T receiver
after the USB device had been disconnected, while the frontend device
node was still open:

  Read of size 8 at addr ffff8880068c8e90
  vp7045_usb_op drivers/media/usb/dvb-usb/vp7045.c:61
  vp7045_read_reg drivers/media/usb/dvb-usb/vp7045.c:86
  vp7045_fe_read_status drivers/media/usb/dvb-usb/vp7045-fe.c:29
  dvb_frontend_do_ioctl drivers/media/dvb-core/dvb_frontend.c:2101
  dvb_usercopy drivers/media/dvb-core/dvbdev.c:996

vp7045_usb_op() builds the receive control pipe from d->udev, but the
dvb_usb_device holding it had already been freed by dvb_usb_exit().

Wait for such an ioctl instead of re-initialising the semaphore,
whether or not a frontend thread exists, so that the removal cannot
proceed while a callback is using the driver state.  When the frontend
is being removed, ->exit is set back to DVB_FE_NORMAL_EXIT there if the
frontend thread cleared it on its way out, so that ioctls arriving
later return -ENODEV before they reach the driver.  The same function
also runs when a stopping thread is replaced on open, and
dvb_frontend_start() sets ->exit again for the new thread there, so
the restore is done for removal only.

dvb_frontend_start() takes fepriv->sem and leaves it to the new
thread to release.  If kthread_stop() stops that thread before
threadfn() runs, nobody releases that hold, which would block the
wait above forever; release it and clear ->thread in that case.

The compat property ioctls take the same semaphore but bypass the
->exit check in dvb_frontend_do_ioctl(), so check ->exit after taking
the semaphore there as well.

Both the early return and the semaphore reset have been in this
function since the initial git import of the tree.  An attempt in
2022 to close this race, commit 6769a0b7ee0c ("media: dvb-core: Fix
use-after-free on race condition at dvb_frontend"), waited for every
user of the device node to close it, and was reverted by
commit ec21a38df77a ("Revert "media: dvb-core: Fix use-after-free
on race condition at dvb_frontend"") after it blocked the open path
for minutes.  The wait added here covers only the ioctl that is
still running, and is taken after the frontend thread has exited.

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>
---
v4:
  - Set ->exit back to DVB_FE_NORMAL_EXIT only when the frontend is
    being removed.  dvb_frontend_stop() also runs when a stopping
    thread is replaced on open, where dvb_frontend_start() sets
    ->exit again; restoring it there would leave a live device
    rejecting ioctls if the open gives up early.
  - Do not claim that an ioctl holds the semaphore for its whole
    duration: the blocking FE_GET_EVENT wait drops it while it
    sleeps, which is a separate path and unchanged here.
  Code and commit message changes, from review comments.

 drivers/media/dvb-core/dvb_frontend.c | 44 +++++++++++++++++++++------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 0286da57f382..ec8a6c271c33 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -807,7 +807,7 @@ static int dvb_frontend_thread(void *data)
 	return 0;
 }
 
-static void dvb_frontend_stop(struct dvb_frontend *fe)
+static void dvb_frontend_stop(struct dvb_frontend *fe, bool removing)
 {
 	struct dvb_frontend_private *fepriv = fe->frontend_priv;
 
@@ -817,13 +817,34 @@ static void dvb_frontend_stop(struct dvb_frontend *fe)
 		fe->exit = DVB_FE_NORMAL_EXIT;
 	mb();
 
-	if (!fepriv->thread)
-		return;
-
-	kthread_stop(fepriv->thread);
+	if (fepriv->thread) {
+		/*
+		 * kthread_stop() returns -EINTR when it stopped the thread
+		 * before threadfn() ran.  That thread never released the
+		 * semaphore dvb_frontend_start() took for it, nor cleared
+		 * ->thread.
+		 */
+		if (kthread_stop(fepriv->thread) == -EINTR) {
+			up(&fepriv->sem);
+			fepriv->thread = NULL;
+		}
+		fepriv->state = FESTATE_IDLE;
+	}
 
-	sema_init(&fepriv->sem, 1);
-	fepriv->state = FESTATE_IDLE;
+	/*
+	 * An ioctl calls into the driver while holding fepriv->sem, but
+	 * checks ->exit only after taking it.  One that started before
+	 * ->exit was set above can still be running inside the driver, whose
+	 * state is about to be removed, so wait for it here instead of
+	 * re-initialising the semaphore.  When the frontend is being removed
+	 * the thread may also have exited on its own and left ->exit at
+	 * DVB_FE_NO_EXIT; restore it while holding the semaphore, so that
+	 * ioctls arriving later return -ENODEV.
+	 */
+	down(&fepriv->sem);
+	if (removing && fe->exit == DVB_FE_NO_EXIT)
+		fe->exit = DVB_FE_NORMAL_EXIT;
+	up(&fepriv->sem);
 
 	/* paranoia check in case a signal arrived */
 	if (fepriv->thread)
@@ -866,7 +887,7 @@ static int dvb_frontend_start(struct dvb_frontend *fe)
 		if (fe->exit == DVB_FE_NO_EXIT)
 			return 0;
 		else
-			dvb_frontend_stop(fe);
+			dvb_frontend_stop(fe, false);
 	}
 
 	if (signal_pending(current))
@@ -2246,6 +2267,11 @@ static long dvb_frontend_compat_ioctl(struct file *file, unsigned int cmd,
 		if (down_interruptible(&fepriv->sem))
 			return -ERESTARTSYS;
 
+		if (fe->exit != DVB_FE_NO_EXIT) {
+			up(&fepriv->sem);
+			return -ENODEV;
+		}
+
 		err = dvb_frontend_handle_compat_ioctl(file, cmd, arg);
 
 		up(&fepriv->sem);
@@ -3099,7 +3125,7 @@ int dvb_unregister_frontend(struct dvb_frontend *fe)
 	dev_dbg(fe->dvb->device, "%s:\n", __func__);
 
 	mutex_lock(&frontend_mutex);
-	dvb_frontend_stop(fe);
+	dvb_frontend_stop(fe, true);
 	dvb_remove_device(fepriv->dvbdev);
 
 	/* fe is invalid now */
-- 
2.34.1