Re: [PATCH 2/2] 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 4 potential issue(s) to consider:

New issues:
- [High] Calling `add_taint_module()` unconditionally dereferences `drv->owner`, causing a kernel panic for built-in drivers where `drv->owner` is `NULL`.
- [Low] The patch breaks the build when `CONFIG_MODULES` is disabled due to a syntactically invalid inline stub for `add_taint_module` introduced in the previous patch.
- [Low] Malformed reStructuredText simple table and outdated `seq 20` script in the tainted-kernels documentation.

Pre-existing issues:
- [High] A race condition in `unbind_store` allows detaching an unintended driver if `dev->driver` changes concurrently.
--

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

driver core: add TAINT_FORCED_BIND for when userspace manually messes with devices and drivers
    
Add a new TAINT_FORCED_BIND taint flag to track when userspace successfully
adds or removes devices from a driver via the sysfs bind and unbind files.
This helps identify when userspace fuzzing or manual interference causes
subsequent kernel crashes.

> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> index 9ead927a37c0f..d4ca8b9e3819e 100644
> --- a/Documentation/admin-guide/tainted-kernels.rst
> +++ b/Documentation/admin-guide/tainted-kernels.rst
> @@ -79,30 +79,31 @@ which bits are set::
>  Table for decoding tainted state
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> -===  ===  ======  ========================================================
> -Bit  Log  Number  Reason that got the kernel tainted
> -===  ===  ======  ========================================================
> +===  ===   ======  ========================================================
> +Bit  Log   Number  Reason that got the kernel tainted
> +===  ===   ======  ========================================================
[ ... ]
> + 19  _/J   524288  userspace used a mutating debug operation in fwctl
> + 20  _/Y  1048576  device was manually bound or unbound from a driver
> +===  ===  =======  ========================================================

[Severity: Low]
The header separator for the Number column is 6 characters wide, but the
footer separator is 7 characters wide. Can this mismatch break Sphinx
reStructuredText table parsing?

Also, does the quick-check script just above this table need to be updated?
It uses seq 20, which fails to cover this newly added 20th taint bit
(requires seq 21).

> 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 is a pre-existing issue, but is there a race condition here?

The check of dev->driver is performed without holding the device lock. If the
driver is detached and a new driver attaches immediately after, could the
subsequent call to device_driver_detach(dev) blindly detach the unintended
new driver?

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

[Severity: High]
For built-in drivers, drv->owner is NULL. Does calling add_taint_module
unconditionally dereference drv->owner, causing a kernel panic when a user
writes to the unbind sysfs file for a statically compiled driver?

[Severity: Low]
Will this unconditional call cause a compilation error when CONFIG_MODULES=n?

The inline stub for add_taint_module in include/linux/module.h omits the
parameter name for enum lockdep_ok, making it syntactically invalid, which
is activated by this new call.

>  		device_driver_detach(dev);
>  		err = count;
>  	}
> @@ -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);

[Severity: High]
Similarly, for built-in drivers where drv->owner is NULL, does this
unconditionally dereference it and cause a kernel panic here as well?

>  		err = device_driver_attach(drv, dev);
>  		if (!err) {
>  			/* success */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-bind_taint-v1-0-52b05f4a965c@linuxfoundation.org?part=2