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 = ®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? > > > --- > > 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 >
