[PATCH] mfd: mt6370: fix potential null-ptr-deref and uaf while removing device
ChiYuan Huang
u0084500 at gmail.com
Tue Nov 29 18:13:48 PST 2022
On Mon, Nov 28, 2022 at 10:36:01PM +0800, Yang Yingliang wrote:
> I got the a UAF and a null-ptr-deref reports while
> doing module loading test:
>
> ==================================================================
> BUG: KASAN: use-after-free in regulator_register.cold.70+0x191/0xe78
> Read of size 8 at addr ffff888118fb8c28 by task systemd-udevd/261
>
> CPU: 2 PID: 261 Comm: systemd-udevd Tainted: G N 6.1.0-rc3+
> Call Trace:
> <TASK>
> dump_stack_lvl+0x67/0x83
> print_report+0x178/0x4b0
> kasan_report+0x90/0x190
> regulator_register.cold.70+0x191/0xe78
> devm_regulator_register+0x57/0xb0
> mt6370_regulator_probe.cold.1+0xd8/0x222 [mt6370_regulator]
>
> Allocated by task 261:
> kasan_save_stack+0x1c/0x40
> kasan_set_track+0x21/0x30
> __kasan_kmalloc+0x7e/0x90
> __kmalloc_node_track_caller+0x55/0x1b0
> devm_kmalloc+0x5e/0x110
> of_get_regulator_init_data+0x9e/0xd60
> regulator_of_get_init_data+0x1a3/0x270
> regulator_register+0x227/0x620
> devm_regulator_register+0x57/0xb0
> mt6370_regulator_probe.cold.1+0xd8/0x222 [mt6370_regulator]
>
> Freed by task 250:
> kasan_save_stack+0x1c/0x40
> kasan_set_track+0x21/0x30
> kasan_save_free_info+0x2a/0x50
> __kasan_slab_free+0x102/0x190
> __kmem_cache_free+0xca/0x400
> release_nodes+0x9b/0xb9
> devres_release_group.cold.10+0x5f/0x64
> i2c_device_remove+0x60/0xf0
> device_remove+0x73/0xc0
> device_release_driver_internal+0x12d/0x210
> bus_remove_device+0x1bd/0x240
> device_del+0x357/0x770
> i2c_unregister_device.part.22+0xe2/0x100
> i2c_unregister_device+0x29/0x30
> ==================================================================
>
> Here is how UAF happens:
>
> 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()
>
> While probing mt6370 regulator, the init_data is allocated in
> regulator_of_get_init_data(), and it's added to devres of
> device mt6370. When the device mt6370 is deleting, the devres
> is freed, then it causes a UAF in regulator_register().
>
> ==================================================================
> BUG: kernel NULL pointer dereference, address: 0000000000000354
> CPU: 2 PID: 261 Comm: systemd-udevd Tainted: G B N 6.1.0-rc3+
> RIP: 0010:regmap_read+0x21/0xc0
> Call Trace:
> <TASK>
> regulator_get_voltage_sel_regmap+0x98/0x110
> regulator_attr_is_visible+0x2da/0x4c0
> internal_create_group+0x1f5/0x6b0
> internal_create_groups.part.4+0x65/0xe0
> sysfs_create_groups+0x24/0x50
> device_add+0x5a9/0x1100
> regulator_register.cold.70+0xb06/0xe78
> devm_regulator_register+0x57/0xb0
> mt6370_regulator_probe.cold.1+0xd8/0x222 [mt6370_regulator]
> ==================================================================
>
> Here is how null-ptr-deref happens:
>
> CPU A |CPU B
> mt6370_probe() |
> // add regmap to devres |
> devm_regmap_init() |
> devm_mfd_add_devices() |
> |
> i2c_unregister_device() |
> device_del() |
> devres_release_all() |
> // regmap is removed |
> remove_nodes() |
> |mt6370_regulator_probe()
> | regulator_register()
> | // can't find regmap, it's NULL
> | devm_get_regmap()
> | device_add()
> | // causes null-ptr-deref
> | regulator_get_voltage_sel_regmap()
>
> While probing mt6370 regulator, the mt6370 is deleting, the devres
> are removed including regmap, so in regulator_register() get a NULL
> regmap pointer, then it causes null-ptr-derf while using it in
> regulator_get_voltage_sel_regmap().
>
> To fix theses races, switch to use non-devm function to add mfd
> devices, make removing child(regulator) devices earlier than
> freeing parent device resources, so the devres can not be freed
> until mt6370_regulator_probe() is finished.
It means the root cause is by 'mt6370-regulator', not mt6370 mfd driver.
Bucause there's no of_node for this regulator mfd cell.
That's why I directly use the parent dev (mt6370 i2c dev) to reuse
'regulator_of_get_init_data'
But I didn't notice this will cause the devres race condition.
To better solve it, I'll revise 'mt6370-regulator' code to
prevent this case by 'of_regulator_match' func.
Thanks for the issue reporting.
Best regards,
ChiYuan Huang.
>
> Fixes: b2adf788e603 ("mfd: mt6370: Add MediaTek MT6370 support")
> Signed-off-by: Yang Yingliang <yangyingliang at huawei.com>
> ---
> drivers/mfd/mt6370.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mfd/mt6370.c b/drivers/mfd/mt6370.c
> index cf19cce2fdc0..dd19146612cf 100644
> --- a/drivers/mfd/mt6370.c
> +++ b/drivers/mfd/mt6370.c
> @@ -268,28 +268,33 @@ static int mt6370_probe(struct i2c_client *i2c)
> switch (vid) {
> case MT6370_VENID_MT6372P:
> case MT6370_VENID_MT6372CP:
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> - mt6372_exclusive_devices,
> - ARRAY_SIZE(mt6372_exclusive_devices),
> - NULL, 0,
> - regmap_irq_get_domain(info->irq_data));
> + ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + mt6372_exclusive_devices,
> + ARRAY_SIZE(mt6372_exclusive_devices),
> + NULL, 0,
> + regmap_irq_get_domain(info->irq_data));
> break;
> default:
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> - mt6370_exclusive_devices,
> - ARRAY_SIZE(mt6370_exclusive_devices),
> - NULL, 0,
> - regmap_irq_get_domain(info->irq_data));
> + ret = mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + mt6370_exclusive_devices,
> + ARRAY_SIZE(mt6370_exclusive_devices),
> + NULL, 0,
> + regmap_irq_get_domain(info->irq_data));
> break;
> }
>
> if (ret)
> return dev_err_probe(dev, ret, "Failed to add the exclusive devices\n");
>
> - return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> - mt6370_devices, ARRAY_SIZE(mt6370_devices),
> - NULL, 0,
> - regmap_irq_get_domain(info->irq_data));
> + return mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + mt6370_devices, ARRAY_SIZE(mt6370_devices),
> + NULL, 0,
> + regmap_irq_get_domain(info->irq_data));
> +}
> +
> +static void mt6370_remove(struct i2c_client *i2c)
> +{
> + mfd_remove_devices(&i2c->dev);
> }
>
> static const struct of_device_id mt6370_match_table[] = {
> @@ -304,6 +309,7 @@ static struct i2c_driver mt6370_driver = {
> .of_match_table = mt6370_match_table,
> },
> .probe_new = mt6370_probe,
> + .remove = mt6370_remove,
> };
> module_i2c_driver(mt6370_driver);
>
> --
> 2.25.1
>
>
More information about the Linux-mediatek
mailing list