syzbot reported a memory leak [1], because patch 4e1da516debb did
not consider the exceptional exit case in do_cmd_ioctl() where runflags
is not set.
This caused chanlist not to be properly freed by do_become_nonbusy(),
as it only frees chanlist when runflags is correctly set.

Added a check in do_become_nonbusy() for the case where runflags is not
set, to properly free the chanlist memory.

[1]
BUG: memory leak
  backtrace (crc 844a0efa):
    __comedi_get_user_chanlist drivers/comedi/comedi_fops.c:1815 [inline]
    do_cmd_ioctl.part.0+0x112/0x350 drivers/comedi/comedi_fops.c:1890
    do_cmd_ioctl drivers/comedi/comedi_fops.c:1858 [inline]

Fixes: 4e1da516debb ("comedi: Add reference counting for Comedi command handling")
Reported-by: syzbot+f238baf6ded841b5a82e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f238baf6ded841b5a82e
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 drivers/comedi/comedi_fops.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 657c98cd723e..003586a381ad 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -793,13 +793,15 @@ static void do_become_nonbusy(struct comedi_device *dev,
 	__comedi_clear_subdevice_runflags(s, COMEDI_SRF_RUNNING |
 					     COMEDI_SRF_BUSY);
 	spin_unlock_irqrestore(&s->spin_lock, flags);
-	if (comedi_is_runflags_busy(runflags)) {
+	if (async) {
 		/*
 		 * "Run active" counter was set to 1 when setting up the
 		 * command.  Decrement it and wait for it to become 0.
 		 */
-		comedi_put_is_subdevice_running(s);
-		wait_for_completion(&async->run_complete);
+		if (comedi_is_runflags_busy(runflags)) {
+			comedi_put_is_subdevice_running(s);
+			wait_for_completion(&async->run_complete);
+		}
 		comedi_buf_reset(s);
 		async->inttrig = NULL;
 		kfree(async->cmd.chanlist);
-- 
2.43.0

On 15/12/2025 11:11, Edward Adam Davis wrote:
> syzbot reported a memory leak [1], because patch 4e1da516debb did
> not consider the exceptional exit case in do_cmd_ioctl() where runflags
> is not set.
> This caused chanlist not to be properly freed by do_become_nonbusy(),
> as it only frees chanlist when runflags is correctly set.
> 
> Added a check in do_become_nonbusy() for the case where runflags is not
> set, to properly free the chanlist memory.
> 
> [1]
> BUG: memory leak
>    backtrace (crc 844a0efa):
>      __comedi_get_user_chanlist drivers/comedi/comedi_fops.c:1815 [inline]
>      do_cmd_ioctl.part.0+0x112/0x350 drivers/comedi/comedi_fops.c:1890
>      do_cmd_ioctl drivers/comedi/comedi_fops.c:1858 [inline]
> 
> Fixes: 4e1da516debb ("comedi: Add reference counting for Comedi command handling")
> Reported-by: syzbot+f238baf6ded841b5a82e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f238baf6ded841b5a82e
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>   drivers/comedi/comedi_fops.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> index 657c98cd723e..003586a381ad 100644
> --- a/drivers/comedi/comedi_fops.c
> +++ b/drivers/comedi/comedi_fops.c
> @@ -793,13 +793,15 @@ static void do_become_nonbusy(struct comedi_device *dev,
>   	__comedi_clear_subdevice_runflags(s, COMEDI_SRF_RUNNING |
>   					     COMEDI_SRF_BUSY);
>   	spin_unlock_irqrestore(&s->spin_lock, flags);
> -	if (comedi_is_runflags_busy(runflags)) {
> +	if (async) {

The `if (async)` test is actually redundant (the code should not be 
reachable if `async` is null), but harmless, and the pre-4e1da516debb 
code tested it as a precaution, so no problem.

>   		/*
>   		 * "Run active" counter was set to 1 when setting up the
>   		 * command.  Decrement it and wait for it to become 0.
>   		 */
> -		comedi_put_is_subdevice_running(s);
> -		wait_for_completion(&async->run_complete);
> +		if (comedi_is_runflags_busy(runflags)) {
> +			comedi_put_is_subdevice_running(s);
> +			wait_for_completion(&async->run_complete);
> +		}
>   		comedi_buf_reset(s);
>   		async->inttrig = NULL;
>   		kfree(async->cmd.chanlist);

Thanks for the fix.  Looks good!

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-