[PATCH 1/2] regulator: mt6370: Fix potential UAF issue

[PATCH 1/2] regulator: mt6370: Fix potential UAF issue

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Following by the below patch, there's potential UAF issue.
https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/

CPU A				|CPU B
mt6370_probe()			|
  devm_mfd_add_devices()	|
				|mt6370_regulator_probe()
				|  regulator_register()
				|    //allocate init_data and add it to
devres
				|    regulator_of_get_init_data()
i2c_unregister_device()		|
  device_del()			|
    devres_release_all()	|
      // init_data is freed	|
      release_nodes()		|
				|  // using init_data causes UAF
				|  regulator_register()

The original code uses i2c dev as the parent in order to reuse
the 'regulator_of_get_init_data'. But this will cause regulation
constraint devres attached to i2c dev, not the mfd cell platform
device.

Use 'of_regulator_match' to directly parse regulation constraint from
parent dev node. Correct all regulator devs parent back to the platform
device itself.

Fixes: 8171c93bac1b ("regulator: mt6370: Add mt6370 DisplayBias and VibLDO support")
Reported-by: Yang Yingliang <yangyingliang@huawei.com>
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/regulator/mt6370-regulator.c | 61 ++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/mt6370-regulator.c b/drivers/regulator/mt6370-regulator.c
index e73f5a4..c2b589a 100644
--- a/drivers/regulator/mt6370-regulator.c
+++ b/drivers/regulator/mt6370-regulator.c
@@ -11,6 +11,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
 
 enum {
 	MT6370_IDX_DSVBOOST = 0,
@@ -183,8 +184,6 @@ static int mt6370_of_parse_cb(struct device_node *np,
 static const struct regulator_desc mt6370_regulator_descs[] = {
 	{
 		.name = "mt6370-dsv-vbst",
-		.of_match = of_match_ptr("dsvbst"),
-		.regulators_node = of_match_ptr("regulators"),
 		.id = MT6370_IDX_DSVBOOST,
 		.type = REGULATOR_VOLTAGE,
 		.owner = THIS_MODULE,
@@ -200,8 +199,6 @@ static const struct regulator_desc mt6370_regulator_descs[] = {
 	},
 	{
 		.name = "mt6370-dsv-vpos",
-		.of_match = of_match_ptr("dsvpos"),
-		.regulators_node = of_match_ptr("regulators"),
 		.id = MT6370_IDX_DSVPOS,
 		.type = REGULATOR_VOLTAGE,
 		.owner = THIS_MODULE,
@@ -224,8 +221,6 @@ static const struct regulator_desc mt6370_regulator_descs[] = {
 	},
 	{
 		.name = "mt6370-dsv-vneg",
-		.of_match = of_match_ptr("dsvneg"),
-		.regulators_node = of_match_ptr("regulators"),
 		.id = MT6370_IDX_DSVNEG,
 		.type = REGULATOR_VOLTAGE,
 		.owner = THIS_MODULE,
@@ -248,8 +243,6 @@ static const struct regulator_desc mt6370_regulator_descs[] = {
 	},
 	{
 		.name = "mt6370-vib-ldo",
-		.of_match = of_match_ptr("vibldo"),
-		.regulators_node = of_match_ptr("regulators"),
 		.id = MT6370_IDX_VIBLDO,
 		.type = REGULATOR_VOLTAGE,
 		.owner = THIS_MODULE,
@@ -320,23 +313,57 @@ static int mt6370_regulator_irq_register(struct mt6370_priv *priv)
 	return 0;
 }
 
+static struct of_regulator_match mt6370_regulator_match[MT6370_MAX_IDX] =  {
+	[MT6370_IDX_DSVBOOST]	= { .name = "dsvbst" },
+	[MT6370_IDX_DSVPOS]	= { .name = "dsvpos" },
+	[MT6370_IDX_DSVNEG]	= { .name = "dsvneg" },
+	[MT6370_IDX_VIBLDO]	= { .name = "vibldo" },
+};
+
 static int mt6370_regualtor_register(struct mt6370_priv *priv)
 {
 	struct regulator_dev *rdev;
-	struct regulator_config cfg = {};
 	struct device *parent = priv->dev->parent;
-	int i;
+	struct device *dev = priv->dev;
+	struct device_node *regulator_np;
+	int i, ret;
+
+	regulator_np = of_get_child_by_name(parent->of_node, "regulators");
+	if (!regulator_np) {
+		dev_err(dev, "Could not find parent 'regulators' node\n");
+		return -ENODEV;
+	}
+
+	ret = of_regulator_match(dev, regulator_np, mt6370_regulator_match,
+				 ARRAY_SIZE(mt6370_regulator_match));
 
-	cfg.dev = parent;
-	cfg.driver_data = priv;
+	of_node_put(regulator_np);
+
+	if (ret < 0) {
+		dev_err(dev, "Error parsing regulator init data: %d\n", ret);
+		return ret;
+	}
 
 	for (i = 0; i < MT6370_MAX_IDX; i++) {
-		rdev = devm_regulator_register(priv->dev,
-					       mt6370_regulator_descs + i,
-					       &cfg);
+		const struct regulator_desc *desc = mt6370_regulator_descs + i;
+		struct regulator_config cfg = {};
+
+		cfg.dev = dev;
+		cfg.driver_data = priv;
+		cfg.init_data = mt6370_regulator_match[i].init_data;
+		cfg.of_node = mt6370_regulator_match[i].of_node;
+
+		if (cfg.of_node && desc->of_parse_cb) {
+			ret = desc->of_parse_cb(cfg.of_node, desc, &cfg);
+			if (ret) {
+				dev_err(dev, "Failed in of_parse_cb\n");
+				return ret;
+			}
+		}
+
+		rdev = devm_regulator_register(dev, desc, &cfg);
 		if (IS_ERR(rdev)) {
-			dev_err(priv->dev,
-				"Failed to register (%d) regulator\n", i);
+			dev_err(dev, "Failed to register (%d) regulator\n", i);
 			return PTR_ERR(rdev);
 		}
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] regulator: mt6370: Switch to use dev_err_probe() helper

[cy_huang]

From: ChiYuan Huang <cy_huang@richtek.com>

Use dev_err_probe helper to simplify the probe function.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/regulator/mt6370-regulator.c | 39 +++++++++++++-----------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/mt6370-regulator.c b/drivers/regulator/mt6370-regulator.c
index c2b589a..e090fbe 100644
--- a/drivers/regulator/mt6370-regulator.c
+++ b/drivers/regulator/mt6370-regulator.c
@@ -303,11 +303,9 @@ static int mt6370_regulator_irq_register(struct mt6370_priv *priv)
 		ret = devm_request_threaded_irq(priv->dev, irq, NULL,
 						mt6370_irqs[i].handler, 0,
 						mt6370_irqs[i].name, rdev);
-		if (ret) {
-			dev_err(priv->dev,
-				"Failed to register (%d) interrupt\n", i);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(priv->dev, ret,
+					     "Failed to register (%d) interrupt\n", i);
 	}
 
 	return 0;
@@ -329,20 +327,16 @@ static int mt6370_regualtor_register(struct mt6370_priv *priv)
 	int i, ret;
 
 	regulator_np = of_get_child_by_name(parent->of_node, "regulators");
-	if (!regulator_np) {
-		dev_err(dev, "Could not find parent 'regulators' node\n");
-		return -ENODEV;
-	}
+	if (!regulator_np)
+		return dev_err_probe(dev, -ENODEV, "Could not find parent 'regulators' node\n");
 
 	ret = of_regulator_match(dev, regulator_np, mt6370_regulator_match,
 				 ARRAY_SIZE(mt6370_regulator_match));
 
 	of_node_put(regulator_np);
 
-	if (ret < 0) {
-		dev_err(dev, "Error parsing regulator init data: %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Error parsing regulator init data\n");
 
 	for (i = 0; i < MT6370_MAX_IDX; i++) {
 		const struct regulator_desc *desc = mt6370_regulator_descs + i;
@@ -355,17 +349,14 @@ static int mt6370_regualtor_register(struct mt6370_priv *priv)
 
 		if (cfg.of_node && desc->of_parse_cb) {
 			ret = desc->of_parse_cb(cfg.of_node, desc, &cfg);
-			if (ret) {
-				dev_err(dev, "Failed in of_parse_cb\n");
-				return ret;
-			}
+			if (ret)
+				return dev_err_probe(dev, ret, "Failed in of_parse_cb\n");
 		}
 
 		rdev = devm_regulator_register(dev, desc, &cfg);
-		if (IS_ERR(rdev)) {
-			dev_err(dev, "Failed to register (%d) regulator\n", i);
-			return PTR_ERR(rdev);
-		}
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(rdev),
+					     "Failed to register (%d) regulator\n", i);
 
 		priv->rdev[i] = rdev;
 	}
@@ -385,10 +376,8 @@ static int mt6370_regulator_probe(struct platform_device *pdev)
 	priv->dev = &pdev->dev;
 
 	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!priv->regmap) {
-		dev_err(&pdev->dev, "Failed to init regmap\n");
-		return -ENODEV;
-	}
+	if (!priv->regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to init regmap\n");
 
 	ret = mt6370_regualtor_register(priv);
 	if (ret)
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] regulator: mt6370: Fix potential UAF issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]

On Wed, Nov 30, 2022 at 04:37:42PM +0800, cy_huang wrote:

> The original code uses i2c dev as the parent in order to reuse
> the 'regulator_of_get_init_data'. But this will cause regulation
> constraint devres attached to i2c dev, not the mfd cell platform
> device.

This is a general issue which will affect a lot of MFDs, we would be
better to fix this by changing the API to provide a device to be used
for the devres allocations separately to the one used for looking up the
DT.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] regulator: mt6370: Fix potential UAF issue

[ChiYuan Huang]

Mark Brown <broonie@kernel.org> 於 2022年12月1日 週四 晚上7:43寫道：
>
> On Wed, Nov 30, 2022 at 04:37:42PM +0800, cy_huang wrote:
>
> > The original code uses i2c dev as the parent in order to reuse
> > the 'regulator_of_get_init_data'. But this will cause regulation
> > constraint devres attached to i2c dev, not the mfd cell platform
> > device.
>
> This is a general issue which will affect a lot of MFDs, we would be
> better to fix this by changing the API to provide a device to be used
> for the devres allocations separately to the one used for looking up the
> DT.

Not to affect too much, the better way may change the 'regulator_register' API.
Append it as regulator_register(dev, .....
This could separate device object with devres allocation and DT lookup.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] regulator: mt6370: Fix potential UAF issue

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 866 bytes --]

On Fri, Dec 02, 2022 at 11:35:35AM +0800, ChiYuan Huang wrote:
> Mark Brown <broonie@kernel.org> 於 2022年12月1日 週四 晚上7:43寫道：
> > > The original code uses i2c dev as the parent in order to reuse
> > > the 'regulator_of_get_init_data'. But this will cause regulation
> > > constraint devres attached to i2c dev, not the mfd cell platform
> > > device.

> > This is a general issue which will affect a lot of MFDs, we would be
> > better to fix this by changing the API to provide a device to be used
> > for the devres allocations separately to the one used for looking up the
> > DT.

> Not to affect too much, the better way may change the 'regulator_register' API.
> Append it as regulator_register(dev, .....
> This could separate device object with devres allocation and DT lookup.

Yes, I think so - a new optional argument.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel