linux-kernel - Re: [PATCH] comedi: runflags cannot determine whether to reclaim chanlist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1440a936-ef7c-4a17-989b-02079ecf4171@mev.co.uk>
Date: Mon, 15 Dec 2025 12:25:19 +0000
From: Ian Abbott <abbotti@....co.uk>
To: Edward Adam Davis <eadavis@...com>,
 syzbot+f238baf6ded841b5a82e@...kaller.appspotmail.com
Cc: hsweeten@...ionengravers.com, linux-kernel@...r.kernel.org,
 syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] comedi: runflags cannot determine whether to reclaim
 chanlist

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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f238baf6ded841b5a82e
> Signed-off-by: Edward Adam Davis <eadavis@...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@....co.uk>

-- 
-=( Ian Abbott <abbotti@....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 )=-