Hi, On 13.12.2022 15:19, ChiYuan Huang wrote: > Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> 於 2022年12月13日 週二 晚上7:33寫道： >> 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 = ®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@xxxxxxxxxxx> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
