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

From: ChiYuan Huang <u0084500@gmail.com>
To: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: broonie@kernel.org, djrscally@gmail.com, hdegoede@redhat.com,
	 markgross@kernel.org, lgirdwood@gmail.com,
	mcoquelin.stm32@gmail.com,  alexandre.torgue@foss.st.com,
	yangyingliang@huawei.com, gene_chen@richtek.com,
	 chiaen_wu@richtek.com, platform-driver-x86@vger.kernel.org,
	 linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	 linux-arm-kernel@lists.infradead.org,
	ChiYuan Huang <cy_huang@richtek.com>
Subject: Re: [PATCH v2] regulator: core: Use different devices for resource allocation and DT lookup
Date: Wed, 14 Dec 2022 08:35:32 +0800	[thread overview]
Message-ID: <CADiBU3_+T1QiTHVkDbby_eoCS7Jg-AfCA5PS=rWhx_L9d_OAjA@mail.gmail.com> (raw)
In-Reply-To: <dc6c80f1-f34d-eaab-d561-32caa7fa140c@samsung.com>

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
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel