[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dc6c80f1-f34d-eaab-d561-32caa7fa140c@samsung.com>
Date: Tue, 13 Dec 2022 15:29:00 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: ChiYuan Huang <u0084500@...il.com>
Cc: broonie@...nel.org, djrscally@...il.com, hdegoede@...hat.com,
markgross@...nel.org, lgirdwood@...il.com,
mcoquelin.stm32@...il.com, alexandre.torgue@...s.st.com,
yangyingliang@...wei.com, gene_chen@...htek.com,
chiaen_wu@...htek.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org,
ChiYuan Huang <cy_huang@...htek.com>
Subject: Re: [PATCH v2] regulator: core: Use different devices for resource
allocation and DT lookup
Hi,
On 13.12.2022 15:19, ChiYuan Huang wrote:
> Marek Szyprowski <m.szyprowski@...sung.com> 於 2022年12月13日 週二 晚上7:33寫道:
>> On 06.12.2022 08:22, cy_huang wrote:
>>> From: ChiYuan Huang <cy_huang@...htek.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@...wei.com>
>>> Signed-off-by: ChiYuan Huang <cy_huang@...htek.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 = ®ulator_class;
> - rdev->dev.parent = dev;
> + rdev->dev.parent = config->dev;
> dev_set_name(&rdev->dev, "regulator.%lu",
> (unsigned long) atomic_inc_return(®ulator_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@...sung.com>
Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists