From: ChiYuan Huang <[email protected]>
[ Upstream commit 8f3cbcd6b440032ebc7f7d48a1689dcc70a4eb98 ]
Following by the below discussion, there's the potential UAF issue
between regulator and mfd.
https://lore.kernel.org/all/[email protected]/
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 <[email protected]>
Signed-off-by: ChiYuan Huang <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Mark Brown <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
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
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/[email protected]/
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 <[email protected]>
>
> [ Upstream commit 8f3cbcd6b440032ebc7f7d48a1689dcc70a4eb98 ]
>
> Following by the below discussion, there's the potential UAF issue
> between regulator and mfd.
> https://lore.kernel.org/all/[email protected]/
>
> >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 <[email protected]>
> Signed-off-by: ChiYuan Huang <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> Signed-off-by: Mark Brown <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> 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
>
On Mon, Dec 19, 2022 at 09:07:01AM +0800, ChiYuan Huang wrote:
>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/[email protected]/
>
> 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")
I'll take it too, thanks!
--
Thanks,
Sasha