Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

comedi: fix race between polling and detaching

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>

authored by

Ian Abbott and committed by
Greg Kroah-Hartman
35b6fc51 631ae0c0

+36 -11
+25 -8
drivers/comedi/comedi_fops.c
··· 787 787 struct comedi_subdevice *s; 788 788 int i; 789 789 790 + lockdep_assert_held_write(&dev->attach_lock); 790 791 lockdep_assert_held(&dev->mutex); 791 792 if (!dev->attached) 792 793 return 0; ··· 796 795 s = &dev->subdevices[i]; 797 796 if (s->busy) 798 797 return 1; 799 - if (s->async && comedi_buf_is_mmapped(s)) 798 + if (!s->async) 799 + continue; 800 + if (comedi_buf_is_mmapped(s)) 801 + return 1; 802 + /* 803 + * There may be tasks still waiting on the subdevice's wait 804 + * queue, although they should already be about to be removed 805 + * from it since the subdevice has no active async command. 806 + */ 807 + if (wq_has_sleeper(&s->async->wait_head)) 800 808 return 1; 801 809 } 802 810 ··· 835 825 return -EPERM; 836 826 837 827 if (!arg) { 838 - if (is_device_busy(dev)) 839 - return -EBUSY; 840 - if (dev->attached) { 841 - struct module *driver_module = dev->driver->module; 828 + int rc = 0; 842 829 843 - comedi_device_detach(dev); 844 - module_put(driver_module); 830 + if (dev->attached) { 831 + down_write(&dev->attach_lock); 832 + if (is_device_busy(dev)) { 833 + rc = -EBUSY; 834 + } else { 835 + struct module *driver_module = 836 + dev->driver->module; 837 + 838 + comedi_device_detach_locked(dev); 839 + module_put(driver_module); 840 + } 841 + up_write(&dev->attach_lock); 845 842 } 846 - return 0; 843 + return rc; 847 844 } 848 845 849 846 if (copy_from_user(&it, arg, sizeof(it)))
+1
drivers/comedi/comedi_internal.h
··· 50 50 int insn_inval(struct comedi_device *dev, struct comedi_subdevice *s, 51 51 struct comedi_insn *insn, unsigned int *data); 52 52 53 + void comedi_device_detach_locked(struct comedi_device *dev); 53 54 void comedi_device_detach(struct comedi_device *dev); 54 55 int comedi_device_attach(struct comedi_device *dev, 55 56 struct comedi_devconfig *it);
+10 -3
drivers/comedi/drivers.c
··· 158 158 int i; 159 159 struct comedi_subdevice *s; 160 160 161 - lockdep_assert_held(&dev->attach_lock); 161 + lockdep_assert_held_write(&dev->attach_lock); 162 162 lockdep_assert_held(&dev->mutex); 163 163 if (dev->subdevices) { 164 164 for (i = 0; i < dev->n_subdevices; i++) { ··· 196 196 comedi_clear_hw_dev(dev); 197 197 } 198 198 199 - void comedi_device_detach(struct comedi_device *dev) 199 + void comedi_device_detach_locked(struct comedi_device *dev) 200 200 { 201 + lockdep_assert_held_write(&dev->attach_lock); 201 202 lockdep_assert_held(&dev->mutex); 202 203 comedi_device_cancel_all(dev); 203 - down_write(&dev->attach_lock); 204 204 dev->attached = false; 205 205 dev->detach_count++; 206 206 if (dev->driver) 207 207 dev->driver->detach(dev); 208 208 comedi_device_detach_cleanup(dev); 209 + } 210 + 211 + void comedi_device_detach(struct comedi_device *dev) 212 + { 213 + lockdep_assert_held(&dev->mutex); 214 + down_write(&dev->attach_lock); 215 + comedi_device_detach_locked(dev); 209 216 up_write(&dev->attach_lock); 210 217 } 211 218