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

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

Hello,

On Mon, Sep 28, 2026 at 04:57:50PM +0000, sashiko-bot@kernel.org wrote:
> > 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?

I copied from sysrq_always_enabled_setup(), I guess both could indeed
benefit from kstrtobool().

> > +	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.

The idea is to mark the relevant drivers with .support_driver_override =
1 before this patch is applied to minimize the fallout. I added the two
drivers pointed out to my list to check.
 
> > +				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.

I wasn't aware of that leak. Is that a real thing?
 
Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]