[alsa-devel] [RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override
Hi,
The problem =========== Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value.
However the users are unaware of this and set the driver_override like:
pdev->driver_override = "exynos5-subcmu";
which obviously leads to error.
Solution ======== I provided simple helper for platform device. If this approach is acceptable, I can convert also other buses, like AMBA, SPI.
Dependencies and pick up order ============================== Patch 1: please pick it as is through clock tree Patch 3: Depends on patch 1 (merge conflict) and 2. Patch 4: Depends on patch 2.
Best regards, Krzysztof
Krzysztof Kozlowski (4): clk: samsung: exynos5: Fix possible NULL pointer exception on platform_device_alloc() failure driver: platform: Provide helper for safer setting of driver_override clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override
drivers/base/platform.c | 63 ++++++++++++++++++++++---------- drivers/clk/samsung/clk-exynos5-subcmu.c | 12 ++++-- drivers/slimbus/qcom-ngd-ctrl.c | 2 +- include/linux/platform_device.h | 9 ++++- 4 files changed, 61 insertions(+), 25 deletions(-)
During initialization of subdevices if platform_device_alloc() failed, returned NULL pointer will be later dereferenced. Add proper error paths to exynos5_clk_register_subcmu(). The return value of this function is still ignored because at this stage of init there is nothing we can do.
Signed-off-by: Krzysztof Kozlowski krzk@kernel.org --- drivers/clk/samsung/clk-exynos5-subcmu.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos5-subcmu.c b/drivers/clk/samsung/clk-exynos5-subcmu.c index 93306283d764..d07ef26bd052 100644 --- a/drivers/clk/samsung/clk-exynos5-subcmu.c +++ b/drivers/clk/samsung/clk-exynos5-subcmu.c @@ -136,15 +136,21 @@ static int __init exynos5_clk_register_subcmu(struct device *parent, { struct of_phandle_args genpdspec = { .np = pd_node }; struct platform_device *pdev; + int ret;
pdev = platform_device_alloc(info->pd_name, -1); + if (!pdev) + return -ENOMEM; + pdev->dev.parent = parent; pdev->driver_override = "exynos5-subcmu"; platform_set_drvdata(pdev, (void *)info); of_genpd_add_device(&genpdspec, &pdev->dev); - platform_device_add(pdev); + ret = platform_device_add(pdev); + if (ret) + platform_device_put(pdev);
- return 0; + return ret; }
static int __init exynos5_clk_probe(struct platform_device *pdev)
The core platform driver expects that driver_override is an dynamically allocated memory so later it can kfree() it.
However such assumption is not documented and there are already users setting it to a const memory. This leads to kfree() of const 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.
Signed-off-by: Krzysztof Kozlowski krzk@kernel.org --- drivers/base/platform.c | 63 ++++++++++++++++++++++++++++------------- include/linux/platform_device.h | 9 +++++- 2 files changed, 51 insertions(+), 21 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 0d3611cd1b3b..1458903a33c8 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -730,6 +730,45 @@ int __init_or_module __platform_driver_probe(struct platform_driver *drv, } EXPORT_SYMBOL_GPL(__platform_driver_probe);
+/* + * platform_set_driver_override() - Helper to set or clear driver override. + * @pdev: platform device + * @override: Driver name to force a match, pass empty string to clear it + * + * Returns: 0 on success or a negative error code on failure. + */ +int platform_set_driver_override(struct platform_device *pdev, + const char *override) +{ + struct device *dev = &pdev->dev; + char *driver_override, *old, *cp; + + if (!pdev || !override) + return -EINVAL; + + driver_override = kstrndup(override, strlen(override), 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); + + return 0; +} + /** * __platform_create_bundle - register driver and create corresponding device * @driver: platform driver structure @@ -879,31 +918,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;
- 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 = platform_set_driver_override(pdev, buf); + if (ret) + return ret;
return count; } diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index c7c081dc6034..1dd06fed3306 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -29,7 +29,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 + * platform_set_driver_override() to set or clear it. + */ + char *driver_override;
/* MFD cell pointer */ struct mfd_cell *mfd_cell; @@ -220,6 +224,9 @@ static inline void platform_set_drvdata(struct platform_device *pdev, dev_set_drvdata(&pdev->dev, data); }
+int platform_set_driver_override(struct platform_device *pdev, + const char *override); + /* module_platform_driver() - Helper macro for drivers that don't do * anything special in module init/exit. This eliminates a lot of * boilerplate. Each module may only use this macro once, and
Platform driver driver_override field should not be initialized from const memory because the core later kfree() it. If driver_override is manually set later through sysfs, kfree() of old value leads to:
$ echo "new_value" > /sys/bus/platform/drivers/.../driver_override
kernel BUG at ../mm/slub.c:3960! Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM ... (kfree) from [<c058e8c0>] (platform_set_driver_override+0x84/0xac) (platform_set_driver_override) from [<c058e908>] (driver_override_store+0x20/0x34) (driver_override_store) from [<c031f778>] (kernfs_fop_write+0x100/0x1dc) (kernfs_fop_write) from [<c0296de8>] (__vfs_write+0x2c/0x17c) (__vfs_write) from [<c02970c4>] (vfs_write+0xa4/0x188) (vfs_write) from [<c02972e8>] (ksys_write+0x4c/0xac) (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Signed-off-by: Krzysztof Kozlowski krzk@kernel.org --- drivers/clk/samsung/clk-exynos5-subcmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/samsung/clk-exynos5-subcmu.c b/drivers/clk/samsung/clk-exynos5-subcmu.c index d07ef26bd052..880d92ad20e8 100644 --- a/drivers/clk/samsung/clk-exynos5-subcmu.c +++ b/drivers/clk/samsung/clk-exynos5-subcmu.c @@ -143,7 +143,7 @@ static int __init exynos5_clk_register_subcmu(struct device *parent, return -ENOMEM;
pdev->dev.parent = parent; - pdev->driver_override = "exynos5-subcmu"; + platform_set_driver_override(pdev, "exynos5-subcmu"); platform_set_drvdata(pdev, (void *)info); of_genpd_add_device(&genpdspec, &pdev->dev); ret = platform_device_add(pdev);
Platform driver driver_override field should not be initialized from const memory because the core later kfree() it. If driver_override is manually set later through sysfs, kfree() of old value leads to:
$ echo "new_value" > /sys/bus/platform/drivers/.../driver_override
kernel BUG at ../mm/slub.c:3960! Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM ... (kfree) from [<c058e8c0>] (platform_set_driver_override+0x84/0xac) (platform_set_driver_override) from [<c058e908>] (driver_override_store+0x20/0x34) (driver_override_store) from [<c031f778>] (kernfs_fop_write+0x100/0x1dc) (kernfs_fop_write) from [<c0296de8>] (__vfs_write+0x2c/0x17c) (__vfs_write) from [<c02970c4>] (vfs_write+0xa4/0x188) (vfs_write) from [<c02972e8>] (ksys_write+0x4c/0xac) (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Signed-off-by: Krzysztof Kozlowski krzk@kernel.org --- drivers/slimbus/qcom-ngd-ctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index 71f094c9ec68..fc46bf6ec903 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -1344,7 +1344,7 @@ static int of_qcom_slim_ngd_register(struct device *parent, ngd->pdev = platform_device_alloc(QCOM_SLIM_NGD_DRV_NAME, id); ngd->id = id; ngd->pdev->dev.parent = parent; - ngd->pdev->driver_override = QCOM_SLIM_NGD_DRV_NAME; + platform_set_driver_override(ngd->pdev, QCOM_SLIM_NGD_DRV_NAME); ngd->pdev->dev.of_node = node; ctrl->ngd = ngd; platform_set_drvdata(ngd->pdev, ctrl);
Hi Krzysztof,
On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski krzk@kernel.org wrote:
The problem
Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value.
However the users are unaware of this and set the driver_override like:
pdev->driver_override = "exynos5-subcmu";which obviously leads to error.
IMHO driver_override is not meant to be set by a driver, only from userspace, for binding the device to vfio (is there another use case?).
clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override
I see all users set override immediately after allocating a platform device. Can't they just allocate a platform device using the override name instead? What am I missing?
Thanks!
Gr{oetje,eeting}s,
Geert
On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Krzysztof,
On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski krzk@kernel.org wrote:
The problem
Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value.
However the users are unaware of this and set the driver_override like:
pdev->driver_override = "exynos5-subcmu";which obviously leads to error.
IMHO driver_override is not meant to be set by a driver, only from userspace, for binding the device to vfio (is there another use case?).
clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override
I see all users set override immediately after allocating a platform device. Can't they just allocate a platform device using the override name instead? What am I missing?
For the clk-exynos5-subcmu.c case, driver registers several sub-devices. Each sub-devices is a clock controller associated with power domain. I guess the author wanted to have meaningful names of these sub-devices (DISP, CAM etc. like names of power domains), instead of just exynos5-subcmu.1, exynos5-subcmu.2 ...
If driver_override should not be used for such case, then I could replace it with what you said.
For the other uses of driver_override (drivers/rpmsg/rpmsg_internal.h and drivers/slimbus/qcom-ngd-ctrl.c) I don't know.
Best regards, Krzysztof
Quoting Krzysztof Kozlowski (2019-02-18 03:14:29)
On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Krzysztof,
On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski krzk@kernel.org wrote:
The problem
Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value.
However the users are unaware of this and set the driver_override like:
pdev->driver_override = "exynos5-subcmu";which obviously leads to error.
IMHO driver_override is not meant to be set by a driver, only from userspace, for binding the device to vfio (is there another use case?).
clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override
I see all users set override immediately after allocating a platform device. Can't they just allocate a platform device using the override name instead? What am I missing?
For the clk-exynos5-subcmu.c case, driver registers several sub-devices. Each sub-devices is a clock controller associated with power domain. I guess the author wanted to have meaningful names of these sub-devices (DISP, CAM etc. like names of power domains), instead of just exynos5-subcmu.1, exynos5-subcmu.2 ...
If driver_override should not be used for such case, then I could replace it with what you said.
Looking at the introduction of the code into the platform bus makes it sound like it was all for vfio devices. If the clk driver doesn't need it for that purpose and can get by with more generic names then it seems best to avoid using it entirely. So can you do that and resend the first patch in this series too? Effectively splitting the clk parts from the larger issue of kfree()ing of const memory.
On Wed, 20 Feb 2019 at 23:01, Stephen Boyd sboyd@kernel.org wrote:
Quoting Krzysztof Kozlowski (2019-02-18 03:14:29)
On Mon, 18 Feb 2019 at 11:40, Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Krzysztof,
On Mon, Feb 18, 2019 at 11:27 AM Krzysztof Kozlowski krzk@kernel.org wrote:
The problem
Several device types (platform, amba, spi etc.) provide a driver_override field. On sysfs store or during device removal, they kfree() the existing value.
However the users are unaware of this and set the driver_override like:
pdev->driver_override = "exynos5-subcmu";which obviously leads to error.
IMHO driver_override is not meant to be set by a driver, only from userspace, for binding the device to vfio (is there another use case?).
clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override slimbus: ngd: Fix kfree() of const memory on setting driver_override
I see all users set override immediately after allocating a platform device. Can't they just allocate a platform device using the override name instead? What am I missing?
For the clk-exynos5-subcmu.c case, driver registers several sub-devices. Each sub-devices is a clock controller associated with power domain. I guess the author wanted to have meaningful names of these sub-devices (DISP, CAM etc. like names of power domains), instead of just exynos5-subcmu.1, exynos5-subcmu.2 ...
If driver_override should not be used for such case, then I could replace it with what you said.
Looking at the introduction of the code into the platform bus makes it sound like it was all for vfio devices. If the clk driver doesn't need it for that purpose and can get by with more generic names then it seems best to avoid using it entirely. So can you do that and resend the first patch in this series too? Effectively splitting the clk parts from the larger issue of kfree()ing of const memory.
Sure, let me send just the clk parts.
BR, Krzysztof
participants (3)
-
Geert Uytterhoeven -
Krzysztof Kozlowski -
Stephen Boyd