Re: [PATCH v4 3/3] driver core: add TAINT_FORCED_BIND for when userspace manually messes with devices and drivers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The documentation incorrectly states that the kernel is tainted only when a device is 'successfully' bound or removed, but the code taints the kernel before the operation, even if it fails.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a TOCTOU race in `unbind_store` allows detaching the wrong driver from a device due to a lockless check followed by an unconditional detach.
--

commit ab4daed0c54fee77e3c6d683fc2a55fadc4aaef3
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

driver core: add TAINT_FORCED_BIND for when userspace manually messes with devices and drivers

This commit adds a new taint flag, TAINT_FORCED_BIND, which is set when
userspace manually binds or unbinds a device from a driver via sysfs. The
taint is added before the callback runs to ensure oops reports reflect the
attempted manual manipulation. This aims to help kernel developers identify
when crashes result from abnormal userspace driver binding operations.

> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> index 9ead927a37c0f..abbf5e3dd7495 100644
> --- a/Documentation/admin-guide/tainted-kernels.rst
> +++ b/Documentation/admin-guide/tainted-kernels.rst

[ ... ]

> @@ -189,3 +190,6 @@ More detailed explanation for tainting
>   19) ``J`` if userspace opened /dev/fwctl/* and performed a FWTCL_RPC_DEBUG_WRITE
>       to use the devices debugging features. Device debugging features could
>       cause the device to malfunction in undefined ways.
> +
> + 20) ``Y`` If userspace wrote to the `bind` or `unbind` sysfs files and
> +     successfully bound or removed a device from a driver.

[Severity: Medium]
The documentation here explicitly says "successfully bound or removed", but
looking at the code in bind_store() and unbind_store() in drivers/base/bus.c,
the kernel is tainted before the operation is attempted. Does this mean the
kernel remains permanently tainted even if the attach or detach operation
fails, making this documentation inaccurate?

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index d17bd91490ee4..c51ad96d4de4b 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c

[ ... ]

> @@ -241,6 +242,7 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf,
>  
>  	dev = bus_find_device_by_name(bus, NULL, buf);
>  	if (dev && dev->driver == drv) {

[Severity: High]
This isn't a bug introduced by this patch, but looking at unbind_store(), is
there a TOCTOU race window here?

The check dev->driver == drv evaluates without holding a lock. If Thread A
evaluates this check, and concurrently Thread B unbinds the driver and binds a
new driver to the same device:

Thread A
unbind_store()
    if (dev && dev->driver == drv) { // true

Thread B
unbinds original driver
binds new unrelated driver

Thread A then continues:

> +		add_taint_module(drv->owner, TAINT_FORCED_BIND, LOCKDEP_STILL_OK);
>  		device_driver_detach(dev);

and calls device_driver_detach(dev). Since device_driver_detach() in
drivers/base/dd.c calls device_release_driver_internal(dev, NULL, dev->parent),
the NULL passed as the driver argument bypasses the specific driver match
validation inside device_release_driver_internal.

Does this mean the new, unrelated driver could be unconditionally detached by
Thread A?

>  		err = count;
>  	}
>  	put_device(dev);
> @@ -264,6 +266,7 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf,
>  
>  	dev = bus_find_device_by_name(bus, NULL, buf);
>  	if (dev && driver_match_device(drv, dev)) {
> +		add_taint_module(drv->owner, TAINT_FORCED_BIND, LOCKDEP_STILL_OK);
>  		err = device_driver_attach(drv, dev);

[Severity: Medium]
This matches the observation above: if device_driver_attach(drv, dev) fails,
the kernel is already tainted, which contradicts the documentation stating it
only happens on success. Could the documentation be updated to clarify that
the taint is applied on the attempt, regardless of success?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-bind_taint-v4-0-eadf8a090903@linuxfoundation.org?part=3