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

Commit Message

ChiYuan Huang Nov. 30, 2022, 8:37 a.m. UTC

From: ChiYuan Huang <[e-mail]>

Following by the below patch, there's potential UAF issue.
https://lore.kernel.org/all/[e-mail]/

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 <[e-mail]>
Signed-off-by: ChiYuan Huang <[e-mail]>
---
 drivers/regulator/mt6370-regulator.c | 61 ++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 17 deletions(-)

Comments

Mark Brown Dec. 1, 2022, 11:43 a.m. UTC | #1

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.

ChiYuan Huang Dec. 2, 2022, 3:35 a.m. UTC | #2

Mark Brown <[e-mail]> 於 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.

Mark Brown Dec. 2, 2022, 12:08 p.m. UTC | #3

On Fri, Dec 02, 2022 at 11:35:35AM +0800, ChiYuan Huang wrote:
> Mark Brown <[e-mail]> 於 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.

Patch

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