From: ChiYuan Huang <[email protected]>
Following by the below patch, there's potential UAF issue.
https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
Signed-off-by: ChiYuan Huang <[email protected]>
---
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
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.
Mark Brown <[email protected]> 於 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.
On Fri, Dec 02, 2022 at 11:35:35AM +0800, ChiYuan Huang wrote:
> Mark Brown <[email protected]> 於 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.