| 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
| ||
|
Message-ID: <294599d2-0d07-430a-a200-821d04d14914@mev.co.uk>
Date: Wed, 22 Oct 2025 12:05:55 +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,
syzbot+ab8008c24e84adee93ff@...kaller.appspotmail.com,
lvc-project@...uxtesting.org
Subject: Re: [PATCH] comedi: check device's attached status in compat ioctls
On 20/10/2025 18:41, Nikita Zhandarovich wrote:
> Syzbot identified an issue [1] that crashes kernel, seemingly due to
> unexistent callback dev->get_valid_routes(). By all means, this should
> not occur as said callback must always be set to
> get_zero_valid_routes() in __comedi_device_postconfig().
>
> As the crash seems to appear exclusively in i386 kernels, at least,
> judging from [1] reports, the blame lies with compat versions
> of standard IOCTL handlers. Several of them are modified and
> do not use comedi_unlocked_ioctl(). While functionality of these
> ioctls essentially copy their original versions, they do not
> have required sanity check for device's attached status. This,
> in turn, leads to a possibility of calling select IOCTLs on a
> device that has not been properly setup, even via COMEDI_DEVCONFIG.
>
> Doing so on unconfigured devices means that several crucial steps
> are missed, for instance, specifying dev->get_valid_routes()
> callback.
>
> Fix this somewhat crudely by ensuring device's attached status before
> performing any ioctls, improving logic consistency between modern
> and compat functions.
>
> [1] Syzbot report:
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> ...
> CR2: ffffffffffffffd6 CR3: 000000006c717000 CR4: 0000000000352ef0
> Call Trace:
> <TASK>
> get_valid_routes drivers/comedi/comedi_fops.c:1322 [inline]
> parse_insn+0x78c/0x1970 drivers/comedi/comedi_fops.c:1401
> do_insnlist_ioctl+0x272/0x700 drivers/comedi/comedi_fops.c:1594
> compat_insnlist drivers/comedi/comedi_fops.c:3208 [inline]
> comedi_compat_ioctl+0x810/0x990 drivers/comedi/comedi_fops.c:3273
> __do_compat_sys_ioctl fs/ioctl.c:695 [inline]
> __se_compat_sys_ioctl fs/ioctl.c:638 [inline]
> __ia32_compat_sys_ioctl+0x242/0x370 fs/ioctl.c:638
> do_syscall_32_irqs_on arch/x86/entry/syscall_32.c:83 [inline]
> ...
>
> Reported-by: syzbot+ab8008c24e84adee93ff@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ab8008c24e84adee93ff
> Fixes: 3fbfd2223a27 ("comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
> Compile-tested only due to my QEMU setup currently being broken.
> Also, there is no decent syzkaller repro for this problem so
> testing this is a little tricky.
>
> P.S. 'Fixes' tag is technically not correct, as each compat ioctl
> that requires special handling arguments-wise has been modified
> in separate commit. I've opted not to mention each and every one
> of them.
>
> P.P.S. While these multiple identical checks look abhorrent, I've
> decided against changing current approach to compat functions.
>
> drivers/comedi/comedi_fops.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
> index 7e2f2b1a1c36..123e9af2ed44 100644
> --- a/drivers/comedi/comedi_fops.c
> +++ b/drivers/comedi/comedi_fops.c
> @@ -3023,6 +3023,11 @@ static int compat_chaninfo(struct file *file, unsigned long arg)
> chaninfo.rangelist = compat_ptr(chaninfo32.rangelist);
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + return -ENODEV;
> + }
> err = do_chaninfo_ioctl(dev, &chaninfo);
> mutex_unlock(&dev->mutex);
> return err;
> @@ -3044,6 +3049,11 @@ static int compat_rangeinfo(struct file *file, unsigned long arg)
> rangeinfo.range_ptr = compat_ptr(rangeinfo32.range_ptr);
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + return -ENODEV;
> + }
> err = do_rangeinfo_ioctl(dev, &rangeinfo);
> mutex_unlock(&dev->mutex);
> return err;
> @@ -3120,6 +3130,11 @@ static int compat_cmd(struct file *file, unsigned long arg)
> return rc;
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + return -ENODEV;
> + }
> rc = do_cmd_ioctl(dev, &cmd, ©, file);
> mutex_unlock(&dev->mutex);
> if (copy) {
> @@ -3145,6 +3160,11 @@ static int compat_cmdtest(struct file *file, unsigned long arg)
> return rc;
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + return -ENODEV;
> + }
> rc = do_cmdtest_ioctl(dev, &cmd, ©, file);
> mutex_unlock(&dev->mutex);
> if (copy) {
> @@ -3205,6 +3225,12 @@ static int compat_insnlist(struct file *file, unsigned long arg)
> }
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + kfree(insns);
> + return -ENODEV;
> + }
> rc = do_insnlist_ioctl(dev, insns, insnlist32.n_insns, file);
> mutex_unlock(&dev->mutex);
> kfree(insns);
> @@ -3224,6 +3250,11 @@ static int compat_insn(struct file *file, unsigned long arg)
> return rc;
>
> mutex_lock(&dev->mutex);
> + if (!dev->attached) {
> + dev_dbg(dev->class_dev, "no driver attached\n");
> + mutex_unlock(&dev->mutex);
> + return -ENODEV;
> + }
> rc = do_insn_ioctl(dev, &insn, file);
> mutex_unlock(&dev->mutex);
> return rc;
It's OK as is, but may be better to use `else` to avoid duplicate calls
to `mutex_unlock()`, `kfree()`, etc., replacing `return -ENODEV;` with
`rc = -ENODEV;` or `err = -ENODEV;`.
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 )=-
Powered by blists - more mailing lists