On Sun, Dec 18, 2022 at 11:07:38AM -0500, Sasha Levin wrote: Hi, Thanks, but there's one more case not considered. It may cause a unexpected regulator shutdown by regulator core. Here's the discussion link that reported from Marek Szyprowski. https://lore.kernel.org/lkml/dd329b51-f11a-2af6-9549-c8a014fd5a71@xxxxxxxxxxx/ I have post a patch to fix it. You may need to cherry-pick the below patch also. 0debed5b117d ("regulator: core: Fix resolve supply lookup issue") Best regards, ChiYuan. > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > [ Upstream commit 8f3cbcd6b440032ebc7f7d48a1689dcc70a4eb98 ] > > Following by the below discussion, there's the potential UAF issue > between regulator and mfd. > https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@xxxxxxxxxx/ > > >From the analysis of Yingliang > > 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() > > It's common to use mfd core to create child device for the regulator. > In order to do the DT lookup for init data, the child that registered > the regulator would pass its parent as the parameter. And this causes > init data resource allocated to its parent, not itself. The issue happen > when parent device is going to release and regulator core is still doing > some operation of init data constraint for the regulator of child device. > > To fix it, this patch expand 'regulator_register' API to use the > different devices for init data allocation and DT lookup. > > Reported-by: Yang Yingliang <yangyingliang@xxxxxxxxxx> > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > Link: https://lore.kernel.org/r/1670311341-32664-1-git-send-email-u0084500@xxxxxxxxx > Signed-off-by: Mark Brown <broonie@xxxxxxxxxx> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx> > --- > drivers/platform/x86/intel/int3472/clk_and_regulator.c | 3 ++- > drivers/regulator/core.c | 8 ++++---- > drivers/regulator/devres.c | 2 +- > drivers/regulator/of_regulator.c | 2 +- > drivers/regulator/stm32-vrefbuf.c | 2 +- > include/linux/regulator/driver.h | 3 ++- > 6 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c > index 1cf958983e86..b2342b3d78c7 100644 > --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c > +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c > @@ -185,7 +185,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, > cfg.init_data = &init_data; > cfg.ena_gpiod = int3472->regulator.gpio; > > - int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > + int3472->regulator.rdev = regulator_register(int3472->dev, > + &int3472->regulator.rdesc, > &cfg); > if (IS_ERR(int3472->regulator.rdev)) { > ret = PTR_ERR(int3472->regulator.rdev); > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > index 02ea917c7fd1..d7119b92c0b4 100644 > --- a/drivers/regulator/core.c > +++ b/drivers/regulator/core.c > @@ -5386,6 +5386,7 @@ static struct regulator_coupler generic_regulator_coupler = { > > /** > * regulator_register - register regulator > + * @dev: the device that drive the regulator > * @regulator_desc: regulator to register > * @cfg: runtime configuration for regulator > * > @@ -5394,7 +5395,8 @@ static struct regulator_coupler generic_regulator_coupler = { > * or an ERR_PTR() on error. > */ > struct regulator_dev * > -regulator_register(const struct regulator_desc *regulator_desc, > +regulator_register(struct device *dev, > + const struct regulator_desc *regulator_desc, > const struct regulator_config *cfg) > { > const struct regulator_init_data *init_data; > @@ -5403,7 +5405,6 @@ regulator_register(const struct regulator_desc *regulator_desc, > struct regulator_dev *rdev; > bool dangling_cfg_gpiod = false; > bool dangling_of_gpiod = false; > - struct device *dev; > int ret, i; > > if (cfg == NULL) > @@ -5415,8 +5416,7 @@ regulator_register(const struct regulator_desc *regulator_desc, > goto rinse; > } > > - dev = cfg->dev; > - WARN_ON(!dev); > + WARN_ON(!dev || !cfg->dev); > > if (regulator_desc->name == NULL || regulator_desc->ops == NULL) { > ret = -EINVAL; > diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c > index 32823a87fd40..d94db64cd490 100644 > --- a/drivers/regulator/devres.c > +++ b/drivers/regulator/devres.c > @@ -221,7 +221,7 @@ struct regulator_dev *devm_regulator_register(struct device *dev, > if (!ptr) > return ERR_PTR(-ENOMEM); > > - rdev = regulator_register(regulator_desc, config); > + rdev = regulator_register(dev, regulator_desc, config); > if (!IS_ERR(rdev)) { > *ptr = rdev; > devres_add(dev, ptr); > diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > index e12b681c72e5..bd0c5d1fd647 100644 > --- a/drivers/regulator/of_regulator.c > +++ b/drivers/regulator/of_regulator.c > @@ -505,7 +505,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, > struct device_node *child; > struct regulator_init_data *init_data = NULL; > > - child = regulator_of_get_init_node(dev, desc); > + child = regulator_of_get_init_node(config->dev, desc); > if (!child) > return NULL; > > diff --git a/drivers/regulator/stm32-vrefbuf.c b/drivers/regulator/stm32-vrefbuf.c > index 30ea3bc8ca19..7a454b7b6eab 100644 > --- a/drivers/regulator/stm32-vrefbuf.c > +++ b/drivers/regulator/stm32-vrefbuf.c > @@ -210,7 +210,7 @@ static int stm32_vrefbuf_probe(struct platform_device *pdev) > pdev->dev.of_node, > &stm32_vrefbuf_regu); > > - rdev = regulator_register(&stm32_vrefbuf_regu, &config); > + rdev = regulator_register(&pdev->dev, &stm32_vrefbuf_regu, &config); > if (IS_ERR(rdev)) { > ret = PTR_ERR(rdev); > dev_err(&pdev->dev, "register failed with error %d\n", ret); > diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h > index f9a7461e72b8..d3b4a3d4514a 100644 > --- a/include/linux/regulator/driver.h > +++ b/include/linux/regulator/driver.h > @@ -687,7 +687,8 @@ static inline int regulator_err2notif(int err) > > > struct regulator_dev * > -regulator_register(const struct regulator_desc *regulator_desc, > +regulator_register(struct device *dev, > + const struct regulator_desc *regulator_desc, > const struct regulator_config *config); > struct regulator_dev * > devm_regulator_register(struct device *dev, > -- > 2.35.1 >
