regulator: core: Use different devices for resource allocation and DT lookup
Commit Message
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>
---
Hi, Mark:
I have already tested on MT6370. It works.
Due to 'regulator_register' API change, There're still two existing users use it.
I also modify it by the way.
---
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(-)
Comments
Hi cy_huang,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on broonie-regulator/for-next]
[also build test ERROR on linus/master v6.1-rc8 next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/cy_huang/regulator-core-Use-different-devices-for-resource-allocation-and-DT-lookup/20221206-110134
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link: https://lore.kernel.org/r/1670295548-19598-1-git-send-email-u0084500%40gmail.com
patch subject: [PATCH] regulator: core: Use different devices for resource allocation and DT lookup
config: x86_64-allmodconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/7e148c1165ffaafd082958ed931135458ade32d3
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review cy_huang/regulator-core-Use-different-devices-for-resource-allocation-and-DT-lookup/20221206-110134
git checkout 7e148c1165ffaafd082958ed931135458ade32d3
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/platform/x86/intel/int3472/clk_and_regulator.c: In function 'skl_int3472_register_regulator':
>> drivers/platform/x86/intel/int3472/clk_and_regulator.c:188:54: error: 'int3742' undeclared (first use in this function); did you mean 'int3472'?
188 | int3472->regulator.rdev = regulator_register(int3742->dev,
| ^~~~~~~
| int3472
drivers/platform/x86/intel/int3472/clk_and_regulator.c:188:54: note: each undeclared identifier is reported only once for each function it appears in
vim +188 drivers/platform/x86/intel/int3472/clk_and_regulator.c
138
139 int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
140 struct acpi_resource_gpio *agpio)
141 {
142 const struct int3472_sensor_config *sensor_config;
143 char *path = agpio->resource_source.string_ptr;
144 struct regulator_consumer_supply supply_map;
145 struct regulator_init_data init_data = { };
146 struct regulator_config cfg = { };
147 int ret;
148
149 sensor_config = int3472->sensor_config;
150 if (IS_ERR(sensor_config)) {
151 dev_err(int3472->dev, "No sensor module config\n");
152 return PTR_ERR(sensor_config);
153 }
154
155 if (!sensor_config->supply_map.supply) {
156 dev_err(int3472->dev, "No supply name defined\n");
157 return -ENODEV;
158 }
159
160 init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
161 init_data.num_consumer_supplies = 1;
162 supply_map = sensor_config->supply_map;
163 supply_map.dev_name = int3472->sensor_name;
164 init_data.consumer_supplies = &supply_map;
165
166 snprintf(int3472->regulator.regulator_name,
167 sizeof(int3472->regulator.regulator_name), "%s-regulator",
168 acpi_dev_name(int3472->adev));
169 snprintf(int3472->regulator.supply_name,
170 GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
171
172 int3472->regulator.rdesc = INT3472_REGULATOR(
173 int3472->regulator.regulator_name,
174 int3472->regulator.supply_name,
175 &int3472_gpio_regulator_ops);
176
177 int3472->regulator.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0],
178 "int3472,regulator");
179 if (IS_ERR(int3472->regulator.gpio)) {
180 dev_err(int3472->dev, "Failed to get regulator GPIO line\n");
181 return PTR_ERR(int3472->regulator.gpio);
182 }
183
184 cfg.dev = &int3472->adev->dev;
185 cfg.init_data = &init_data;
186 cfg.ena_gpiod = int3472->regulator.gpio;
187
> 188 int3472->regulator.rdev = regulator_register(int3742->dev,
189 &int3472->regulator.rdesc,
190 &cfg);
191 if (IS_ERR(int3472->regulator.rdev)) {
192 ret = PTR_ERR(int3472->regulator.rdev);
193 goto err_free_gpio;
194 }
195
196 return 0;
197
198 err_free_gpio:
199 gpiod_put(int3472->regulator.gpio);
200
201 return ret;
202 }
203
@@ -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(int3742->dev,
+ &int3472->regulator.rdesc,
&cfg);
if (IS_ERR(int3472->regulator.rdev)) {
ret = PTR_ERR(int3472->regulator.rdev);
@@ -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;
@@ -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);
@@ -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;
@@ -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);
@@ -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,