Re: [PATCH AUTOSEL 6.1 81/85] regulator: core: Use different devices for resource allocation and DT lookup

From: ChiYuan Huang
Date: Sun Dec 18 2022 - 20:08:44 EST

Next message: Gongwei Li: "[PATCH V1] usb: Fix typo in comment"
Previous message: Thomas Winter: "[PATCH 2/2] ip/ip6_gre: Fix non-point-to-point tunnel not generating IPv6 link local address"
In reply to: Sasha Levin: "[PATCH AUTOSEL 6.1 81/85] regulator: core: Use different devices for resource allocation and DT lookup"
Next in thread: Sasha Levin: "[PATCH AUTOSEL 6.1 85/85] Bluetooth: Add quirk to disable MWS Transport Configuration"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Dec 18, 2022 at 11:01: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 1cfac32121c0..10df84c2c288 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5402,6 +5402,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
> *
> @@ -5410,7 +5411,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;
> @@ -5419,7 +5421,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;
> bool resolved_early = false;
>
> @@ -5432,8 +5433,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 3265e75e97ab..5c7ff9b3e8a7 100644
> --- a/drivers/regulator/devres.c
> +++ b/drivers/regulator/devres.c
> @@ -385,7 +385,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 0aff1c2886b5..cd726d4e8fbf 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Next message: Gongwei Li: "[PATCH V1] usb: Fix typo in comment"
Previous message: Thomas Winter: "[PATCH 2/2] ip/ip6_gre: Fix non-point-to-point tunnel not generating IPv6 link local address"
In reply to: Sasha Levin: "[PATCH AUTOSEL 6.1 81/85] regulator: core: Use different devices for resource allocation and DT lookup"
Next in thread: Sasha Levin: "[PATCH AUTOSEL 6.1 85/85] Bluetooth: Add quirk to disable MWS Transport Configuration"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]