[PATCH] 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 before it calls into the driver, so a driver callback can still
be executing 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.  Ioctls that
arrive later see ->exit and return -ENODEV before they reach the
driver.

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/dvb_frontend.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 0286da57f382..076e24ea8a4b 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -817,13 +817,21 @@ 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(fepriv->thread);
+		fepriv->state = FESTATE_IDLE;
+	}
 
-	sema_init(&fepriv->sem, 1);
-	fepriv->state = FESTATE_IDLE;
+	/*
+	 * An ioctl holds fepriv->sem for its whole duration, but it checks
+	 * ->exit only before it calls into the driver.  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.  Ioctls that arrive from now on see
+	 * ->exit and return -ENODEV without reaching the driver.
+	 */
+	down(&fepriv->sem);
+	up(&fepriv->sem);
 
 	/* paranoia check in case a signal arrived */
 	if (fepriv->thread)
-- 
2.34.1