comedi: drivers: do not detach device if driv->attach() fails

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

Posted by Nikita Zhandarovich 3 months, 2 weeks ago

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@sina.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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6616bba359cec7a1def1
Suggested-by: Hillf Danton <hdanton@sina.com>
Fixes: 74ece108f9e5 ("staging: comedi: move detach out of post-config")
Cc: stable@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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. */

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

Posted by Ian Abbott 3 months, 2 weeks ago

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@sina.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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6616bba359cec7a1def1
> Suggested-by: Hillf Danton <hdanton@sina.com>
> Fixes: 74ece108f9e5 ("staging: comedi: move detach out of post-config")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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@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 )=-

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

Posted by Ian Abbott 3 months, 2 weeks ago

On 22/10/2025 11:45, Ian Abbott wrote:
> 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@sina.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@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=6616bba359cec7a1def1
>> Suggested-by: Hillf Danton <hdanton@sina.com>
>> Fixes: 74ece108f9e5 ("staging: comedi: move detach out of post-config")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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.
> 

The problem seems to be the "c6xdigio" driver 
("drivers/comedi/drivers/c6digio.c"). Its comedi `->attach()` handler 
`c6digio_attach()` can return an error before the call to 
`pnp_register_driver()`.  Also, it does not check the return value from 
`pnp_register_driver()`.  On error, comedi will call the `->detach()` 
handler `c6xdigio_detach()` which calls `pnp_unregister_driver()` 
unconditionally, leading to the warning reported by Syzbot.

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

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

Posted by Жандарович Никита Игоревич 3 months, 2 weeks ago


> -----Original Message-----
> From: Ian Abbott <abbotti@mev.co.uk>
> Sent: Wednesday, October 22, 2025 2:52 PM
> To: Жандарович Никита Игоревич <n.zhandarovich@fintech.ru>; H Hartley
> Sweeten <hsweeten@visionengravers.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> kernel@vger.kernel.org; Hillf Danton <hdanton@sina.com>;
> syzbot+6616bba359cec7a1def1@syzkaller.appspotmail.com; lvc-
> project@linuxtesting.org
> Subject: Re: [PATCH] comedi: drivers: do not detach device if driv->attach()
> fails
> 
> On 22/10/2025 11:45, Ian Abbott wrote:
> > 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@sina.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@syzkaller.appspotmail.com
> >> Closes:
> https://syzkaller.appspot.com/bug?extid=6616bba359cec7a1def1
> >> Suggested-by: Hillf Danton <hdanton@sina.com>
> >> Fixes: 74ece108f9e5 ("staging: comedi: move detach out of post-config")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.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.
> >
> 
> The problem seems to be the "c6xdigio" driver
> ("drivers/comedi/drivers/c6digio.c"). Its comedi `->attach()` handler
> `c6digio_attach()` can return an error before the call to
> `pnp_register_driver()`.  Also, it does not check the return value from
> `pnp_register_driver()`.  On error, comedi will call the `->detach()`
> handler `c6xdigio_detach()` which calls `pnp_unregister_driver()`
> unconditionally, leading to the warning reported by Syzbot.
> 

Yep, you are absolutely right. I saw that c6digio must be to blame but could 
not see that pnp stuff needed more finesse. I'll defer to you on this as I can
see your patch being already tested by syzkaller.

Thanks!

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