diff mbox series

mfd: mt6370: fix potential null-ptr-deref and uaf while removing device

Message ID 20221128143601.1698148-1-yangyingliang@huawei.com (mailing list archive)
State New, archived
Headers show
Series mfd: mt6370: fix potential null-ptr-deref and uaf while removing device | expand

Commit Message

Yang Yingliang Nov. 28, 2022, 2:36 p.m. UTC 
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.

Fixes: b2adf788e603 ("mfd: mt6370: Add MediaTek MT6370 support")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/mfd/mt6370.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

Comments

ChiYuan Huang Nov. 30, 2022, 2:13 a.m. UTC | #1 
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@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
> 
>
diff mbox series

Patch

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);