comedi: fix race between polling and detaching

commit 35b6fc51c666fc96355be5cd633ed0fe4ccf68b2 upstream.

syzbot reports a use-after-free in comedi in the below link, which is
due to comedi gladly removing the allocated async area even though poll
requests are still active on the wait_queue_head inside of it. This can
cause a use-after-free when the poll entries are later triggered or
removed, as the memory for the wait_queue_head has been freed.  We need
to check there are no tasks queued on any of the subdevices' wait queues
before allowing the device to be detached by the `COMEDI_DEVCONFIG`
ioctl.

Tasks will read-lock `dev->attach_lock` before adding themselves to the
subdevice wait queue, so fix the problem in the `COMEDI_DEVCONFIG` ioctl
handler by write-locking `dev->attach_lock` before checking that all of
the subdevices are safe to be deleted.  This includes testing for any
sleepers on the subdevices' wait queues.  It remains locked until the
device has been detached.  This requires the `comedi_device_detach()`
function to be refactored slightly, moving the bulk of it into new
function `comedi_device_detach_locked()`.

Note that the refactor of `comedi_device_detach()` results in
`comedi_device_cancel_all()` now being called while `dev->attach_lock`
is write-locked, which wasn't the case previously, but that does not
matter.

Thanks to Jens Axboe for diagnosing the problem and co-developing this
patch.

Cc: stable <stable@kernel.org>
Fixes: 2f3fdcd7ce93 ("staging: comedi: add rw_semaphore to protect against device detachment")
Link: https://lore.kernel.org/all/687bd5fe.a70a0220.693ce.0091.GAE@google.com/
Reported-by: syzbot+01523a0ae5600aef5895@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=01523a0ae5600aef5895
Co-developed-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Tested-by: Jens Axboe <axboe@kernel.dk>
Link: https://lore.kernel.org/r/20250722155316.27432-1-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index c83fd14dd7ad568619bf1545ef90437cecf3d9c2..23b7178522ae031bf1fa5b541ff160a18644b704 100644 (file)
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -787,6 +787,7 @@ static int is_device_busy(struct comedi_device *dev)
        struct comedi_subdevice *s;
        int i;
 
+       lockdep_assert_held_write(&dev->attach_lock);
        lockdep_assert_held(&dev->mutex);
        if (!dev->attached)
                return 0;
@@ -795,7 +796,16 @@ static int is_device_busy(struct comedi_device *dev)
                s = &dev->subdevices[i];
                if (s->busy)
                        return 1;
-               if (s->async && comedi_buf_is_mmapped(s))
+               if (!s->async)
+                       continue;
+               if (comedi_buf_is_mmapped(s))
+                       return 1;
+               /*
+                * There may be tasks still waiting on the subdevice's wait
+                * queue, although they should already be about to be removed
+                * from it since the subdevice has no active async command.
+                */
+               if (wq_has_sleeper(&s->async->wait_head))
                        return 1;
        }
 
@@ -825,15 +835,22 @@ static int do_devconfig_ioctl(struct comedi_device *dev,
                return -EPERM;
 
        if (!arg) {
-               if (is_device_busy(dev))
-                       return -EBUSY;
+               int rc = 0;
+
                if (dev->attached) {
-                       struct module *driver_module = dev->driver->module;
+                       down_write(&dev->attach_lock);
+                       if (is_device_busy(dev)) {
+                               rc = -EBUSY;
+                       } else {
+                               struct module *driver_module =
+                                       dev->driver->module;
 
-                       comedi_device_detach(dev);
-                       module_put(driver_module);
+                               comedi_device_detach_locked(dev);
+                               module_put(driver_module);
+                       }
+                       up_write(&dev->attach_lock);
                }
-               return 0;
+               return rc;
        }
 
        if (copy_from_user(&it, arg, sizeof(it)))

diff --git a/drivers/comedi/comedi_internal.h b/drivers/comedi/comedi_internal.h
index 9b3631a654c895a6b1ae70d93cf69eae6bd469d6..cf10ba016ebc8141091c6917cbe039a21bbacf97 100644 (file)
--- a/drivers/comedi/comedi_internal.h
+++ b/drivers/comedi/comedi_internal.h
@@ -50,6 +50,7 @@ extern struct mutex comedi_drivers_list_lock;
 int insn_inval(struct comedi_device *dev, struct comedi_subdevice *s,
               struct comedi_insn *insn, unsigned int *data);
 
+void comedi_device_detach_locked(struct comedi_device *dev);
 void comedi_device_detach(struct comedi_device *dev);
 int comedi_device_attach(struct comedi_device *dev,
                         struct comedi_devconfig *it);

diff --git a/drivers/comedi/drivers.c b/drivers/comedi/drivers.c
index 9e4b7c840a8f5a7492556b61d990c92a939955d2..f1dc854928c17684674cf4b72cd26ab902a304fb 100644 (file)
--- a/drivers/comedi/drivers.c
+++ b/drivers/comedi/drivers.c
@@ -158,7 +158,7 @@ static void comedi_device_detach_cleanup(struct comedi_device *dev)
        int i;
        struct comedi_subdevice *s;
 
-       lockdep_assert_held(&dev->attach_lock);
+       lockdep_assert_held_write(&dev->attach_lock);
        lockdep_assert_held(&dev->mutex);
        if (dev->subdevices) {
                for (i = 0; i < dev->n_subdevices; i++) {
@@ -196,16 +196,23 @@ static void comedi_device_detach_cleanup(struct comedi_device *dev)
        comedi_clear_hw_dev(dev);
 }
 
-void comedi_device_detach(struct comedi_device *dev)
+void comedi_device_detach_locked(struct comedi_device *dev)
 {
+       lockdep_assert_held_write(&dev->attach_lock);
        lockdep_assert_held(&dev->mutex);
        comedi_device_cancel_all(dev);
-       down_write(&dev->attach_lock);
        dev->attached = false;
        dev->detach_count++;
        if (dev->driver)
                dev->driver->detach(dev);
        comedi_device_detach_cleanup(dev);
+}
+
+void comedi_device_detach(struct comedi_device *dev)
+{
+       lockdep_assert_held(&dev->mutex);
+       down_write(&dev->attach_lock);
+       comedi_device_detach_locked(dev);
        up_write(&dev->attach_lock);
 }