mfd: twl-core: unregister the auxiliary platform device on unbind
Subsystems (3)
PATCH 1/1 mfd: twl-core: unregister the auxiliary platform device on unbind
twl_probe() registers a standalone platform device named "twl", but keeps its pointer only in a local variable. Probe errors unregister the device, while successful probe leaves no way for twl_remove() to release it. The platform device therefore remains registered after the I2C driver unbinds.
Register a managed action on the I2C device immediately after adding the platform device. This unregisters it on probe failure and driver unbind, and also handles failure to register the action itself. Remove the manual error-path unregister to avoid releasing the device twice.
This issue was identified during our ongoing static-analysis research while reviewing kernel code.
--- drivers/mfd/twl-core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/mfd/twl-core.c +15 -8
@@ -719,6 +719,11 @@ static const struct mfd_cell twl6032_cells[] = { { .name = "twl6032-clk" }, }; +static void twl_unregister_device (void *data) +{ +platform_device_unregister (data); +} +/* NOTE: This driver only handles a single twl4030/tps659x0 chip */ static int twl_probe (struct i2c_client *client)
@@ -754,17 +759,22 @@ twl_probe(struct i2c_client *client)return status; } + status =devm_add_action_or_reset (&client->dev, twl_unregister_device, + pdev); +if (status) +return status; +if (!i2c_check_functionality (client->adapter,I2C_FUNC_I2C )) {dev_dbg (&client->dev,"can't talk I2C?\n" ); status = -EIO ; - goto free; + goto out; } twl_priv =devm_kzalloc (&client->dev,sizeof (struct twl_private ),GFP_KERNEL );if (!twl_priv) { status = -ENOMEM ; - goto free; + goto out; }if ((id->driver_data) &TWL6030_CLASS ) {
@@ -784,7 +794,7 @@ twl_probe(struct i2c_client *client) GFP_KERNEL);if (!twl_priv->twl_modules) { status = -ENOMEM ; - goto free; + goto out; }for (i =0 ; i < num_slaves; i++) {
@@ -880,7 +890,7 @@ twl_probe(struct i2c_client *client) status =devm_mfd_add_devices (&client->dev,PLATFORM_DEVID_NONE , cells, num_cells,NULL ,0 ,NULL );if (status <0 ) - goto free; + goto out;if (of_device_is_system_power_controller (node)) {if (!pm_power_off)
@@ -896,10 +906,7 @@ twl_probe(struct i2c_client *client) fail:if (status <0 )twl_remove (client); -free: -if (status <0 ) -platform_device_unregister (pdev); - +out:return status; }
Signature · 1 line
2.39.5
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] The commit message subject violates the MFD subsystem capitalization rules.
Pre-existing issues:
- [Critical] The global pointer twl_priv is left dangling on probe failure or driver unbind, leading to Use-After-Free. --
commit e934e995c4acbf6c01e2cc4e04fc84cdb6e5e228
Author: Myeonghun Pak <[email protected]>
mfd: twl-core: unregister the auxiliary platform device on unbind
This commit registers a managed action to unregister the auxiliary platform device on unbind. It fixes an issue where successful probes left no way for twl_remove() to release the device, and also removes the manual error-path unregister step.
Quoted 1 line
mfd: twl-core: unregister the auxiliary platform device on unbind
[Severity: Low]
This isn't a bug, but does this subject line violate the MFD subsystem capitalization rules? The MFD guidelines state that the description after the subsystem prefix should be capitalized. For example, it could be formatted as "mfd: twl-core: Unregister the auxiliary platform device...".
Quoted 4 lines
drivers/mfd/twl-core.c +0
-0
[ ... ]
Quoted 1 line
drivers/mfd/twl-core.c +0
-0@@ -754,17 +759,22 @@ twl_probe(struct i2c_client *client)
[ ... ]
drivers/mfd/twl-core.c twl_probe(struct i2c_client *client) · line 778
Quoted 7 lines
drivers/mfd/twl-core.c +1
-1twl_priv =devm_kzalloc (&client->dev,sizeof (struct twl_private ),GFP_KERNEL );if (!twl_priv) { status = -ENOMEM ; - goto free; + goto out; }
[Severity: Critical]
This isn't a bug introduced by this patch, but is there a use-after-free risk with how the twl_priv global pointer is managed?
Because twl_priv is allocated with devm_kzalloc(), the devres subsystem will automatically free this memory if the driver unbinds or if the probe fails later. However, twl_priv is a global pointer and it is never set to NULL in the twl_remove() function.
Since child devices created via of_platform_populate() are unmanaged and never depopulated in the remove path, they remain registered. If a user subsequently interacts with a child device (like twl4030-gpio via sysfs), the child driver calls accessors like twl_i2c_read() which in turn call twl_get_regmap().
In twl_get_regmap():
if (unlikely(!twl_priv || !twl_priv->ready)) {
...
}Because the global twl_priv pointer was never cleared, this check passes the first condition and attempts to access the freed memory to check the ready field, leading to a use-after-free.
Would it be safer to explicitly clear twl_priv during twl_remove(), or otherwise adjust the allocation lifecycle to avoid leaving a dangling global pointer?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: (subset) [PATCH] mfd: twl-core: unregister the auxiliary platform device on unbind
In reply to Myeonghun PakQuoted 11 lines · Myeonghun Pak
twl_probe()registers a standalone platform device named "twl", but keeps its pointer only in a local variable. Probe errors unregister the device, while successful probe leaves no way fortwl_remove()to release it. The platform device therefore remains registered after the I2C driver unbinds.Register a managed action on the I2C device immediately after adding the platform device. This unregisters it on probe failure and driver unbind, and also handles failure to register the action itself. Remove the manual error-path unregister to avoid releasing the device twice.
[...]
Applied, thanks!
[1/1] mfd: twl-core: unregister the auxiliary platform device on unbind
commit: 62d519c24f8373772d4fb390ba5926ebc8c17509
--
Lee Jones [李琼斯]