[PATCH 0/6] leds: fix broken devres usage

[PATCH 0/6] leds: fix broken devres usage

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device (about half of the MFD LED drivers do so).

This means you cannot blindly do devres conversions so that
deregistration ends up being tied to the parent device, something which
leads to use-after-free on driver unbind when the class device is
released while still being registered (and, for example, oopses on later
parent MFD driver unbind or LED class callbacks, or resource leaks and
name clashes on child driver reload).

Included is also a clean up removing some pointless casts when
registering class devices.

All but the lm3533 one have only been compile tested.

Johan


Johan Hovold (6):
  leds: 88pm860x: fix use-after-free on unbind
  leds: da903x: fix use-after-free on unbind
  leds: lm3533: fix use-after-free on unbind
  leds: lm36274: fix use-after-free on unbind
  leds: wm831x-status: fix use-after-free on unbind
  leds: drop redundant struct-device pointer casts

 drivers/leds/leds-88pm860x.c      | 14 +++++++++++++-
 drivers/leds/leds-da903x.c        | 14 +++++++++++++-
 drivers/leds/leds-lm3533.c        | 12 +++++++++---
 drivers/leds/leds-lm355x.c        |  9 +++------
 drivers/leds/leds-lm36274.c       | 15 ++++++++++++---
 drivers/leds/leds-lm3642.c        |  9 +++------
 drivers/leds/leds-wm831x-status.c | 14 +++++++++++++-
 7 files changed, 66 insertions(+), 21 deletions(-)

-- 
2.26.2

[PATCH 1/6] leds: 88pm860x: fix use-after-free on unbind

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device. This means you cannot blindly do devres conversions
so that deregistration ends up being tied to the parent device,
something which leads to use-after-free on driver unbind when the class
device is released while still being registered.

Fixes: 375446df95ee ("leds: 88pm860x: Use devm_led_classdev_register")
Cc: stable <stable@vger.kernel.org>     # 4.6
Cc: Amitoj Kaur Chawla <amitoj1606@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-88pm860x.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-88pm860x.c b/drivers/leds/leds-88pm860x.c
index b3044c9a8120..465c3755cf2e 100644
--- a/drivers/leds/leds-88pm860x.c
+++ b/drivers/leds/leds-88pm860x.c
@@ -203,21 +203,33 @@ static int pm860x_led_probe(struct platform_device *pdev)
 	data->cdev.brightness_set_blocking = pm860x_led_set;
 	mutex_init(&data->lock);
 
-	ret = devm_led_classdev_register(chip->dev, &data->cdev);
+	ret = led_classdev_register(chip->dev, &data->cdev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register LED: %d\n", ret);
 		return ret;
 	}
 	pm860x_led_set(&data->cdev, 0);
+
+	platform_set_drvdata(pdev, data);
+
 	return 0;
 }
 
+static int pm860x_led_remove(struct platform_device *pdev)
+{
+	struct pm860x_led *data = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&data->cdev);
+
+	return 0;
+}
 
 static struct platform_driver pm860x_led_driver = {
 	.driver	= {
 		.name	= "88pm860x-led",
 	},
 	.probe	= pm860x_led_probe,
+	.remove	= pm860x_led_remove,
 };
 
 module_platform_driver(pm860x_led_driver);
-- 
2.26.2

[PATCH 2/6] leds: da903x: fix use-after-free on unbind

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device. This means you cannot blindly do devres conversions
so that deregistration ends up being tied to the parent device,
something which leads to use-after-free on driver unbind when the class
device is released while still being registered.

Fixes: eed16255d66b ("leds: da903x: Use devm_led_classdev_register")
Cc: stable <stable@vger.kernel.org>     # 4.6
Cc: Amitoj Kaur Chawla <amitoj1606@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-da903x.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-da903x.c b/drivers/leds/leds-da903x.c
index ed1b303f699f..2b5fb00438a2 100644
--- a/drivers/leds/leds-da903x.c
+++ b/drivers/leds/leds-da903x.c
@@ -110,12 +110,23 @@ static int da903x_led_probe(struct platform_device *pdev)
 	led->flags = pdata->flags;
 	led->master = pdev->dev.parent;
 
-	ret = devm_led_classdev_register(led->master, &led->cdev);
+	ret = led_classdev_register(led->master, &led->cdev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register LED %d\n", id);
 		return ret;
 	}
 
+	platform_set_drvdata(pdev, led);
+
+	return 0;
+}
+
+static int da903x_led_remove(struct platform_device *pdev)
+{
+	struct da903x_led *led = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&led->cdev);
+
 	return 0;
 }
 
@@ -124,6 +135,7 @@ static struct platform_driver da903x_led_driver = {
 		.name	= "da903x-led",
 	},
 	.probe		= da903x_led_probe,
+	.remove		= da903x_led_remove,
 };
 
 module_platform_driver(da903x_led_driver);
-- 
2.26.2

[PATCH 3/6] leds: lm3533: fix use-after-free on unbind

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device. This means you cannot blindly do devres conversions
so that deregistration ends up being tied to the parent device,
something which leads to use-after-free on driver unbind when the class
device is released while still being registered.

Fixes: 50154e29e5cc ("leds: lm3533: Use devm_led_classdev_register")
Cc: stable <stable@vger.kernel.org>     # 4.6
Cc: Amitoj Kaur Chawla <amitoj1606@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-lm3533.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
index 9504ad405aef..b3edee703193 100644
--- a/drivers/leds/leds-lm3533.c
+++ b/drivers/leds/leds-lm3533.c
@@ -694,7 +694,7 @@ static int lm3533_led_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, led);
 
-	ret = devm_led_classdev_register(pdev->dev.parent, &led->cdev);
+	ret = led_classdev_register(pdev->dev.parent, &led->cdev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register LED %d\n", pdev->id);
 		return ret;
@@ -704,13 +704,18 @@ static int lm3533_led_probe(struct platform_device *pdev)
 
 	ret = lm3533_led_setup(led, pdata);
 	if (ret)
-		return ret;
+		goto err_deregister;
 
 	ret = lm3533_ctrlbank_enable(&led->cb);
 	if (ret)
-		return ret;
+		goto err_deregister;
 
 	return 0;
+
+err_deregister:
+	led_classdev_unregister(&led->cdev);
+
+	return ret;
 }
 
 static int lm3533_led_remove(struct platform_device *pdev)
@@ -720,6 +725,7 @@ static int lm3533_led_remove(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	lm3533_ctrlbank_disable(&led->cb);
+	led_classdev_unregister(&led->cdev);
 
 	return 0;
 }
-- 
2.26.2

[PATCH 4/6] leds: lm36274: fix use-after-free on unbind

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device. This means you cannot use devres so that
deregistration ends up being tied to the parent device, something which
leads to use-after-free on driver unbind when the class device is
released while still being registered.

Fixes: 11e1bbc116a7 ("leds: lm36274: Introduce the TI LM36274 LED driver")
Cc: stable <stable@vger.kernel.org>     # 5.3
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-lm36274.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-lm36274.c b/drivers/leds/leds-lm36274.c
index 836b60c9a2b8..db842eeb7ca2 100644
--- a/drivers/leds/leds-lm36274.c
+++ b/drivers/leds/leds-lm36274.c
@@ -133,7 +133,7 @@ static int lm36274_probe(struct platform_device *pdev)
 	lm36274_data->pdev = pdev;
 	lm36274_data->dev = lmu->dev;
 	lm36274_data->regmap = lmu->regmap;
-	dev_set_drvdata(&pdev->dev, lm36274_data);
+	platform_set_drvdata(pdev, lm36274_data);
 
 	ret = lm36274_parse_dt(lm36274_data);
 	if (ret) {
@@ -147,8 +147,16 @@ static int lm36274_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	return devm_led_classdev_register(lm36274_data->dev,
-					 &lm36274_data->led_dev);
+	return led_classdev_register(lm36274_data->dev, &lm36274_data->led_dev);
+}
+
+static int lm36274_remove(struct platform_device *pdev)
+{
+	struct lm36274 *lm36274_data = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&lm36274_data->led_dev);
+
+	return 0;
 }
 
 static const struct of_device_id of_lm36274_leds_match[] = {
@@ -159,6 +167,7 @@ MODULE_DEVICE_TABLE(of, of_lm36274_leds_match);
 
 static struct platform_driver lm36274_driver = {
 	.probe  = lm36274_probe,
+	.remove = lm36274_remove,
 	.driver = {
 		.name = "lm36274-leds",
 	},
-- 
2.26.2

[PATCH 5/6] leds: wm831x-status: fix use-after-free on unbind

[Johan Hovold]

Several MFD child drivers register their class devices directly under
the parent device. This means you cannot blindly do devres conversions
so that deregistration ends up being tied to the parent device,
something which leads to use-after-free on driver unbind when the class
device is released while still being registered.

Fixes: 8d3b6a4001ce ("leds: wm831x-status: Use devm_led_classdev_register")
Cc: stable <stable@vger.kernel.org>     # 4.6
Cc: Amitoj Kaur Chawla <amitoj1606@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-wm831x-status.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-wm831x-status.c b/drivers/leds/leds-wm831x-status.c
index 082df7f1dd90..67f4235cb28a 100644
--- a/drivers/leds/leds-wm831x-status.c
+++ b/drivers/leds/leds-wm831x-status.c
@@ -269,12 +269,23 @@ static int wm831x_status_probe(struct platform_device *pdev)
 	drvdata->cdev.blink_set = wm831x_status_blink_set;
 	drvdata->cdev.groups = wm831x_status_groups;
 
-	ret = devm_led_classdev_register(wm831x->dev, &drvdata->cdev);
+	ret = led_classdev_register(wm831x->dev, &drvdata->cdev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register LED: %d\n", ret);
 		return ret;
 	}
 
+	platform_set_drvdata(pdev, drvdata);
+
+	return 0;
+}
+
+static int wm831x_status_remove(struct platform_device *pdev)
+{
+	struct wm831x_status *drvdata = platform_get_drvdata(pdev);
+
+	led_classdev_unregister(&drvdata->cdev);
+
 	return 0;
 }
 
@@ -283,6 +294,7 @@ static struct platform_driver wm831x_status_driver = {
 		   .name = "wm831x-status",
 		   },
 	.probe = wm831x_status_probe,
+	.remove = wm831x_status_remove,
 };
 
 module_platform_driver(wm831x_status_driver);
-- 
2.26.2

[PATCH 6/6] leds: drop redundant struct-device pointer casts

[Johan Hovold]

Drop the pointless and needlessly confusing casts of struct-device
pointers.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/leds/leds-lm355x.c | 9 +++------
 drivers/leds/leds-lm3642.c | 9 +++------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lm355x.c b/drivers/leds/leds-lm355x.c
index a5abb499574b..c690eafe2746 100644
--- a/drivers/leds/leds-lm355x.c
+++ b/drivers/leds/leds-lm355x.c
@@ -453,8 +453,7 @@ static int lm355x_probe(struct i2c_client *client,
 	chip->cdev_flash.max_brightness = 16;
 	chip->cdev_flash.brightness_set_blocking = lm355x_strobe_brightness_set;
 	chip->cdev_flash.default_trigger = "flash";
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_flash);
+	err = led_classdev_register(&client->dev, &chip->cdev_flash);
 	if (err < 0)
 		goto err_out;
 	/* torch */
@@ -462,8 +461,7 @@ static int lm355x_probe(struct i2c_client *client,
 	chip->cdev_torch.max_brightness = 8;
 	chip->cdev_torch.brightness_set_blocking = lm355x_torch_brightness_set;
 	chip->cdev_torch.default_trigger = "torch";
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_torch);
+	err = led_classdev_register(&client->dev, &chip->cdev_torch);
 	if (err < 0)
 		goto err_create_torch_file;
 	/* indicator */
@@ -477,8 +475,7 @@ static int lm355x_probe(struct i2c_client *client,
 	/* indicator pattern control only for LM3556 */
 	if (id->driver_data == CHIP_LM3556)
 		chip->cdev_indicator.groups = lm355x_indicator_groups;
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_indicator);
+	err = led_classdev_register(&client->dev, &chip->cdev_indicator);
 	if (err < 0)
 		goto err_create_indicator_file;
 
diff --git a/drivers/leds/leds-lm3642.c b/drivers/leds/leds-lm3642.c
index 4232906fcb75..62c14872caf7 100644
--- a/drivers/leds/leds-lm3642.c
+++ b/drivers/leds/leds-lm3642.c
@@ -340,8 +340,7 @@ static int lm3642_probe(struct i2c_client *client,
 	chip->cdev_flash.brightness_set_blocking = lm3642_strobe_brightness_set;
 	chip->cdev_flash.default_trigger = "flash";
 	chip->cdev_flash.groups = lm3642_flash_groups,
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_flash);
+	err = led_classdev_register(&client->dev, &chip->cdev_flash);
 	if (err < 0) {
 		dev_err(chip->dev, "failed to register flash\n");
 		goto err_out;
@@ -353,8 +352,7 @@ static int lm3642_probe(struct i2c_client *client,
 	chip->cdev_torch.brightness_set_blocking = lm3642_torch_brightness_set;
 	chip->cdev_torch.default_trigger = "torch";
 	chip->cdev_torch.groups = lm3642_torch_groups,
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_torch);
+	err = led_classdev_register(&client->dev, &chip->cdev_torch);
 	if (err < 0) {
 		dev_err(chip->dev, "failed to register torch\n");
 		goto err_create_torch_file;
@@ -365,8 +363,7 @@ static int lm3642_probe(struct i2c_client *client,
 	chip->cdev_indicator.max_brightness = 8;
 	chip->cdev_indicator.brightness_set_blocking =
 						lm3642_indicator_brightness_set;
-	err = led_classdev_register((struct device *)
-				    &client->dev, &chip->cdev_indicator);
+	err = led_classdev_register(&client->dev, &chip->cdev_indicator);
 	if (err < 0) {
 		dev_err(chip->dev, "failed to register indicator\n");
 		goto err_create_indicator_file;
-- 
2.26.2

Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
>
> Several MFD child drivers register their class devices directly under
> the parent device (about half of the MFD LED drivers do so).
>
> This means you cannot blindly do devres conversions so that
> deregistration ends up being tied to the parent device, something which
> leads to use-after-free on driver unbind when the class device is
> released while still being registered (and, for example, oopses on later
> parent MFD driver unbind or LED class callbacks, or resource leaks and
> name clashes on child driver reload).

Shouldn't MFD take reference count for their children?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] leds: fix broken devres usage

[Johan Hovold]

On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > Several MFD child drivers register their class devices directly under
> > the parent device (about half of the MFD LED drivers do so).
> >
> > This means you cannot blindly do devres conversions so that
> > deregistration ends up being tied to the parent device, something which
> > leads to use-after-free on driver unbind when the class device is
> > released while still being registered (and, for example, oopses on later
> > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > name clashes on child driver reload).
> 
> Shouldn't MFD take reference count for their children?

That's not the issue here. The child driver is allocating memory for the
class device (for example using devres), and that will end up being
freed on unbind while said device is still registered. The child driver
may then even be unloaded. No extra reference can fix this.

Johan

Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > Several MFD child drivers register their class devices directly under
> > > the parent device (about half of the MFD LED drivers do so).
> > >
> > > This means you cannot blindly do devres conversions so that
> > > deregistration ends up being tied to the parent device, something which
> > > leads to use-after-free on driver unbind when the class device is
> > > released while still being registered (and, for example, oopses on later
> > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > name clashes on child driver reload).
> >
> > Shouldn't MFD take reference count for their children?
>
> That's not the issue here. The child driver is allocating memory for the
> class device (for example using devres), and that will end up being
> freed on unbind while said device is still registered. The child driver
> may then even be unloaded. No extra reference can fix this.

Okay, I didn't still get how dropping devres will help here.

Say, we have

->probe()
{
 return devm_foo_register();
}

and no ->remove()

vs.

->probe()
{
  return foo_register();
}

->remove()
{
 foo_unregister();
}

So, basically what you seem to workaround is that ->remove() is not
getting called?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] leds: fix broken devres usage

[Johan Hovold]

On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > >
> > > > Several MFD child drivers register their class devices directly under
> > > > the parent device (about half of the MFD LED drivers do so).
> > > >
> > > > This means you cannot blindly do devres conversions so that
> > > > deregistration ends up being tied to the parent device, something which
> > > > leads to use-after-free on driver unbind when the class device is
> > > > released while still being registered (and, for example, oopses on later
> > > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > > name clashes on child driver reload).
> > >
> > > Shouldn't MFD take reference count for their children?
> >
> > That's not the issue here. The child driver is allocating memory for the
> > class device (for example using devres), and that will end up being
> > freed on unbind while said device is still registered. The child driver
> > may then even be unloaded. No extra reference can fix this.
> 
> Okay, I didn't still get how dropping devres will help here.
> 
> Say, we have
> 
> ->probe()
> {
>  return devm_foo_register();
> }
> 
> and no ->remove()
> 
> vs.
> 
> ->probe()
> {
>   return foo_register();
> }
> 
> ->remove()
> {
>  foo_unregister();
> }
> 
> So, basically what you seem to workaround is that ->remove() is not
> getting called?

Any driver which frees a resource before making sure it's no longer used
it is just plain broken. Unfortunately, devres makes this harder to
reason about and people get it wrong. This is roughly the current
situation with these drivers:

	drv->probe(dev)
	  foo = devm_kzalloc(dev);
	  devm_foo_register(dev->parent, foo);	// NOTE: dev->parent

	drv->remove(dev)
	devres_release_all(dev)
	  kfree(foo);				// foo still registered

but foo remains registered until the parent driver is unbound.

Johan

Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > > >
> > > > > Several MFD child drivers register their class devices directly under
> > > > > the parent device (about half of the MFD LED drivers do so).
> > > > >
> > > > > This means you cannot blindly do devres conversions so that
> > > > > deregistration ends up being tied to the parent device, something which
> > > > > leads to use-after-free on driver unbind when the class device is
> > > > > released while still being registered (and, for example, oopses on later
> > > > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > > > name clashes on child driver reload).
> > > >
> > > > Shouldn't MFD take reference count for their children?
> > >
> > > That's not the issue here. The child driver is allocating memory for the
> > > class device (for example using devres), and that will end up being
> > > freed on unbind while said device is still registered. The child driver
> > > may then even be unloaded. No extra reference can fix this.
> >
> > Okay, I didn't still get how dropping devres will help here.
> >
> > Say, we have
> >
> > ->probe()
> > {
> >  return devm_foo_register();
> > }
> >
> > and no ->remove()
> >
> > vs.
> >
> > ->probe()
> > {
> >   return foo_register();
> > }
> >
> > ->remove()
> > {
> >  foo_unregister();
> > }
> >
> > So, basically what you seem to workaround is that ->remove() is not
> > getting called?
>
> Any driver which frees a resource before making sure it's no longer used
> it is just plain broken. Unfortunately, devres makes this harder to
> reason about and people get it wrong. This is roughly the current
> situation with these drivers:
>
>         drv->probe(dev)
>           foo = devm_kzalloc(dev);

>           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent

A-ha! Thanks for this detail.
But why are they doing so?

>         drv->remove(dev)
>         devres_release_all(dev)
>           kfree(foo);                           // foo still registered

> but foo remains registered until the parent driver is unbound.

Since the last fixes against kobject elimination, shouldn't be this
simple fixed by not supplying dev->parent above?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] leds: fix broken devres usage

[Johan Hovold]

On Mon, Jun 01, 2020 at 06:09:23PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@kernel.org> wrote:
> > > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Jun 1, 2020 at 4:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > > > >
> > > > > > Several MFD child drivers register their class devices directly under
> > > > > > the parent device (about half of the MFD LED drivers do so).
> > > > > >
> > > > > > This means you cannot blindly do devres conversions so that
> > > > > > deregistration ends up being tied to the parent device, something which
> > > > > > leads to use-after-free on driver unbind when the class device is
> > > > > > released while still being registered (and, for example, oopses on later
> > > > > > parent MFD driver unbind or LED class callbacks, or resource leaks and
> > > > > > name clashes on child driver reload).
> > > > >
> > > > > Shouldn't MFD take reference count for their children?
> > > >
> > > > That's not the issue here. The child driver is allocating memory for the
> > > > class device (for example using devres), and that will end up being
> > > > freed on unbind while said device is still registered. The child driver
> > > > may then even be unloaded. No extra reference can fix this.
> > >
> > > Okay, I didn't still get how dropping devres will help here.

> > Any driver which frees a resource before making sure it's no longer used
> > it is just plain broken. Unfortunately, devres makes this harder to
> > reason about and people get it wrong. This is roughly the current
> > situation with these drivers:
> >
> >         drv->probe(dev)
> >           foo = devm_kzalloc(dev);
> 
> >           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent
> 
> A-ha! Thanks for this detail.
> But why are they doing so?

As I mentioned in a commit message, we have quite a few MFD drivers that
register class devices under the parent device directly and have been
doing so for a very long time.

As this is reflected in sysfs and we may have users relying on the
current topology, changing this shouldn't be taken too lightly (drivers
may also depend on it). And in any case, it wouldn't be stable material
to fix the regressions at hand.

> >         drv->remove(dev)
> >         devres_release_all(dev)
> >           kfree(foo);                           // foo still registered
> 
> > but foo remains registered until the parent driver is unbound.
> 
> Since the last fixes against kobject elimination, shouldn't be this
> simple fixed by not supplying dev->parent above?

No, that's a separate issue as it also changes the device tree.

Johan

Re: [PATCH 0/6] leds: fix broken devres usage

[Andy Shevchenko]

On Mon, Jun 1, 2020 at 6:31 PM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Jun 01, 2020 at 06:09:23PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 1, 2020 at 5:29 PM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, Jun 01, 2020 at 05:08:40PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 1, 2020 at 5:01 PM Johan Hovold <johan@kernel.org> wrote:
> > > > > On Mon, Jun 01, 2020 at 04:51:01PM +0300, Andy Shevchenko wrote:

...

> > > > > That's not the issue here. The child driver is allocating memory for the
> > > > > class device (for example using devres), and that will end up being
> > > > > freed on unbind while said device is still registered. The child driver
> > > > > may then even be unloaded. No extra reference can fix this.
> > > >
> > > > Okay, I didn't still get how dropping devres will help here.
>
> > > Any driver which frees a resource before making sure it's no longer used
> > > it is just plain broken. Unfortunately, devres makes this harder to
> > > reason about and people get it wrong. This is roughly the current
> > > situation with these drivers:
> > >
> > >         drv->probe(dev)
> > >           foo = devm_kzalloc(dev);
> >
> > >           devm_foo_register(dev->parent, foo);  // NOTE: dev->parent
> >
> > A-ha! Thanks for this detail.
> > But why are they doing so?
>
> As I mentioned in a commit message, we have quite a few MFD drivers that
> register class devices under the parent device directly and have been
> doing so for a very long time.
>
> As this is reflected in sysfs and we may have users relying on the
> current topology, changing this shouldn't be taken too lightly (drivers
> may also depend on it). And in any case, it wouldn't be stable material
> to fix the regressions at hand.

I see.

> > >         drv->remove(dev)
> > >         devres_release_all(dev)
> > >           kfree(foo);                           // foo still registered
> >
> > > but foo remains registered until the parent driver is unbound.
> >
> > Since the last fixes against kobject elimination, shouldn't be this
> > simple fixed by not supplying dev->parent above?
>
> No, that's a separate issue as it also changes the device tree.

Thanks for elaboration.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] leds: fix broken devres usage

[Pavel Machek]

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

Hi!

> Several MFD child drivers register their class devices directly under
> the parent device (about half of the MFD LED drivers do so).
> 
> This means you cannot blindly do devres conversions so that
> deregistration ends up being tied to the parent device, something which
> leads to use-after-free on driver unbind when the class device is
> released while still being registered (and, for example, oopses on later
> parent MFD driver unbind or LED class callbacks, or resource leaks and
> name clashes on child driver reload).
> 
> Included is also a clean up removing some pointless casts when
> registering class devices.
> 
> All but the lm3533 one have only been compile tested.

It would be nicer to have devres framework work with these... but I
guess this should go in...

Best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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