| Message ID | 20220223191310.347669-1-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix broken usage of driver_override (and kfree of static memory) | expand |
On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote: > Several core drivers and buses expect that driver_override is a > dynamically allocated memory thus later they can kfree() it. > > However such assumption is not documented, there were in the past and > there are already users setting it to a string literal. This leads to > kfree() of static memory during device release (e.g. in error paths or > during unbind): > > kernel BUG at ../mm/slub.c:3960! > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM > ... > (kfree) from [<c058da50>] (platform_device_release+0x88/0xb4) > (platform_device_release) from [<c0585be0>] (device_release+0x2c/0x90) > (device_release) from [<c0a69050>] (kobject_put+0xec/0x20c) > (kobject_put) from [<c0f2f120>] (exynos5_clk_probe+0x154/0x18c) > (exynos5_clk_probe) from [<c058de70>] (platform_drv_probe+0x6c/0xa4) > (platform_drv_probe) from [<c058b7ac>] (really_probe+0x280/0x414) > (really_probe) from [<c058baf4>] (driver_probe_device+0x78/0x1c4) > (driver_probe_device) from [<c0589854>] (bus_for_each_drv+0x74/0xb8) > (bus_for_each_drv) from [<c058b48c>] (__device_attach+0xd4/0x16c) > (__device_attach) from [<c058a638>] (bus_probe_device+0x88/0x90) > (bus_probe_device) from [<c05871fc>] (device_add+0x3dc/0x62c) > (device_add) from [<c075ff10>] (of_platform_device_create_pdata+0x94/0xbc) > (of_platform_device_create_pdata) from [<c07600ec>] (of_platform_bus_create+0x1a8/0x4fc) > (of_platform_bus_create) from [<c0760150>] (of_platform_bus_create+0x20c/0x4fc) > (of_platform_bus_create) from [<c07605f0>] (of_platform_populate+0x84/0x118) > (of_platform_populate) from [<c0f3c964>] (of_platform_default_populate_init+0xa0/0xb8) > (of_platform_default_populate_init) from [<c01031f8>] (do_one_initcall+0x8c/0x404) > (do_one_initcall) from [<c0f012c0>] (kernel_init_freeable+0x3d0/0x4d8) > (kernel_init_freeable) from [<c0a7def0>] (kernel_init+0x8/0x114) > (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20) > > Provide a helper which clearly documents the usage of driver_override. > This will allow later to reuse the helper and reduce amount of > duplicated code. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++ > drivers/base/platform.c | 24 +++--------------- > include/linux/device/driver.h | 1 + > include/linux/platform_device.h | 6 ++++- > 4 files changed, 54 insertions(+), 21 deletions(-) > > diff --git a/drivers/base/driver.c b/drivers/base/driver.c > index 8c0d33e182fd..79efe51bb4c0 100644 > --- a/drivers/base/driver.c > +++ b/drivers/base/driver.c > @@ -30,6 +30,50 @@ static struct device *next_device(struct klist_iter *i) > return dev; > } > > +/* > + * set_driver_override() - Helper to set or clear driver override. > + * @dev: Device to change > + * @override: Address of string to change (e.g. &device->driver_override); > + * The contents will be freed and hold newly allocated override. > + * @s: NULL terminated string, new driver name to force a match, pass empty Don't you mean NUL terminated? Do all callers really validate that it's NUL terminated? > + * string to clear it > + * > + * Helper to setr or clear driver override in a device, intended for the cases set? > + * when the driver_override field is allocated by driver/bus code. > + * > + * Returns: 0 on success or a negative error code on failure. > + */ > +int driver_set_override(struct device *dev, char **override, const char *s) > +{ > + char *new, *old, *cp; > + > + if (!dev || !override || !s) > + return -EINVAL; > + > + new = kstrndup(s, strlen(s), GFP_KERNEL); what's the point of this kstrndup then? why not just kstrdup? > + if (!new) > + return -ENOMEM; > + > + cp = strchr(new, '\n'); > + if (cp) > + *cp = '\0'; > + > + device_lock(dev); > + old = *override; > + if (strlen(new)) { We are re-reading the string like 3 times here. > + *override = new; > + } else { > + kfree(new); > + *override = NULL; > + } > + device_unlock(dev); > + > + kfree(old); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(driver_set_override); > + > /** > * driver_for_each_device - Iterator for devices bound to a driver. > * @drv: Driver we're iterating. > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 6cb04ac48bf0..d8853b32ea10 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -1275,31 +1275,15 @@ static ssize_t driver_override_store(struct device *dev, > const char *buf, size_t count) > { > struct platform_device *pdev = to_platform_device(dev); > - char *driver_override, *old, *cp; > + int ret; > > /* We need to keep extra room for a newline */ > if (count >= (PAGE_SIZE - 1)) > return -EINVAL; Given everyone seems to repeat this check, how about passing in count and doing the validation in the helper? We will then also avoid the need to do strlen and strchr. > - driver_override = kstrndup(buf, count, GFP_KERNEL); > - if (!driver_override) > - return -ENOMEM; > - > - cp = strchr(driver_override, '\n'); > - if (cp) > - *cp = '\0'; > - > - device_lock(dev); > - old = pdev->driver_override; > - if (strlen(driver_override)) { > - pdev->driver_override = driver_override; > - } else { > - kfree(driver_override); > - pdev->driver_override = NULL; > - } > - device_unlock(dev); > - > - kfree(old); > + ret = driver_set_override(dev, &pdev->driver_override, buf); > + if (ret) > + return ret; > > return count; > } > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index 15e7c5e15d62..81c0d9f65a40 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -151,6 +151,7 @@ extern int __must_check driver_create_file(struct device_driver *driver, > extern void driver_remove_file(struct device_driver *driver, > const struct driver_attribute *attr); > > +int driver_set_override(struct device *dev, char **override, const char *s); > extern int __must_check driver_for_each_device(struct device_driver *drv, > struct device *start, > void *data, > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 7c96f169d274..37ac14459499 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -31,7 +31,11 @@ struct platform_device { > struct resource *resource; > > const struct platform_device_id *id_entry; > - char *driver_override; /* Driver name to force a match */ > + /* > + * Driver name to force a match, use > + * driver_set_override() to set or clear it. > + */ > + char *driver_override; > > /* MFD cell pointer */ > struct mfd_cell *mfd_cell; > -- > 2.32.0
On 23/02/2022 22:33, Michael S. Tsirkin wrote: > On Wed, Feb 23, 2022 at 08:13:00PM +0100, Krzysztof Kozlowski wrote: >> Several core drivers and buses expect that driver_override is a >> dynamically allocated memory thus later they can kfree() it. >> >> However such assumption is not documented, there were in the past and >> there are already users setting it to a string literal. This leads to >> kfree() of static memory during device release (e.g. in error paths or >> during unbind): >> >> kernel BUG at ../mm/slub.c:3960! >> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM >> ... >> (kfree) from [<c058da50>] (platform_device_release+0x88/0xb4) >> (platform_device_release) from [<c0585be0>] (device_release+0x2c/0x90) >> (device_release) from [<c0a69050>] (kobject_put+0xec/0x20c) >> (kobject_put) from [<c0f2f120>] (exynos5_clk_probe+0x154/0x18c) >> (exynos5_clk_probe) from [<c058de70>] (platform_drv_probe+0x6c/0xa4) >> (platform_drv_probe) from [<c058b7ac>] (really_probe+0x280/0x414) >> (really_probe) from [<c058baf4>] (driver_probe_device+0x78/0x1c4) >> (driver_probe_device) from [<c0589854>] (bus_for_each_drv+0x74/0xb8) >> (bus_for_each_drv) from [<c058b48c>] (__device_attach+0xd4/0x16c) >> (__device_attach) from [<c058a638>] (bus_probe_device+0x88/0x90) >> (bus_probe_device) from [<c05871fc>] (device_add+0x3dc/0x62c) >> (device_add) from [<c075ff10>] (of_platform_device_create_pdata+0x94/0xbc) >> (of_platform_device_create_pdata) from [<c07600ec>] (of_platform_bus_create+0x1a8/0x4fc) >> (of_platform_bus_create) from [<c0760150>] (of_platform_bus_create+0x20c/0x4fc) >> (of_platform_bus_create) from [<c07605f0>] (of_platform_populate+0x84/0x118) >> (of_platform_populate) from [<c0f3c964>] (of_platform_default_populate_init+0xa0/0xb8) >> (of_platform_default_populate_init) from [<c01031f8>] (do_one_initcall+0x8c/0x404) >> (do_one_initcall) from [<c0f012c0>] (kernel_init_freeable+0x3d0/0x4d8) >> (kernel_init_freeable) from [<c0a7def0>] (kernel_init+0x8/0x114) >> (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20) >> >> Provide a helper which clearly documents the usage of driver_override. >> This will allow later to reuse the helper and reduce amount of >> duplicated code. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >> --- >> drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++ >> drivers/base/platform.c | 24 +++--------------- >> include/linux/device/driver.h | 1 + >> include/linux/platform_device.h | 6 ++++- >> 4 files changed, 54 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/base/driver.c b/drivers/base/driver.c >> index 8c0d33e182fd..79efe51bb4c0 100644 >> --- a/drivers/base/driver.c >> +++ b/drivers/base/driver.c >> @@ -30,6 +30,50 @@ static struct device *next_device(struct klist_iter *i) >> return dev; >> } >> >> +/* >> + * set_driver_override() - Helper to set or clear driver override. >> + * @dev: Device to change >> + * @override: Address of string to change (e.g. &device->driver_override); >> + * The contents will be freed and hold newly allocated override. >> + * @s: NULL terminated string, new driver name to force a match, pass empty > > Don't you mean NUL terminated? Yeah, NUL. > Do all callers really validate that it's NUL terminated? Good point, the callers use it in device attributes (sysfs) only, so it might come non-NUL. Previously this was solved by kstrndup() which is always terminating the string. > >> + * string to clear it >> + * >> + * Helper to setr or clear driver override in a device, intended for the cases > > set? D'oh! > >> + * when the driver_override field is allocated by driver/bus code. >> + * >> + * Returns: 0 on success or a negative error code on failure. >> + */ >> +int driver_set_override(struct device *dev, char **override, const char *s) >> +{ >> + char *new, *old, *cp; >> + >> + if (!dev || !override || !s) >> + return -EINVAL; >> + >> + new = kstrndup(s, strlen(s), GFP_KERNEL); > > > what's the point of this kstrndup then? why not just kstrdup? Thanks, it's a copy-paste. Useless now, but I'll pass the count directly from the callers and then this will be NULL-terminating it. > >> + if (!new) >> + return -ENOMEM; >> + >> + cp = strchr(new, '\n'); >> + if (cp) >> + *cp = '\0'; >> + >> + device_lock(dev); >> + old = *override; >> + if (strlen(new)) { > > We are re-reading the string like 3 times here. Yep, the same in old code. I guess we could compare just pointers - whether 'cp' is not NULL and different than 's'. > >> + *override = new; >> + } else { >> + kfree(new); >> + *override = NULL; >> + } >> + device_unlock(dev); >> + >> + kfree(old); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(driver_set_override); >> + >> /** >> * driver_for_each_device - Iterator for devices bound to a driver. >> * @drv: Driver we're iterating. >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index 6cb04ac48bf0..d8853b32ea10 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -1275,31 +1275,15 @@ static ssize_t driver_override_store(struct device *dev, >> const char *buf, size_t count) >> { >> struct platform_device *pdev = to_platform_device(dev); >> - char *driver_override, *old, *cp; >> + int ret; >> >> /* We need to keep extra room for a newline */ >> if (count >= (PAGE_SIZE - 1)) >> return -EINVAL; > > Given everyone seems to repeat this check, how about passing > in count and doing the validation in the helper? Good idea. > We will then also avoid the need to do strlen and strchr. The strlen() could be removed, but the strchr() should stay. What solution do you have in mind to remove strchr()? Thanks for review. Best regards, Krzysztof
Hi, This is a continuation of my old patchset from 2019. [1] Back then, few drivers set driver_override wrong. I fixed Exynos in a different way after discussions. QCOM NGD was not fixed and a new user appeared - IMX SCU. It seems "char *" in driver_override looks too consty, so we tend to make a mistake of storing there string literals. Changes of latest since v1 (not the old 2019 solution): ======================================================= https://lore.kernel.org/all/708eabb1-7b35-d525-d4c3-451d4a3de84f@rasmusvillemoes.dk/ 1. Add helper for setting driver_override. 2. Use the helper. Dependencies (and stable): ========================== 1. All patches, including last three fixes, depend on first patch introducing the helper. 2. The last three commits - fixes - are probably not backportable directly, because of this dependency. I don't know how to express it here, since stable-kernel-rules.rst mentions only commits as possible dependencies. [1] https://lore.kernel.org/all/1550484960-2392-3-git-send-email-krzk@kernel.org/ Best regards, Krzysztof Krzysztof Kozlowski (11): driver: platform: add and use helper for safer setting of driver_override amba: use helper for safer setting of driver_override fsl-mc: use helper for safer setting of driver_override hv: vmbus: use helper for safer setting of driver_override pci: use helper for safer setting of driver_override s390: cio: use helper for safer setting of driver_override spi: use helper for safer setting of driver_override vdpa: use helper for safer setting of driver_override clk: imx: scu: fix kfree() of static memory on setting driver_override slimbus: qcom-ngd: fix kfree() of static memory on setting driver_override rpmsg: fix kfree() of static memory on setting driver_override drivers/amba/bus.c | 24 +++--------------- drivers/base/driver.c | 44 +++++++++++++++++++++++++++++++++ drivers/base/platform.c | 24 +++--------------- drivers/bus/fsl-mc/fsl-mc-bus.c | 22 +++-------------- drivers/clk/imx/clk-scu.c | 7 +++++- drivers/hv/vmbus_drv.c | 24 +++--------------- drivers/pci/pci-sysfs.c | 24 +++--------------- drivers/rpmsg/rpmsg_internal.h | 13 ++++++++-- drivers/rpmsg/rpmsg_ns.c | 14 +++++++++-- drivers/s390/cio/css.c | 24 +++--------------- drivers/slimbus/qcom-ngd-ctrl.c | 12 ++++++++- drivers/spi/spi.c | 20 +++------------ drivers/vdpa/vdpa.c | 25 +++---------------- include/linux/device/driver.h | 1 + include/linux/platform_device.h | 6 ++++- include/linux/spi/spi.h | 2 +- 16 files changed, 123 insertions(+), 163 deletions(-)