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

comedi: Add reference counting for Comedi command handling

For interrupts from badly behaved hardware (as emulated by Syzbot), it
is possible for the Comedi core functions that manage the progress of
asynchronous data acquisition to be called from driver ISRs while no
asynchronous command has been set up, which can cause problems such as
invalid pointer dereferencing or dividing by zero.

To help protect against that, introduce new functions to maintain a
reference counter for asynchronous commands that are being set up.
`comedi_get_is_subdevice_running(s)` will check if a command has been
set up on a subdevice and is still marked as running, and if so will
increment the reference counter and return `true`, otherwise it will
return `false` without modifying the reference counter.
`comedi_put_is_subdevice_running(s)` will decrement the reference
counter and set a completion event when decremented to 0.

Change the `do_cmd_ioctl()` function (responsible for setting up the
asynchronous command) to reinitialize the completion event and set the
reference counter to 1 before it marks the subdevice as running. Change
the `do_become_nonbusy()` function (responsible for destroying a
completed command) to call `comedi_put_is_subdevice_running(s)` and wait
for the completion event after marking the subdevice as not running.

Because the subdevice normally gets marked as not running before the
call to `do_become_nonbusy()` (and may also be called when the Comedi
device is being detached from the low-level driver), add a new flag
`COMEDI_SRF_BUSY` to the set of subdevice run-flags that indicates that
an asynchronous command was set up and will need to be destroyed. This
flag is set by `do_cmd_ioctl()` and cleared and checked by
`do_become_nonbusy()`.

Subsequent patches will change the Comedi core functions that are called
from low-level drivers for asynchrous command handling to make use of
the `comedi_get_is_subdevice_running()` and
`comedi_put_is_subdevice_running()` functions, and will modify the ISRs
of some of these low-level drivers if they dereference the subdevice's
`async` pointer directly.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
Link: https://patch.msgid.link/20251023133001.8439-2-abbotti@mev.co.uk
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

authored by

Ian Abbott and committed by
Greg Kroah-Hartman
4e1da516 a51f025b

+78 -8
+70 -8
drivers/comedi/comedi_fops.c
··· 38 38 * COMEDI_SRF_ERROR: indicates an COMEDI_CB_ERROR event has occurred 39 39 * since the last command was started 40 40 * COMEDI_SRF_RUNNING: command is running 41 + * COMEDI_SRF_BUSY: command was started and subdevice still busy 41 42 * COMEDI_SRF_FREE_SPRIV: free s->private on detach 42 43 * 43 44 * COMEDI_SRF_BUSY_MASK: runflags that indicate the subdevice is "busy" ··· 46 45 #define COMEDI_SRF_RT BIT(1) 47 46 #define COMEDI_SRF_ERROR BIT(2) 48 47 #define COMEDI_SRF_RUNNING BIT(27) 48 + #define COMEDI_SRF_BUSY BIT(28) 49 49 #define COMEDI_SRF_FREE_SPRIV BIT(31) 50 50 51 - #define COMEDI_SRF_BUSY_MASK (COMEDI_SRF_ERROR | COMEDI_SRF_RUNNING) 51 + #define COMEDI_SRF_BUSY_MASK \ 52 + (COMEDI_SRF_ERROR | COMEDI_SRF_RUNNING | COMEDI_SRF_BUSY) 52 53 53 54 /** 54 55 * struct comedi_file - Per-file private data for COMEDI device ··· 668 665 return runflags & COMEDI_SRF_ERROR; 669 666 } 670 667 668 + static bool comedi_is_runflags_busy(unsigned int runflags) 669 + { 670 + return runflags & COMEDI_SRF_BUSY; 671 + } 672 + 671 673 /** 672 674 * comedi_is_subdevice_running() - Check if async command running on subdevice 673 675 * @s: COMEDI subdevice. ··· 694 686 695 687 return comedi_is_runflags_running(runflags); 696 688 } 689 + 690 + /** 691 + * comedi_get_is_subdevice_running() - Get if async command running on subdevice 692 + * @s: COMEDI subdevice. 693 + * 694 + * If an asynchronous COMEDI command is running on the subdevice, increment 695 + * a reference counter. If the function return value indicates that a 696 + * command is running, then the details of the command will not be destroyed 697 + * before a matching call to comedi_put_is_subdevice_running(). 698 + * 699 + * Return: %true if an asynchronous COMEDI command is active on the 700 + * subdevice, else %false. 701 + */ 702 + bool comedi_get_is_subdevice_running(struct comedi_subdevice *s) 703 + { 704 + unsigned long flags; 705 + bool running; 706 + 707 + spin_lock_irqsave(&s->spin_lock, flags); 708 + running = __comedi_is_subdevice_running(s); 709 + if (running) 710 + refcount_inc(&s->async->run_active); 711 + spin_unlock_irqrestore(&s->spin_lock, flags); 712 + return running; 713 + } 714 + EXPORT_SYMBOL_GPL(comedi_get_is_subdevice_running); 715 + 716 + /** 717 + * comedi_put_is_subdevice_running() - Put if async command running on subdevice 718 + * @s: COMEDI subdevice. 719 + * 720 + * Decrements the reference counter that was incremented when 721 + * comedi_get_is_subdevice_running() returned %true. 722 + */ 723 + void comedi_put_is_subdevice_running(struct comedi_subdevice *s) 724 + { 725 + if (refcount_dec_and_test(&s->async->run_active)) 726 + complete_all(&s->async->run_complete); 727 + } 728 + EXPORT_SYMBOL_GPL(comedi_put_is_subdevice_running); 697 729 698 730 bool comedi_can_auto_free_spriv(struct comedi_subdevice *s) 699 731 { ··· 784 736 struct comedi_subdevice *s) 785 737 { 786 738 struct comedi_async *async = s->async; 739 + unsigned int runflags; 740 + unsigned long flags; 787 741 788 742 lockdep_assert_held(&dev->mutex); 789 - comedi_update_subdevice_runflags(s, COMEDI_SRF_RUNNING, 0); 790 - if (async) { 743 + spin_lock_irqsave(&s->spin_lock, flags); 744 + runflags = __comedi_get_subdevice_runflags(s); 745 + __comedi_clear_subdevice_runflags(s, COMEDI_SRF_RUNNING | 746 + COMEDI_SRF_BUSY); 747 + spin_unlock_irqrestore(&s->spin_lock, flags); 748 + if (comedi_is_runflags_busy(runflags)) { 749 + /* 750 + * "Run active" counter was set to 1 when setting up the 751 + * command. Decrement it and wait for it to become 0. 752 + */ 753 + comedi_put_is_subdevice_running(s); 754 + wait_for_completion(&async->run_complete); 791 755 comedi_buf_reset(s); 792 756 async->inttrig = NULL; 793 757 kfree(async->cmd.chanlist); 794 758 async->cmd.chanlist = NULL; 795 759 s->busy = NULL; 796 760 wake_up_interruptible_all(&async->wait_head); 797 - } else { 798 - dev_err(dev->class_dev, 799 - "BUG: (?) %s called with async=NULL\n", __func__); 800 - s->busy = NULL; 801 761 } 802 762 } 803 763 ··· 1916 1860 if (async->cmd.flags & CMDF_WAKE_EOS) 1917 1861 async->cb_mask |= COMEDI_CB_EOS; 1918 1862 1863 + /* 1864 + * Set the "run active" counter with an initial count of 1 that will 1865 + * complete the "safe to reset" event when it is decremented to 0. 1866 + */ 1867 + refcount_set(&s->async->run_active, 1); 1868 + reinit_completion(&s->async->run_complete); 1919 1869 comedi_update_subdevice_runflags(s, COMEDI_SRF_BUSY_MASK, 1920 - COMEDI_SRF_RUNNING); 1870 + COMEDI_SRF_RUNNING | COMEDI_SRF_BUSY); 1921 1871 1922 1872 /* 1923 1873 * Set s->busy _after_ setting COMEDI_SRF_RUNNING flag to avoid
+1
drivers/comedi/drivers.c
··· 677 677 return -ENOMEM; 678 678 679 679 init_waitqueue_head(&async->wait_head); 680 + init_completion(&async->run_complete); 680 681 s->async = async; 681 682 682 683 async->max_bufsize = comedi_default_buf_maxsize_kb * 1024;
+7
include/linux/comedi/comedidev.h
··· 15 15 #include <linux/spinlock_types.h> 16 16 #include <linux/rwsem.h> 17 17 #include <linux/kref.h> 18 + #include <linux/completion.h> 18 19 #include <linux/comedi.h> 19 20 20 21 #define COMEDI_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c)) ··· 273 272 * @events: Bit-vector of events that have occurred. 274 273 * @cmd: Details of comedi command in progress. 275 274 * @wait_head: Task wait queue for file reader or writer. 275 + * @run_complete: "run complete" completion event. 276 + * @run_active: "run active" reference counter. 276 277 * @cb_mask: Bit-vector of events that should wake waiting tasks. 277 278 * @inttrig: Software trigger function for command, or NULL. 278 279 * ··· 360 357 unsigned int events; 361 358 struct comedi_cmd cmd; 362 359 wait_queue_head_t wait_head; 360 + struct completion run_complete; 361 + refcount_t run_active; 363 362 unsigned int cb_mask; 364 363 int (*inttrig)(struct comedi_device *dev, struct comedi_subdevice *s, 365 364 unsigned int x); ··· 589 584 int comedi_dev_put(struct comedi_device *dev); 590 585 591 586 bool comedi_is_subdevice_running(struct comedi_subdevice *s); 587 + bool comedi_get_is_subdevice_running(struct comedi_subdevice *s); 588 + void comedi_put_is_subdevice_running(struct comedi_subdevice *s); 592 589 593 590 void *comedi_alloc_spriv(struct comedi_subdevice *s, size_t size); 594 591 void comedi_set_spriv_auto_free(struct comedi_subdevice *s);