[v2] regulator: core: Use different devices for resource allocation and DT lookup

  From: ChiYuan Huang <cy_huang@richtek.com>

Following by the below discussion, there's the potential UAF issue
between regulator and mfd.
https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/

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@huawei.com>
Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
loop Yang Yingliang in cc list.

Since v2
- Fix typo 'int3742' to 'int3472' for kernel build test

---
 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(-)
  

On Tue, 06 Dec 2022 15:22:21 +0800, cy_huang wrote:
> Following by the below discussion, there's the potential UAF issue
> between regulator and mfd.
> https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/
> 
> 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()
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: core: Use different devices for resource allocation and DT lookup
      commit: 8f3cbcd6b440032ebc7f7d48a1689dcc70a4eb98

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
  

Dear All,

On 06.12.2022 08:22, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Following by the below discussion, there's the potential UAF issue
> between regulator and mfd.
> https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/
>
> >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@huawei.com>
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>


This patch landed in linux-next 202212 as commit 8f3cbcd6b440 
("regulator: core: Use different devices for resource allocation and DT 
lookup"). Unfortunately it causes serious regression on my test systems. 
It looks that some supplies are not resolved correctly and then turned 
off as 'unused', even if they provide power to other core regulators in 
the system. I've observed this issue on Samsung Chromebook Peach-Pit and 
Peach-Pi (ARM 32bit Exynos based). The symptoms are somehow similar to 
the issue reported here some time ago:

https://lore.kernel.org/all/58b92e75-f373-dae7-7031-8abd465bb874@samsung.com/

I've post more information once I analyze this issue further.


> ---
> loop Yang Yingliang in cc list.
>
> Since v2
> - Fix typo 'int3742' to 'int3472' for kernel build test
>
> ---
>   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(-)
>
> ...

Best regards
  

HI,

Marek Szyprowski <m.szyprowski@samsung.com> 於 2022年12月13日 週二 晚上7:33寫道：
>
> Dear All,
>
> On 06.12.2022 08:22, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Following by the below discussion, there's the potential UAF issue
> > between regulator and mfd.
> > https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/
> >
> > >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@huawei.com>
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>
>
> This patch landed in linux-next 202212 as commit 8f3cbcd6b440
> ("regulator: core: Use different devices for resource allocation and DT
> lookup"). Unfortunately it causes serious regression on my test systems.
> It looks that some supplies are not resolved correctly and then turned
> off as 'unused', even if they provide power to other core regulators in
> the system. I've observed this issue on Samsung Chromebook Peach-Pit and
> Peach-Pi (ARM 32bit Exynos based). The symptoms are somehow similar to
> the issue reported here some time ago:
>
> https://lore.kernel.org/all/58b92e75-f373-dae7-7031-8abd465bb874@samsung.com/
>
> I've post more information once I analyze this issue further.
>
It seems the issue occurs in 'regulator register' resolve supply.
Due to the parent device don't have the of_node, to resolve the
supply, it may need to get the
dt node by recursively finding child regulator.
Like this
parent {
  regulators {
     xxx-supply = <&vdd12-ldo>;
     vdd12-ldo: vdd12-ldo {
         regulator-name = "xxx";
         regulator-min-microvolts = <xxxxx>;
         regulator-max-microvolts = <xxxxx>;
      }
  };
};
From this case, 'resolve supply' need to parse at least the more top
level like 'regulators'.
But now, it only take 'vdd12-ldo' as the node or its child to parse its supply.

Below's the fix I guess.
It'll make the parent of the regulator the same as the dev parameter
in 'regulator_config'.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ea4a720..7c5036e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5526,7 +5526,7 @@ regulator_register(struct device *dev,

        /* register with sysfs */
        rdev->dev.class = &regulator_class;
-       rdev->dev.parent = dev;
+       rdev->dev.parent = config->dev;
        dev_set_name(&rdev->dev, "regulator.%lu",
                    (unsigned long) atomic_inc_return(&regulator_no));
        dev_set_drvdata(&rdev->dev, rdev);

I don't have the board. Could you help to test this change to see
whether it's been fixed or not?
>
> > ---
> > loop Yang Yingliang in cc list.
> >
> > Since v2
> > - Fix typo 'int3742' to 'int3472' for kernel build test
> >
> > ---
> >   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(-)
> >
> > ...
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
  

Hi,

On 13.12.2022 15:19, ChiYuan Huang wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> 於 2022年12月13日 週二 晚上7:33寫道：
>> On 06.12.2022 08:22, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@richtek.com>
>>>
>>> Following by the below discussion, there's the potential UAF issue
>>> between regulator and mfd.
>>> https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/
>>>
>>> >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@huawei.com>
>>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>>
>> This patch landed in linux-next 202212 as commit 8f3cbcd6b440
>> ("regulator: core: Use different devices for resource allocation and DT
>> lookup"). Unfortunately it causes serious regression on my test systems.
>> It looks that some supplies are not resolved correctly and then turned
>> off as 'unused', even if they provide power to other core regulators in
>> the system. I've observed this issue on Samsung Chromebook Peach-Pit and
>> Peach-Pi (ARM 32bit Exynos based). The symptoms are somehow similar to
>> the issue reported here some time ago:
>>
>> https://lore.kernel.org/all/58b92e75-f373-dae7-7031-8abd465bb874@samsung.com/
>>
>> I've post more information once I analyze this issue further.
>>
> It seems the issue occurs in 'regulator register' resolve supply.
> Due to the parent device don't have the of_node, to resolve the
> supply, it may need to get the
> dt node by recursively finding child regulator.
> Like this
> parent {
>    regulators {
>       xxx-supply = <&vdd12-ldo>;
>       vdd12-ldo: vdd12-ldo {
>           regulator-name = "xxx";
>           regulator-min-microvolts = <xxxxx>;
>           regulator-max-microvolts = <xxxxx>;
>        }
>    };
> };
> >From this case, 'resolve supply' need to parse at least the more top
> level like 'regulators'.
> But now, it only take 'vdd12-ldo' as the node or its child to parse its supply.
>
> Below's the fix I guess.
> It'll make the parent of the regulator the same as the dev parameter
> in 'regulator_config'.
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index ea4a720..7c5036e 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5526,7 +5526,7 @@ regulator_register(struct device *dev,
>
>          /* register with sysfs */
>          rdev->dev.class = &regulator_class;
> -       rdev->dev.parent = dev;
> +       rdev->dev.parent = config->dev;
>          dev_set_name(&rdev->dev, "regulator.%lu",
>                      (unsigned long) atomic_inc_return(&regulator_no));
>          dev_set_drvdata(&rdev->dev, rdev);
>
> I don't have the board. Could you help to test this change to see
> whether it's been fixed or not?

The above change fixes the issue. Thanks! Feel free to add following 
tags to the final patch:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


Best regards
  

On Tue, Dec 13, 2022 at 03:29:00PM +0100, Marek Szyprowski wrote:
> On 13.12.2022 15:19, ChiYuan Huang wrote:

> > I don't have the board. Could you help to test this change to see
> > whether it's been fixed or not?

> The above change fixes the issue. Thanks! Feel free to add following 
> tags to the final patch:

> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for jumping on this so quickly!  Marek, are these boards (or
similar ones) generally available?  They seem great at showing up issues
so it'd be good if I could get them into my CI and spot problems earlier
(or something like kernelci.org would be about as good)?
  

On 13.12.2022 17:32, Mark Brown wrote:
> On Tue, Dec 13, 2022 at 03:29:00PM +0100, Marek Szyprowski wrote:
>> On 13.12.2022 15:19, ChiYuan Huang wrote:
>>> I don't have the board. Could you help to test this change to see
>>> whether it's been fixed or not?
>> The above change fixes the issue. Thanks! Feel free to add following
>> tags to the final patch:
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Thanks for jumping on this so quickly!  Marek, are these boards (or
> similar ones) generally available?  They seem great at showing up issues
> so it'd be good if I could get them into my CI and spot problems earlier
> (or something like kernelci.org would be about as good)?

Well, they are quite old, but I've seen some used ones on eBay. Just 
look for "Samsung Chomebook XE503C12" (Peach-Pit) or XE503C32 (Peach-Pi, 
very rare).

Getting it integrated into the CI requires a bit of manual work. You 
have to solder UART lines to the test points on the motherboard. The 
board can be fully controlled via so called Embedded Controller, what in 
practice means that you can turn power on/off by sending commands to the 
dedicated EC UART. The kernel console UART uses 1.8V, while the EC UART 
- 3.3V. Let me know if you need more details.

Peach-Pi was used on kernelci.org some time ago, but I have no idea what 
has happened to it.

Best regards
  

Marek Szyprowski <m.szyprowski@samsung.com> 於 2022年12月13日 週二 晚上10:29寫道：
>
> Hi,
>
> On 13.12.2022 15:19, ChiYuan Huang wrote:
> > Marek Szyprowski <m.szyprowski@samsung.com> 於 2022年12月13日 週二 晚上7:33寫道：
> >> On 06.12.2022 08:22, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@richtek.com>
> >>>
> >>> Following by the below discussion, there's the potential UAF issue
> >>> between regulator and mfd.
> >>> https://lore.kernel.org/all/20221128143601.1698148-1-yangyingliang@huawei.com/
> >>>
> >>> >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@huawei.com>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >>
> >> This patch landed in linux-next 202212 as commit 8f3cbcd6b440
> >> ("regulator: core: Use different devices for resource allocation and DT
> >> lookup"). Unfortunately it causes serious regression on my test systems.
> >> It looks that some supplies are not resolved correctly and then turned
> >> off as 'unused', even if they provide power to other core regulators in
> >> the system. I've observed this issue on Samsung Chromebook Peach-Pit and
> >> Peach-Pi (ARM 32bit Exynos based). The symptoms are somehow similar to
> >> the issue reported here some time ago:
> >>
> >> https://lore.kernel.org/all/58b92e75-f373-dae7-7031-8abd465bb874@samsung.com/
> >>
> >> I've post more information once I analyze this issue further.
> >>
> > It seems the issue occurs in 'regulator register' resolve supply.
> > Due to the parent device don't have the of_node, to resolve the
> > supply, it may need to get the
> > dt node by recursively finding child regulator.
> > Like this
> > parent {
> >    regulators {
> >       xxx-supply = <&vdd12-ldo>;
> >       vdd12-ldo: vdd12-ldo {
> >           regulator-name = "xxx";
> >           regulator-min-microvolts = <xxxxx>;
> >           regulator-max-microvolts = <xxxxx>;
> >        }
> >    };
> > };
> > >From this case, 'resolve supply' need to parse at least the more top
> > level like 'regulators'.
> > But now, it only take 'vdd12-ldo' as the node or its child to parse its supply.
> >
> > Below's the fix I guess.
> > It'll make the parent of the regulator the same as the dev parameter
> > in 'regulator_config'.
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index ea4a720..7c5036e 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -5526,7 +5526,7 @@ regulator_register(struct device *dev,
> >
> >          /* register with sysfs */
> >          rdev->dev.class = &regulator_class;
> > -       rdev->dev.parent = dev;
> > +       rdev->dev.parent = config->dev;
> >          dev_set_name(&rdev->dev, "regulator.%lu",
> >                      (unsigned long) atomic_inc_return(&regulator_no));
> >          dev_set_drvdata(&rdev->dev, rdev);
> >
> > I don't have the board. Could you help to test this change to see
> > whether it's been fixed or not?
>
> The above change fixes the issue. Thanks! Feel free to add following
> tags to the final patch:
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
Thanks.
I'll submit the change to fix it.

>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
  

On Tue, Dec 13, 2022 at 11:23:01PM +0100, Marek Szyprowski wrote:
> On 13.12.2022 17:32, Mark Brown wrote:

> > Thanks for jumping on this so quickly!  Marek, are these boards (or
> > similar ones) generally available?  They seem great at showing up issues
> > so it'd be good if I could get them into my CI and spot problems earlier
> > (or something like kernelci.org would be about as good)?

> Well, they are quite old, but I've seen some used ones on eBay. Just 
> look for "Samsung Chomebook XE503C12" (Peach-Pit) or XE503C32 (Peach-Pi, 
> very rare).

> Getting it integrated into the CI requires a bit of manual work. You 
> have to solder UART lines to the test points on the motherboard. The 
> board can be fully controlled via so called Embedded Controller, what in 
> practice means that you can turn power on/off by sending commands to the 
> dedicated EC UART. The kernel console UART uses 1.8V, while the EC UART 
> - 3.3V. Let me know if you need more details.

Ah, oh dear - I'm afraid my soldering ability isn't up to that sort of
modification.

> Peach-Pi was used on kernelci.org some time ago, but I have no idea what 
> has happened to it.

I suspect it might've fallen off the end of the ChromeOS support window 
and got pulled, I'll see if I can find out.  Perhaps there's some
modified boards kicking about that didn't get thrown out.