Syzbot identified an issue [1] in pcl818_ai_cancel(), which stems from
the fact that in case of early device detach via pcl818_detach(),
subdevice dev->read_subdev may not have initialized its pointer to
&struct comedi_async as intended. Thus, any such dereferencing of
&s->async->cmd will lead to general protection fault and kernel crash.

Mitigate this problem by checking in advance whether async struct is
properly present.

Original idea for this patch belongs to Hillf Danton
<hdanton@sina.com>.

Note: as suggested by Ian in [2], there may be a different solution to
improving the way ISRs are set up (and possibly, this flaw as well).
However, until that idea (based on reference counters and completion
structs) comes to fruition, it makes sense to fix this now, if only
to stop kernel crashing so often during syzkaller runs.

[1] Syzbot crash:
Oops: general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
CPU: 1 UID: 0 PID: 6050 Comm: syz.0.18 Not tainted syzkaller #0 PREEMPT(full)
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
RIP: 0010:pcl818_ai_cancel+0x69/0x3f0 drivers/comedi/drivers/pcl818.c:762
...
Call Trace:
 <TASK>
 pcl818_detach+0x66/0xd0 drivers/comedi/drivers/pcl818.c:1115
 comedi_device_detach_locked+0x178/0x750 drivers/comedi/drivers.c:207
 do_devconfig_ioctl drivers/comedi/comedi_fops.c:848 [inline]
 comedi_unlocked_ioctl+0xcde/0x1020 drivers/comedi/comedi_fops.c:2178
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:597 [inline]
...

[2] Ian's suggestion on how to fix interrupt handlers in comedi drivers:
Link: https://lore.kernel.org/all/9c92913c-c04b-4784-9cdc-5d75b10d2ed9@mev.co.uk/

Reported-by: syzbot+fce5d9d5bd067d6fbe9b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fce5d9d5bd067d6fbe9b
Suggested-by: Hillf Danton <hdanton@sina.com>
Fixes: 00aba6e7b565 ("staging: comedi: pcl818: remove 'neverending_ai' from private data")
Cc: stable@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
P.S. Tested with syzkaller reproducers as well as by doing a trial
syzkaller run w/wo the patch.

 drivers/comedi/drivers/pcl818.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/pcl818.c b/drivers/comedi/drivers/pcl818.c
index 4127adcfb229..3c6cfb212492 100644
--- a/drivers/comedi/drivers/pcl818.c
+++ b/drivers/comedi/drivers/pcl818.c
@@ -759,12 +759,15 @@ static int pcl818_ai_cancel(struct comedi_device *dev,
 {
 	struct pcl818_private *devpriv = dev->private;
 	struct comedi_isadma *dma = devpriv->dma;
-	struct comedi_cmd *cmd = &s->async->cmd;
+	struct comedi_cmd *cmd;
 
 	if (!devpriv->ai_cmd_running)
 		return 0;
 
 	if (dma) {
+		if (!s || !s->async)
+			return 0;
+		cmd = &s->async->cmd;
 		if (cmd->stop_src == TRIG_NONE ||
 		    (cmd->stop_src == TRIG_COUNT &&
 		     s->async->scans_done < cmd->stop_arg)) {

On 20/10/2025 17:12, Nikita Zhandarovich wrote:
> Syzbot identified an issue [1] in pcl818_ai_cancel(), which stems from
> the fact that in case of early device detach via pcl818_detach(),
> subdevice dev->read_subdev may not have initialized its pointer to
> &struct comedi_async as intended. Thus, any such dereferencing of
> &s->async->cmd will lead to general protection fault and kernel crash.
> 
> Mitigate this problem by checking in advance whether async struct is
> properly present.
> 
> Original idea for this patch belongs to Hillf Danton
> <hdanton@sina.com>.
> 
> Note: as suggested by Ian in [2], there may be a different solution to
> improving the way ISRs are set up (and possibly, this flaw as well).
> However, until that idea (based on reference counters and completion
> structs) comes to fruition, it makes sense to fix this now, if only
> to stop kernel crashing so often during syzkaller runs.

In this case, it seems to be because the driver allows the device to 
operate without an IRQ, but will not support async commands in that case 
and the subdevice's `async` pointer will be null.  However, the 
`pcl818_ai_cancel()` function called from `pcl818_detach()` assumes that 
the `async` pointer is valid.

The unchecked dereference of the `async` pointer in `pcl818_ai_cancel()` 
seems to have been introduced by 00aba6e7b5653 ("staging: comedi: 
pcl818: remove 'neverending_ai' from private data").


> [1] Syzbot crash:
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] SMP KASAN PTI
> KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
> CPU: 1 UID: 0 PID: 6050 Comm: syz.0.18 Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025
> RIP: 0010:pcl818_ai_cancel+0x69/0x3f0 drivers/comedi/drivers/pcl818.c:762
> ...
> Call Trace:
>   <TASK>
>   pcl818_detach+0x66/0xd0 drivers/comedi/drivers/pcl818.c:1115
>   comedi_device_detach_locked+0x178/0x750 drivers/comedi/drivers.c:207
>   do_devconfig_ioctl drivers/comedi/comedi_fops.c:848 [inline]
>   comedi_unlocked_ioctl+0xcde/0x1020 drivers/comedi/comedi_fops.c:2178
>   vfs_ioctl fs/ioctl.c:51 [inline]
>   __do_sys_ioctl fs/ioctl.c:597 [inline]
> ...
> 
> [2] Ian's suggestion on how to fix interrupt handlers in comedi drivers:
> Link: https://lore.kernel.org/all/9c92913c-c04b-4784-9cdc-5d75b10d2ed9@mev.co.uk/
> 
> Reported-by: syzbot+fce5d9d5bd067d6fbe9b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fce5d9d5bd067d6fbe9b
> Suggested-by: Hillf Danton <hdanton@sina.com>
> Fixes: 00aba6e7b565 ("staging: comedi: pcl818: remove 'neverending_ai' from private data")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> P.S. Tested with syzkaller reproducers as well as by doing a trial
> syzkaller run w/wo the patch.
> 
>   drivers/comedi/drivers/pcl818.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/comedi/drivers/pcl818.c b/drivers/comedi/drivers/pcl818.c
> index 4127adcfb229..3c6cfb212492 100644
> --- a/drivers/comedi/drivers/pcl818.c
> +++ b/drivers/comedi/drivers/pcl818.c
> @@ -759,12 +759,15 @@ static int pcl818_ai_cancel(struct comedi_device *dev,
>   {
>   	struct pcl818_private *devpriv = dev->private;
>   	struct comedi_isadma *dma = devpriv->dma;
> -	struct comedi_cmd *cmd = &s->async->cmd;
> +	struct comedi_cmd *cmd;
>   
>   	if (!devpriv->ai_cmd_running)
>   		return 0;
>   
>   	if (dma) {
> +		if (!s || !s->async)
> +			return 0;
> +		cmd = &s->async->cmd;
>   		if (cmd->stop_src == TRIG_NONE ||
>   		    (cmd->stop_src == TRIG_COUNT &&
>   		     s->async->scans_done < cmd->stop_arg)) {

An alternative fix would be to remove the call to `pcl818_ai_cancel()` 
from `pcl818_detach()`.  If the subdevice was set up to support 
asynchronous commands (non-null `async` pointer), the subdevice's 
`->cancel()` hander function would have already been called via 
`comedi_device_detach_locked()` ==> `comedi_device_cancel_all()` ==> 
`do_cancel()` before the driver's `->detach()` handler is called.

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