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

[ChiYuan Huang]

HI,

Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 於 2022年12月13日 週二 晚上7:33寫道：
>
> Dear All,
>
> On 06.12.2022 08:22, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >
> > 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>
>
>
> 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@xxxxxxxxxxx/
>
> 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
>