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@vger.kernel.org Co-developed-by: Bin Yu Signed-off-by: Bin Yu Co-developed-by: Cheng Wen Signed-off-by: Cheng Wen Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn> --- 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