linux-kernel - Re: [PATCH] comedi: drivers: do not detach device if driv->attach() fails

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb39d407-622b-4480-a146-2e754f74bcbf@mev.co.uk>
Date: Wed, 22 Oct 2025 11:45:15 +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>,
 syzbot+6616bba359cec7a1def1@...kaller.appspotmail.com,
 lvc-project@...uxtesting.org
Subject: Re: [PATCH] comedi: drivers: do not detach device if driv->attach()
 fails

On 21/10/2025 14:16, Nikita Zhandarovich wrote:
> Syzbot identified an issue [1] in comedi_device_attach() that occurs
> when kernel attempts to detach a comedi device via
> comedi_device_detach() even if a driver-specific attach() method
> already failed. Attempts to follow through with detaching the
> device and unregistering the driver trigger a warning.
> 
> Fix this by rearranging cleanup calls so that comedi_device_detach()
> runs only if the device in question has been successfully attached.
> 
> Original idea for this patch belongs to Hillf Danton
> <hdanton@...a.com>.
> 
> [1] Syzbot crash:
> Unexpected driver unregister!
> WARNING: CPU: 0 PID: 5970 at drivers/base/driver.c:273 driver_unregister drivers/base/driver.c:273 [inline]
> WARNING: CPU: 0 PID: 5970 at drivers/base/driver.c:273 driver_unregister+0x90/0xb0 drivers/base/driver.c:270
> ...
> Call Trace:
>   <TASK>
>   comedi_device_detach_locked+0x12f/0xa50 drivers/comedi/drivers.c:207
>   comedi_device_detach+0x67/0xb0 drivers/comedi/drivers.c:215
>   comedi_device_attach+0x43d/0x900 drivers/comedi/drivers.c:1011
>   do_devconfig_ioctl+0x1b1/0x710 drivers/comedi/comedi_fops.c:872
>   comedi_unlocked_ioctl+0x165d/0x2f00 drivers/comedi/comedi_fops.c:2178
>   vfs_ioctl fs/ioctl.c:51 [inline]
>   __do_sys_ioctl fs/ioctl.c:597 [inline]
> ...
> 
> Reported-by: syzbot+6616bba359cec7a1def1@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6616bba359cec7a1def1
> Suggested-by: Hillf Danton <hdanton@...a.com>
> Fixes: 74ece108f9e5 ("staging: comedi: move detach out of post-config")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
> ---
>   drivers/comedi/drivers.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/comedi/drivers.c b/drivers/comedi/drivers.c
> index c9ebaadc5e82..001083f96138 100644
> --- a/drivers/comedi/drivers.c
> +++ b/drivers/comedi/drivers.c
> @@ -1005,10 +1005,13 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>   	dev->board_name = dev->board_ptr ? *(const char **)dev->board_ptr
>   					 : dev->driver->driver_name;
>   	ret = driv->attach(dev, it);
> -	if (ret >= 0)
> +	if (ret >= 0) {
>   		ret = comedi_device_postconfig(dev);
> -	if (ret < 0) {
> -		comedi_device_detach(dev);
> +		if (ret < 0) {
> +			comedi_device_detach(dev);
> +			module_put(driv->module);
> +		}
> +	} else {
>   		module_put(driv->module);
>   	}
>   	/* On success, the driver module count has been incremented. */

Unfortunately, the low-level drivers expect the `->detach()` handler to 
be called to clean up even if the `->attach()` handler returns an error. 
  So this won't work.

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