linux-kernel - Re: [PATCH v3] comedi: multiq3: sanitize config options in multiq3

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: <61f72946-1d7f-4e31-bc1d-85b779681987@mev.co.uk>
Date: Thu, 23 Oct 2025 14:34:07 +0100
From: Ian Abbott <abbotti@....co.uk>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>,
 H Hartley Sweeten <hsweeten@...ionengravers.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org,
 syzbot+7811bb68a317954a0347@...kaller.appspotmail.com
Subject: Re: [PATCH v3] comedi: multiq3: sanitize config options in
 multiq3_attach()

On 23/10/2025 14:22, Nikita Zhandarovich wrote:
> Syzbot identified an issue [1] in multiq3_attach() that induces a
> task timeout due to open() or COMEDI_DEVCONFIG ioctl operations,
> specifically, in the case of multiq3 driver.
> 
> This problem arose when syzkaller managed to craft weird configuration
> options used to specify the number of channels in encoder subdevice.
> If a particularly great number is passed to s->n_chan in
> multiq3_attach() via it->options[2], then multiple calls to
> multiq3_encoder_reset() at the end of driver-specific attach() method
> will be running for minutes, thus blocking tasks and affected devices
> as well.
> 
> While this issue is most likely not too dangerous for real-life
> devices, it still makes sense to sanitize configuration inputs. Enable
> a sensible limit on the number of encoder chips (4 chips max, each
> with 2 channels) to stop this behaviour from manifesting.
> 
> [1] Syzbot crash:
> INFO: task syz.2.19:6067 blocked for more than 143 seconds.
> ...
> Call Trace:
>   <TASK>
>   context_switch kernel/sched/core.c:5254 [inline]
>   __schedule+0x17c4/0x4d60 kernel/sched/core.c:6862
>   __schedule_loop kernel/sched/core.c:6944 [inline]
>   schedule+0x165/0x360 kernel/sched/core.c:6959
>   schedule_preempt_disabled+0x13/0x30 kernel/sched/core.c:7016
>   __mutex_lock_common kernel/locking/mutex.c:676 [inline]
>   __mutex_lock+0x7e6/0x1350 kernel/locking/mutex.c:760
>   comedi_open+0xc0/0x590 drivers/comedi/comedi_fops.c:2868
>   chrdev_open+0x4cc/0x5e0 fs/char_dev.c:414
>   do_dentry_open+0x953/0x13f0 fs/open.c:965
>   vfs_open+0x3b/0x340 fs/open.c:1097
> ...
> 
> Reported-by: syzbot+7811bb68a317954a0347@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7811bb68a317954a0347
> Fixes: 77e01cdbad51 ("Staging: comedi: add multiq3 driver")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
> v1 -> v2: Lower limit to 8 channels instead of 16 per Ian Abbott's
> <abbotti@....co.uk> suggestion.
> 
> v2 -> v3: Switch to less confusing macro name MULTIQ3_MAX_ENC_CHANS,
> adjust comments accordingly, as well as commit description itself.
> 
>   drivers/comedi/drivers/multiq3.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/comedi/drivers/multiq3.c b/drivers/comedi/drivers/multiq3.c
> index 07ff5383da99..ac369e9a262d 100644
> --- a/drivers/comedi/drivers/multiq3.c
> +++ b/drivers/comedi/drivers/multiq3.c
> @@ -67,6 +67,11 @@
>   #define MULTIQ3_TRSFRCNTR_OL		0x10	/* xfer CNTR to OL (x and y) */
>   #define MULTIQ3_EFLAG_RESET		0x06	/* reset E bit of flag reg */
>   
> +/*
> + * Limit on the number of optional encoder channels
> + */
> +#define MULTIQ3_MAX_ENC_CHANS		8
> +
>   static void multiq3_set_ctrl(struct comedi_device *dev, unsigned int bits)
>   {
>   	/*
> @@ -312,6 +317,10 @@ static int multiq3_attach(struct comedi_device *dev,
>   	s->insn_read	= multiq3_encoder_insn_read;
>   	s->insn_config	= multiq3_encoder_insn_config;
>   
> +	/* sanity check for number of encoder channels */
> +	if (s->n_chan > MULTIQ3_MAX_ENC_CHANS)
> +		s->n_chan = MULTIQ3_MAX_ENC_CHANS;
> +
>   	for (i = 0; i < s->n_chan; i++)
>   		multiq3_encoder_reset(dev, i);
>   

Looks good. Thanks for the fix!

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 )=-