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 <[email protected]> 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/[email protected]?part=3
