1. Patchew
  2. linux
  3. View series

[PATCH] comedi: check device's attached status in compat ioctls

Nikita Zhandarovich posted 1 patch 3 months, 2 weeks ago
There is a newer version of this series
drivers/comedi/comedi_fops.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
[PATCH] comedi: check device's attached status in compat ioctls
Posted by Nikita Zhandarovich 3 months, 2 weeks ago 
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@syzkaller.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@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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, &copy, 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, &copy, 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;
Re: [PATCH] comedi: check device's attached status in compat ioctls
Posted by Ian Abbott 3 months, 2 weeks ago 
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@syzkaller.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@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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, &copy, 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, &copy, 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@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 )=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.