Re: [PATCH v3 3/3] driver core: Disable driver overriding by default

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] driver core: ABI breakage by disabling driver_override by default
- [High] driver core: KASLR bypass via %ps on data pointer
- [Medium] driver core: allow_driver_override_setup ignores parameter value
--

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

driver core: Disable driver overriding by default

This patch disallows matching via driver_override unless the driver explicitly
allows it or the safe guard is disabled using the allow_driver_override
kernel parameter. It also un-inlines device_match_driver_override() and moves
it to an exported function.

> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index c51ad96d4de4b..d294c198ab0c3 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -606,6 +606,49 @@ int bus_add_device(struct device *dev)
>  	return error;
>  }
>  
> +static int __read_mostly allow_driver_override;
> +
> +static int __init allow_driver_override_setup(char *str)
> +{
> +	allow_driver_override = 1;

[Severity: Medium]
Does this setup function ignore the user-provided string value? If an
administrator passes allow_driver_override=0 or allow_driver_override=off
on the kernel command line, it appears this will still unconditionally set
allow_driver_override = 1. Should standard boolean parsing such as
kstrtobool(str, &allow_driver_override) be used here instead to honor the
user intent?

> +
> +	return 1;
> +}

[ ... ]

> +int device_match_driver_override(struct device *dev,
> +				 const struct device_driver *drv)
> +{
> +	guard(spinlock)(&dev->driver_override.lock);
> +	if (dev->driver_override.name) {
> +		int ret = !strcmp(dev->driver_override.name, drv->name);
> +
> +		if (ret > 0) {
> +			if (!allow_driver_override && !drv->support_driver_override) {

[Severity: Critical]
Could this unconditionally suppress the override match for drivers that
haven't been updated, and break existing userspace ABIs? Userspace
frameworks like libvirt, QEMU, or DPDK that rely on this feature for PCI
passthrough via vfio-pci or uio_pci_generic might silently fail to bind
devices if they lack the kernel command-line parameter.

> +				pr_info("Suppress driver override binding. Allow %ps to do overriding or boot with allow_driver_override on cmdline\n",
> +					drv);

[Severity: High]
Does applying the %ps specifier to a data pointer (drv) instead of a string
(drv->name) leak a raw kernel address? If the kernel is built without
CONFIG_KALLSYMS_ALL, kallsyms will fail to resolve the data symbol, and the
print routine falls back to printing the unhashed hexadecimal pointer,
which could bypass KASLR when exposed in dmesg.

> +				return -1;
> +			}
> +
> +			add_taint_module(drv->owner,
> +					 TAINT_DRIVER_OVERRIDE, LOCKDEP_STILL_OK);
> +		}
> +
> +		return ret;
> +	}
> +	return -1;
> +}

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