Skip to content

Commit f6e629d

Browse files
Nikita Zhandarovichgregkh
authored andcommitted
comedi: check device's attached status in compat ioctls
commit 0de7d9c upstream. 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: [email protected] Closes: https://syzkaller.appspot.com/bug?extid=ab8008c24e84adee93ff Fixes: 3fbfd22 ("comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat") Cc: stable <[email protected]> Reviewed-by: Ian Abbott <[email protected]> Signed-off-by: Nikita Zhandarovich <[email protected]> Link: https://patch.msgid.link/[email protected] Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 8952bc1 commit f6e629d

File tree

1 file changed

+36
-6
lines changed

1 file changed

+36
-6
lines changed

drivers/comedi/comedi_fops.c

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2971,7 +2971,12 @@ static int compat_chaninfo(struct file *file, unsigned long arg)
29712971
chaninfo.rangelist = compat_ptr(chaninfo32.rangelist);
29722972

29732973
mutex_lock(&dev->mutex);
2974-
err = do_chaninfo_ioctl(dev, &chaninfo);
2974+
if (!dev->attached) {
2975+
dev_dbg(dev->class_dev, "no driver attached\n");
2976+
err = -ENODEV;
2977+
} else {
2978+
err = do_chaninfo_ioctl(dev, &chaninfo);
2979+
}
29752980
mutex_unlock(&dev->mutex);
29762981
return err;
29772982
}
@@ -2992,7 +2997,12 @@ static int compat_rangeinfo(struct file *file, unsigned long arg)
29922997
rangeinfo.range_ptr = compat_ptr(rangeinfo32.range_ptr);
29932998

29942999
mutex_lock(&dev->mutex);
2995-
err = do_rangeinfo_ioctl(dev, &rangeinfo);
3000+
if (!dev->attached) {
3001+
dev_dbg(dev->class_dev, "no driver attached\n");
3002+
err = -ENODEV;
3003+
} else {
3004+
err = do_rangeinfo_ioctl(dev, &rangeinfo);
3005+
}
29963006
mutex_unlock(&dev->mutex);
29973007
return err;
29983008
}
@@ -3068,7 +3078,12 @@ static int compat_cmd(struct file *file, unsigned long arg)
30683078
return rc;
30693079

30703080
mutex_lock(&dev->mutex);
3071-
rc = do_cmd_ioctl(dev, &cmd, &copy, file);
3081+
if (!dev->attached) {
3082+
dev_dbg(dev->class_dev, "no driver attached\n");
3083+
rc = -ENODEV;
3084+
} else {
3085+
rc = do_cmd_ioctl(dev, &cmd, &copy, file);
3086+
}
30723087
mutex_unlock(&dev->mutex);
30733088
if (copy) {
30743089
/* Special case: copy cmd back to user. */
@@ -3093,7 +3108,12 @@ static int compat_cmdtest(struct file *file, unsigned long arg)
30933108
return rc;
30943109

30953110
mutex_lock(&dev->mutex);
3096-
rc = do_cmdtest_ioctl(dev, &cmd, &copy, file);
3111+
if (!dev->attached) {
3112+
dev_dbg(dev->class_dev, "no driver attached\n");
3113+
rc = -ENODEV;
3114+
} else {
3115+
rc = do_cmdtest_ioctl(dev, &cmd, &copy, file);
3116+
}
30973117
mutex_unlock(&dev->mutex);
30983118
if (copy) {
30993119
err = put_compat_cmd(compat_ptr(arg), &cmd);
@@ -3153,7 +3173,12 @@ static int compat_insnlist(struct file *file, unsigned long arg)
31533173
}
31543174

31553175
mutex_lock(&dev->mutex);
3156-
rc = do_insnlist_ioctl(dev, insns, insnlist32.n_insns, file);
3176+
if (!dev->attached) {
3177+
dev_dbg(dev->class_dev, "no driver attached\n");
3178+
rc = -ENODEV;
3179+
} else {
3180+
rc = do_insnlist_ioctl(dev, insns, insnlist32.n_insns, file);
3181+
}
31573182
mutex_unlock(&dev->mutex);
31583183
kfree(insns);
31593184
return rc;
@@ -3172,7 +3197,12 @@ static int compat_insn(struct file *file, unsigned long arg)
31723197
return rc;
31733198

31743199
mutex_lock(&dev->mutex);
3175-
rc = do_insn_ioctl(dev, &insn, file);
3200+
if (!dev->attached) {
3201+
dev_dbg(dev->class_dev, "no driver attached\n");
3202+
rc = -ENODEV;
3203+
} else {
3204+
rc = do_insn_ioctl(dev, &insn, file);
3205+
}
31763206
mutex_unlock(&dev->mutex);
31773207
return rc;
31783208
}

0 commit comments

Comments
 (0)