linux-kernel - Re: [PATCH] comedi: pcl818: fix null-ptr-deref in pcl818_ai

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: <24d288e5-9e98-49c0-964d-40476aadc267@mev.co.uk>
Date: Wed, 22 Oct 2025 11:02:10 +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, Hillf Danton <hdanton@...a.com>,
 lvc-project@...uxtesting.org
Subject: Re: [PATCH] comedi: pcl818: fix null-ptr-deref in pcl818_ai_cancel()

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@...a.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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fce5d9d5bd067d6fbe9b
> Suggested-by: Hillf Danton <hdanton@...a.com>
> Fixes: 00aba6e7b565 ("staging: comedi: pcl818: remove 'neverending_ai' from private data")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.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@....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 )=-