Re: [PATCH AUTOSEL 6.0 70/73] regulator: core: Use different devices for resource allocation and DT lookup

[ChiYuan Huang]

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
>