Re: [PATCH v2 2/2] Add TAINT_DRIVER_OVERRIDE for usage of driver_override

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] sysfs: driver_override taint is bypassed by subsystem-specific callbacks
- [Medium] sysfs: restoring default driver binding unconditionally taints the kernel
- [Low] trace: TAINT_DRIVER_OVERRIDE bit in module_load event is dead code
- [Low] panic: off-by-one calculation in verbose taint string comment
--

commit a5915871a0d2d9897b48b1d7e230f229bd1e51f7
Author: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Add TAINT_DRIVER_OVERRIDE for usage of driver_override

This commit introduces a new taint flag, TAINT_DRIVER_OVERRIDE, to track when 
userspace manually triggers driver binding via the driver_override sysfs file. 
It includes updates to the core bus sysfs callbacks, documentation, and the 
module tracepoint definitions.

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index c51ad96d4de4b..7d5dc016a457c 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -513,6 +513,7 @@ static ssize_t driver_override_store(struct device *dev,
>  {
>  	int ret;
>  
> +	add_taint_module(NULL, TAINT_DRIVER_OVERRIDE, LOCKDEP_STILL_OK);

[Severity: Medium]
Does this unconditionally taint the kernel even when restoring the default
driver binding?

If userspace writes an empty string or a newline to clear a previous override,
the kernel is permanently marked as tainted. This seems to contradict the
intent of only tainting when forcing a driver.

[Severity: Medium]
Is this taint bypassed by subsystem-specific callbacks?

Subsystems like SPI and AP queue define their own custom driver_override_store
callbacks which directly call __device_set_driver_override() and completely
skip this new taint injection.

For example, in drivers/spi/spi.c:

static ssize_t driver_override_store(...)
{
    ...
    ret = __device_set_driver_override(dev, buf, count);
    ...
}

Should this taint be applied within __device_set_driver_override() instead
to catch these custom subsystem sysfs callbacks?

>  	ret = __device_set_driver_override(dev, buf, count);
>  	if (ret)
>  		return ret;

[ ... ]

> diff --git a/include/trace/events/module.h b/include/trace/events/module.h
> index 19df3e39bba43..c7cdb1f53bc64 100644
> --- a/include/trace/events/module.h
> +++ b/include/trace/events/module.h
> @@ -27,7 +27,8 @@ struct module;
>  	{ (1UL << TAINT_FORCED_MODULE),		"F" },		\
>  	{ (1UL << TAINT_CRAP),			"C" },		\
>  	{ (1UL << TAINT_UNSIGNED_MODULE),	"E" },		\
> -	{ (1UL << TAINT_FORCED_BIND),		"Y" })
> +	{ (1UL << TAINT_FORCED_BIND),		"Y" },		\
> +	{ (1UL << TAINT_DRIVER_OVERRIDE),	"Z" })

[Severity: Low]
Is this trace bit reachable?

Since add_taint_module(NULL, TAINT_DRIVER_OVERRIDE, LOCKDEP_STILL_OK) is
called with NULL rather than a struct module, the taint is only applied
globally to the kernel.

Because the bit is never set in any individual module's taints bitmask, adding
this to the module trace event flags appears to be dead code.

>  
>  TRACE_EVENT(module_load,

[ ... ]

> diff --git a/kernel/panic.c b/kernel/panic.c
> index b824b68fcb084..f5476a61f6f39 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c

[ ... ]

> @@ -862,7 +863,7 @@ static void print_tainted_seq(struct seq_buf *s, bool verbose)
>   * exact size is allocated dynamically; the initial buffer remains
>   * as a fallback if allocation fails.
>   *
> - * The verbose taint string currently requires up to 344 characters.
> + * The verbose taint string currently requires up to 364 characters.

[Severity: Low]
Does this undercount the required buffer size by one character?

The newly added DRIVER_OVERRIDE flag requires exactly 21 characters (15 for
the descriptor "DRIVER_OVERRIDE", 2 for the ", " separator, and 4 for the
format "[Z]=").

The true required length without the null terminator would be 344 + 21 = 365
characters. INIT_TAINT_BUF_MAX is sufficiently padded (370) so this doesn't
cause an overflow, but the documentation appears to have an off-by-one.

>   */
>  #define INIT_TAINT_BUF_MAX 370
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790495516.git.u.kleine-koenig@baylibre.com?part=2