[RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[Krzysztof Kozlowski]

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(-)

-- 
2.7.4

[RFC 1/4] clk: samsung: exynos5: Fix possible NULL pointer exception on platform_device_alloc() failure

[Krzysztof Kozlowski]

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)
-- 
2.7.4

[RFC 2/4] driver: platform: Provide helper for safer setting of driver_override

[Krzysztof Kozlowski]

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
-- 
2.7.4

[RFC 3/4] clk: samsung: exynos5: Fix kfree() of const memory on setting driver_override

[Krzysztof Kozlowski]

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);
-- 
2.7.4

[RFC 4/4] slimbus: ngd: Fix kfree() of const memory on setting driver_override

[Krzysztof Kozlowski]

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);
-- 
2.7.4

Re: [RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[Krzysztof Kozlowski]

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

Re: [RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[Stephen Boyd]

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.

Re: [RFC 0/4] clk/driver: platform: Fix kfree() of const memory on setting driver_override

[Krzysztof Kozlowski]

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