[PATCH 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[PATCH 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[Guenter Roeck]

Sashiko reports:

In lm90_probe(), the devm action to cancel the alert_work and report_work
(lm90_restore_conf) is registered in lm90_init_client() before
devm_hwmon_device_register_with_info() is called.

Because devm executes cleanup actions in reverse order during module
unbind or probe failure, the hwmon device is unregistered and freed first.

If lm90_alert_work() or lm90_report_alarms() runs in the window between
the hwmon device being freed and the delayed works being cancelled,
lm90_update_alarms() will dereference the freed data->hwmon_dev here.

Fix the problem by canceling the workers separately after registering
the hwmon device and before registering the interrupt handler. This ensures
that the workers are canceled after interrupts are disabled and before
the hwmon device is released.

Fixes: f6d0775119fb9 ("hwmon: (lm90) Rework alarm/status handling")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 3c10a5066b53..50b30d719225 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -2584,15 +2584,20 @@ static void lm90_restore_conf(void *_data)
 	struct lm90_data *data = _data;
 	struct i2c_client *client = data->client;
 
-	cancel_delayed_work_sync(&data->alert_work);
-	cancel_work_sync(&data->report_work);
-
 	/* Restore initial configuration */
 	if (data->flags & LM90_HAVE_CONVRATE)
 		lm90_write_convrate(data, data->convrate_orig);
 	lm90_write_reg(client, LM90_REG_CONFIG1, data->config_orig);
 }
 
+static void lm90_stop_work(void *_data)
+{
+	struct lm90_data *data = _data;
+
+	cancel_delayed_work_sync(&data->alert_work);
+	cancel_work_sync(&data->report_work);
+}
+
 static int lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 {
 	struct device_node *np = client->dev.of_node;
@@ -2902,6 +2907,10 @@ static int lm90_probe(struct i2c_client *client)
 
 	data->hwmon_dev = hwmon_dev;
 
+	err = devm_add_action_or_reset(&client->dev, lm90_stop_work, data);
+	if (err)
+		return err;
+
 	if (client->irq) {
 		dev_dbg(dev, "IRQ: %d\n", client->irq);
 		err = devm_request_threaded_irq(dev, client->irq,
-- 
2.45.2

[PATCH 2/2] hwmon: (lm90) Add lock protection to lm90_alert

[Guenter Roeck]

Sashiko reports:

lm90_alert() executes in the smbus alert context and calls
lm90_update_confreg() to disable the hardware alert line, without
acquiring hwmon_lock.

Concurrently, sysfs write operations (such as lm90_write_convrate) hold
the hwmon_lock, temporarily modify data->config, and then restore it.

If an alert interrupt occurs concurrently with a sysfs write, the sysfs
path will overwrite the alert handler's modifications to data->config
and the hardware register.

This unintentionally re-enables the hardware alert line while the alarm is
still active, causing an interrupt storm.

Add the missing lock to lm90_alert() to solve the problem.

Fixes: 7a1d220ccb0cc ("hwmon: (lm90) Introduce function to update configuration register")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm90.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 50b30d719225..6658c743659b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -2939,6 +2939,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
 		 */
 		struct lm90_data *data = i2c_get_clientdata(client);
 
+		hwmon_lock(data->hwmon_dev);
 		if ((data->flags & LM90_HAVE_BROKEN_ALERT) &&
 		    (data->current_alarms & data->alert_alarms)) {
 			if (!(data->config & 0x80)) {
@@ -2948,6 +2949,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
 			schedule_delayed_work(&data->alert_work,
 				max_t(int, HZ, msecs_to_jiffies(data->update_interval)));
 		}
+		hwmon_unlock(data->hwmon_dev);
 	} else {
 		dev_dbg(&client->dev, "Everything OK\n");
 	}
-- 
2.45.2

Re: [PATCH 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Re-ordering work cancellation before hwmon device unregistration introduces a Use-After-Free vulnerability due to a race with sysfs accesses.
--

commit dfb6baf4fb10f085fc08795b014c46ab84d8b479
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (lm90) Stop work before releasing hwmon device
    
This commit addresses a use-after-free issue by moving the cancellation
of alert_work and report_work to a separate devm action. It registers
this new action after the hwmon device is registered, ensuring the works
are cancelled before the hwmon device is freed.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 3c10a5066b53d..50b30d7192256 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c

[ ... ]

> @@ -2902,6 +2907,10 @@ static int lm90_probe(struct i2c_client *client)
>  
>  	data->hwmon_dev = hwmon_dev;
>  
> +	err = devm_add_action_or_reset(&client->dev, lm90_stop_work, data);
> +	if (err)
> +		return err;
> +

Does this reordering introduce a regression where the workqueue can be
re-armed after it is cancelled?

Because this devm action is registered after
devm_hwmon_device_register_with_info(), lm90_stop_work() will be called first
during teardown, cancelling alert_work and report_work.

However, at this point the hwmon device is still registered and its sysfs
attributes remain exposed.

If user-space reads a sysfs attribute in this window, it triggers the
hwmon core to invoke lm90_temp_read(), which calls lm90_update_device()
and then lm90_update_alarms_locked().

If alerts are disabled on the chip, lm90_update_alarms_locked() will call
schedule_work(&data->report_work) or schedule_delayed_work(&data->alert_work).

Could this dynamically re-arm the workqueue after lm90_stop_work() has
already run?

If the work is re-armed, the teardown process will then unregister the
hwmon device and free the data structure. The asynchronously executing work
would then dereference the freed data pointer, resulting in a use-after-free.

Would it be possible to introduce synchronization state, such as checking a
flag in the data structure before scheduling works, or decoupling the
teardown order?

>  	if (client->irq) {
>  		dev_dbg(dev, "IRQ: %d\n", client->irq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514220214.1380831-1-linux@roeck-us.net?part=1

Re: [PATCH 2/2] hwmon: (lm90) Add lock protection to lm90_alert

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Use-After-Free due to incorrect devm teardown sequence
--

commit 8b53a979a14111ccbfe05e43369fd3214a55c211
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (lm90) Add lock protection to lm90_alert

This commit adds hwmon_lock protection to lm90_alert to prevent concurrent
sysfs write operations from overwriting the alert handler's modifications.
This solves an interrupt storm problem when an alert interrupt occurs
concurrently with a sysfs write.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 50b30d7192256..6658c743659b8 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -2939,6 +2939,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
>  		 */
>  		struct lm90_data *data = i2c_get_clientdata(client);
>  
> +		hwmon_lock(data->hwmon_dev);
>  		if ((data->flags & LM90_HAVE_BROKEN_ALERT) &&
>  		    (data->current_alarms & data->alert_alarms)) {
>  			if (!(data->config & 0x80)) {
> @@ -2948,6 +2949,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
>  			schedule_delayed_work(&data->alert_work,
>  				max_t(int, HZ, msecs_to_jiffies(data->update_interval)));
>  		}

This isn't a regression introduced by this patch, but it exists in the
driver's device teardown path. Can this workqueue re-arming lead to a
use-after-free of the data structure during device unbind?

In lm90_probe(), devm_hwmon_device_register_with_info() is called before
adding lm90_stop_work():

drivers/hwmon/lm90.c:lm90_probe() {
    ...
	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
							 data, &data->chip,
							 NULL);
    ...
	err = devm_add_action_or_reset(&client->dev, lm90_stop_work, data);
    ...
}

Because devres executes actions in reverse order of addition,
lm90_stop_work() runs first during device unbind or probe failure. This
synchronously cancels both alert_work and report_work:

drivers/hwmon/lm90.c:lm90_stop_work() {
    ...
	cancel_delayed_work_sync(&data->alert_work);
	cancel_work_sync(&data->report_work);
}

However, at this point the hwmon device is still registered, meaning its
sysfs attributes are fully accessible to userspace.

If a concurrent sysfs read occurs to a file like temp1_input, it will
invoke lm90_temp_read(), which calls lm90_update_device() and subsequently
lm90_update_alarms_locked().

If alarm conditions are still met, this path explicitly re-arms the
workqueues by calling schedule_delayed_work(&data->alert_work, ...) or
schedule_work(&data->report_work).

After the sysfs read finishes, devres proceeds to unregister the hwmon
device and free the lm90_data structure. The incorrectly re-armed work
will eventually execute in the background using the freed data pointer.

> +		hwmon_unlock(data->hwmon_dev);
>  	} else {
>  		dev_dbg(&client->dev, "Everything OK\n");
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514220214.1380831-1-linux@roeck-us.net?part=2