[PATCH v3] 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)

dvb_unregister_frontend() stops the frontend kthread and only then removes
the dvb device node.  dvb_frontend_open() does not hold frontend_mutex, so
an open that is already past the DVB_FE_DEVICE_REMOVED check can call
dvb_frontend_start() after dvb_frontend_stop() has returned and spawn a new
kthread.  The frontend kthread does not hold a reference on the frontend
object, so once the last file descriptor is closed the final
dvb_frontend_put() frees the frontend state while that kthread is still
running, which is the use-after-free reported above.

Remove the device node first.  dvb_device_open() invokes the frontend open
handler while holding minor_rwsem for reading, and dvb_remove_device()
takes the same lock for writing and clears the minor, so once it has
returned no open can reach dvb_frontend_start() any more.  The kthread that
follows is then the last one, and dvb_frontend_stop() waits for it.

No change to the fe->exit state machine is needed.  The thread that is
stopped through kthread_stop() records DVB_FE_DEVICE_REMOVED before it
releases fepriv->sem, which is what keeps concurrent ioctl() callers on
already open descriptors away from the driver, and DVB_FE_DEVICE_REMOVED
keeps the meaning drivers rely on, such as skipping hardware access on a
device that is no longer there.

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 v2:
 - remove the device node before the frontend thread is stopped and
   drop the fe->exit changes: unregistering the node takes
   minor_rwsem for writing, so no open can restart the kthread after
   dvb_frontend_stop().  Leaving the exit state machine alone also
   keeps concurrent ioctl() callers, driver power-down on normal
   unload and the DVB_FE_DEVICE_REMOVED semantics as they were.

Changes since v1:
 - do not mark the frontend removed in dvb_unregister_frontend();
   the ordering alone closes the race, and an unconditional
   DVB_FE_DEVICE_REMOVED broke hardware power-down on normal driver
   unload.

 drivers/media/dvb-core/dvb_frontend.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d082b6c57c76..22d2e0112b74 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -3076,8 +3076,15 @@ 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: dvb_device_open()
+	 * calls dvb_frontend_open() while holding minor_rwsem for reading and
+	 * dvb_remove_device() takes it for writing, so once this returns no
+	 * open can reach dvb_frontend_start() and spawn a kthread that
+	 * dvb_frontend_stop() below would no longer stop.
+	 */
 	dvb_remove_device(fepriv->dvbdev);
+	dvb_frontend_stop(fe);
 
 	/* fe is invalid now */
 	mutex_unlock(&frontend_mutex);
-- 
2.34.1