[PATCH v3] 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 holds fepriv->sem for its whole duration, 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.  The wait is done
while holding the semaphore, and ->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.

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>
---
v3:
  - Release the semaphore that dvb_frontend_start() takes for a
    frontend thread when kthread_stop() stopped that thread before it
    ran, which would otherwise block the wait below forever.
  - Restore ->exit while the semaphore is held, since the frontend
    thread can clear it on its way out.
  - Check ->exit in the compat property ioctl path, which bypasses
    the check in dvb_frontend_do_ioctl().
  Code and commit message changes, from review comments.

 drivers/media/dvb-core/dvb_frontend.c | 37 ++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 0286da57f382..89a7cb109de6 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -817,13 +817,33 @@ 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 holds fepriv->sem for its whole duration, 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.  The frontend thread may also have exited on its own
+	 * and left ->exit at DVB_FE_NO_EXIT, so restore it while holding the
+	 * semaphore; ioctls that arrive later then return -ENODEV.
+	 */
+	down(&fepriv->sem);
+	if (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)
@@ -2246,6 +2266,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);
-- 
2.34.1