| Message ID | 1670311341-32664-1-git-send-email-u0084500@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [v2] regulator: core: Use different devices for resource allocation and DT lookup | expand |
On Tue, 06 Dec 2022 15:22:21 +0800, cy_huang wrote: > 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() > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [1/1] regulator: core: Use different devices for resource allocation and DT lookup commit: 8f3cbcd6b440032ebc7f7d48a1689dcc70a4eb98 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Dear All, 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. > --- > 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
HI, Marek Szyprowski <m.szyprowski@samsung.com> 於 2022年12月13日 週二 晚上7:33寫道： > > Dear All, > > 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 = ®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 >
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 = ®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@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Best regards
On Tue, Dec 13, 2022 at 03:29:00PM +0100, Marek Szyprowski wrote: > On 13.12.2022 15:19, ChiYuan Huang wrote: > > 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 for jumping on this so quickly! Marek, are these boards (or similar ones) generally available? They seem great at showing up issues so it'd be good if I could get them into my CI and spot problems earlier (or something like kernelci.org would be about as good)?
On 13.12.2022 17:32, Mark Brown wrote: > On Tue, Dec 13, 2022 at 03:29:00PM +0100, Marek Szyprowski wrote: >> On 13.12.2022 15:19, ChiYuan Huang wrote: >>> 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 for jumping on this so quickly! Marek, are these boards (or > similar ones) generally available? They seem great at showing up issues > so it'd be good if I could get them into my CI and spot problems earlier > (or something like kernelci.org would be about as good)? Well, they are quite old, but I've seen some used ones on eBay. Just look for "Samsung Chomebook XE503C12" (Peach-Pit) or XE503C32 (Peach-Pi, very rare). Getting it integrated into the CI requires a bit of manual work. You have to solder UART lines to the test points on the motherboard. The board can be fully controlled via so called Embedded Controller, what in practice means that you can turn power on/off by sending commands to the dedicated EC UART. The kernel console UART uses 1.8V, while the EC UART - 3.3V. Let me know if you need more details. Peach-Pi was used on kernelci.org some time ago, but I have no idea what has happened to it. Best regards
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 = ®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@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 >
On Tue, Dec 13, 2022 at 11:23:01PM +0100, Marek Szyprowski wrote: > On 13.12.2022 17:32, Mark Brown wrote: > > Thanks for jumping on this so quickly! Marek, are these boards (or > > similar ones) generally available? They seem great at showing up issues > > so it'd be good if I could get them into my CI and spot problems earlier > > (or something like kernelci.org would be about as good)? > Well, they are quite old, but I've seen some used ones on eBay. Just > look for "Samsung Chomebook XE503C12" (Peach-Pit) or XE503C32 (Peach-Pi, > very rare). > Getting it integrated into the CI requires a bit of manual work. You > have to solder UART lines to the test points on the motherboard. The > board can be fully controlled via so called Embedded Controller, what in > practice means that you can turn power on/off by sending commands to the > dedicated EC UART. The kernel console UART uses 1.8V, while the EC UART > - 3.3V. Let me know if you need more details. Ah, oh dear - I'm afraid my soldering ability isn't up to that sort of modification. > Peach-Pi was used on kernelci.org some time ago, but I have no idea what > has happened to it. I suspect it might've fallen off the end of the ChromeOS support window and got pulled, I'll see if I can find out. Perhaps there's some modified boards kicking about that didn't get thrown out.
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index 1cf9589..b2342b3 100644 --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c @@ -185,7 +185,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, cfg.init_data = &init_data; cfg.ena_gpiod = int3472->regulator.gpio; - int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, + int3472->regulator.rdev = regulator_register(int3472->dev, + &int3472->regulator.rdesc, &cfg); if (IS_ERR(int3472->regulator.rdev)) { ret = PTR_ERR(int3472->regulator.rdev); diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index e8c00a8..ea4a720 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -5396,6 +5396,7 @@ static struct regulator_coupler generic_regulator_coupler = { /** * regulator_register - register regulator + * @dev: the device that drive the regulator * @regulator_desc: regulator to register * @cfg: runtime configuration for regulator * @@ -5404,7 +5405,8 @@ static struct regulator_coupler generic_regulator_coupler = { * or an ERR_PTR() on error. */ struct regulator_dev * -regulator_register(const struct regulator_desc *regulator_desc, +regulator_register(struct device *dev, + const struct regulator_desc *regulator_desc, const struct regulator_config *cfg) { const struct regulator_init_data *init_data; @@ -5413,7 +5415,6 @@ regulator_register(const struct regulator_desc *regulator_desc, struct regulator_dev *rdev; bool dangling_cfg_gpiod = false; bool dangling_of_gpiod = false; - struct device *dev; int ret, i; bool resolved_early = false; @@ -5426,8 +5427,7 @@ regulator_register(const struct regulator_desc *regulator_desc, goto rinse; } - dev = cfg->dev; - WARN_ON(!dev); + WARN_ON(!dev || !cfg->dev); if (regulator_desc->name == NULL || regulator_desc->ops == NULL) { ret = -EINVAL; diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c index 3265e75..5c7ff9b 100644 --- a/drivers/regulator/devres.c +++ b/drivers/regulator/devres.c @@ -385,7 +385,7 @@ struct regulator_dev *devm_regulator_register(struct device *dev, if (!ptr) return ERR_PTR(-ENOMEM); - rdev = regulator_register(regulator_desc, config); + rdev = regulator_register(dev, regulator_desc, config); if (!IS_ERR(rdev)) { *ptr = rdev; devres_add(dev, ptr); diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c index 0aff1c2..cd726d4 100644 --- a/drivers/regulator/of_regulator.c +++ b/drivers/regulator/of_regulator.c @@ -505,7 +505,7 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev, struct device_node *child; struct regulator_init_data *init_data = NULL; - child = regulator_of_get_init_node(dev, desc); + child = regulator_of_get_init_node(config->dev, desc); if (!child) return NULL; diff --git a/drivers/regulator/stm32-vrefbuf.c b/drivers/regulator/stm32-vrefbuf.c index 30ea3bc..7a454b7 100644 --- a/drivers/regulator/stm32-vrefbuf.c +++ b/drivers/regulator/stm32-vrefbuf.c @@ -210,7 +210,7 @@ static int stm32_vrefbuf_probe(struct platform_device *pdev) pdev->dev.of_node, &stm32_vrefbuf_regu); - rdev = regulator_register(&stm32_vrefbuf_regu, &config); + rdev = regulator_register(&pdev->dev, &stm32_vrefbuf_regu, &config); if (IS_ERR(rdev)) { ret = PTR_ERR(rdev); dev_err(&pdev->dev, "register failed with error %d\n", ret); diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h index f9a7461..d3b4a3d 100644 --- a/include/linux/regulator/driver.h +++ b/include/linux/regulator/driver.h @@ -687,7 +687,8 @@ static inline int regulator_err2notif(int err) struct regulator_dev * -regulator_register(const struct regulator_desc *regulator_desc, +regulator_register(struct device *dev, + const struct regulator_desc *regulator_desc, const struct regulator_config *config); struct regulator_dev * devm_regulator_register(struct device *dev,