[PATCH] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[PATCH] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[Hongyan Xu]

When a fan tach channel is present, npcm7xx_pwm_fan_probe() starts
fan_timer. The timer callback polls tach state and rearms the timer, but
the driver has no remove callback or devm cleanup action to stop it. On
device detach, the devm-managed driver data and I/O mappings can be
released while the timer is still pending or running.

Register a devm cleanup action before starting the timer and shut the
timer down synchronously from that action.

This issue was found by a static analysis tool.

Fixes: f1fd4a4db777 ("hwmon: Add NPCM7xx PWM and Fan driver")
Cc: stable@vger.kernel.org
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/hwmon/npcm750-pwm-fan.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
index aea0b8659f5..6503d28ed7b 100644
--- a/drivers/hwmon/npcm750-pwm-fan.c
+++ b/drivers/hwmon/npcm750-pwm-fan.c
@@ -358,6 +358,13 @@ static void npcm7xx_fan_polling(struct timer_list *t)
 	add_timer(&data->fan_timer);
 }
 
+static void npcm7xx_fan_cleanup(void *data)
+{
+	struct npcm7xx_pwm_fan_data *fan_data = data;
+
+	timer_shutdown_sync(&fan_data->fan_timer);
+}
+
 static inline void npcm7xx_fan_compute(struct npcm7xx_pwm_fan_data *data,
 				       u8 fan, u8 cmp, u8 fan_id, u8 flag_int,
 				       u8 flag_mode, u8 flag_clear)
@@ -1016,10 +1023,16 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
 	for (i = 0; i < NPCM7XX_FAN_MAX_CHN_NUM; i++) {
 		if (data->fan_present[i]) {
 			/* fan timer initialization */
-			data->fan_timer.expires = jiffies +
-				msecs_to_jiffies(NPCM7XX_FAN_POLL_TIMER_200MS);
 			timer_setup(&data->fan_timer,
 				    npcm7xx_fan_polling, 0);
+			ret = devm_add_action_or_reset(dev,
+						       npcm7xx_fan_cleanup,
+						       data);
+			if (ret)
+				return ret;
+
+			data->fan_timer.expires = jiffies +
+				msecs_to_jiffies(NPCM7XX_FAN_POLL_TIMER_200MS);
 			add_timer(&data->fan_timer);
 			break;
 		}
-- 
2.50.1.windows.1

Re: [PATCH] hwmon: (npcm750-pwm-fan): stop fan timer on device detach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Stack-allocated string used as interrupt name in `devm_request_irq()`, leading to a dangling pointer and use-after-free.
- [High] Unprotected read of fan_cnt causes a TOCTOU race leading to a divide-by-zero kernel panic.
- [High] Unprotected Read-Modify-Write (RMW) of PWM control registers can corrupt hardware state.
- [High] Missing bounds checks on Device Tree properties lead to out-of-bounds array writes on the heap.
- [High] The interrupt handler incorrectly calculates the fan module index using physical/virtual IRQ assumptions.
--

--- Patch [1]: [PATCH] hwmon: (npcm750-pwm-fan): stop fan timer on device detach ---
commit 2cacb36d0828241791acb61238fb537a2260d7d4
Author: Hongyan Xu <getshell@seu.edu.cn>

hwmon: (npcm750-pwm-fan): stop fan timer on device detach

When a fan tach channel is present, the probe function starts a polling timer.
This patch adds a devm cleanup action to ensure the timer is synchronously
stopped when the device is detached, preventing use-after-free issues if the
driver data is released while the timer is still running.

> diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
> index aea0b8659f5f6..6503d28ed7b04 100644
> --- a/drivers/hwmon/npcm750-pwm-fan.c
> +++ b/drivers/hwmon/npcm750-pwm-fan.c
[ ... ]
> @@ -1016,10 +1023,16 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  	for (i = 0; i < NPCM7XX_FAN_MAX_CHN_NUM; i++) {
>  		if (data->fan_present[i]) {
>  			/* fan timer initialization */
> -			data->fan_timer.expires = jiffies +
> -				msecs_to_jiffies(NPCM7XX_FAN_POLL_TIMER_200MS);
>  			timer_setup(&data->fan_timer,
>  				    npcm7xx_fan_polling, 0);
> +			ret = devm_add_action_or_reset(dev,
> +						       npcm7xx_fan_cleanup,
> +						       data);
> +			if (ret)
> +				return ret;
> +
> +			data->fan_timer.expires = jiffies +
> +				msecs_to_jiffies(NPCM7XX_FAN_POLL_TIMER_200MS);
>  			add_timer(&data->fan_timer);
>  			break;
>  		}

While reviewing this patch, I noticed a few pre-existing issues in this file:

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer in
the IRQ core?

In npcm7xx_pwm_fan_probe():

	char name[20];
	...
	for (i = 0; i < NPCM7XX_FAN_MAX_MODULE; i++) {
		...
		sprintf(name, "NPCM7XX-FAN-MD%d", i);
		ret = devm_request_irq(dev, data->fan_irq[i], npcm7xx_fan_isr,
				       0, name, (void *)data);

The local stack variable name is passed to devm_request_irq(), which stores
the pointer without copying it. When probe returns, the stack frame is
destroyed. Reading /proc/interrupts later would trigger a use-after-free.


[Severity: High]
This is a pre-existing issue, but could this cause a divide-by-zero panic?

In npcm7xx_read_fan():

	if (data->fan_dev[channel].fan_cnt > 0 && ...)
		*val = ((data->input_clk_freq * 60) /
			(data->fan_dev[channel].fan_cnt * ...));

The check and subsequent division are unprotected. Concurrently, if a hardware
timeout occurs, the ISR npcm7xx_fan_isr() -> npcm7xx_check_cmp() sets:

	data->fan_dev[fan_id].fan_cnt = 0;

Without READ_ONCE() or locking, the compiler can emit two memory loads,
allowing the value to change to 0 between the check and the division.


[Severity: High]
This is a pre-existing issue, but does this lockless Read-Modify-Write
corrupt hardware state?

In npcm7xx_pwm_config_set():

	tmp_buf = ioread32(NPCM7XX_PWM_REG_CR(data->pwm_base, module));
	...
	iowrite32(tmp_buf, NPCM7XX_PWM_REG_CR(data->pwm_base, module));

This modifies the shared per-module control register without any locking.
Since it can be called from sysfs and directly by the thermal subsystem
concurrently, updates to different channels on the same module can race and
overwrite each other's changes.


[Severity: High]
This is a pre-existing issue, but could these missing bounds checks lead to
out-of-bounds writes on the heap?

In npcm7xx_en_pwm_fan():

	ret = of_property_read_u32(child, "reg", &pwm_port);
	...
	data->pwm_present[pwm_port] = true;

And similarly for the fan indices:

	for (ch = 0; ch < fan_cnt; ch++) {
		index = fan_ch[ch];
		data->fan_present[index] = true;

Untrusted Device Tree properties (reg and fan-tach-ch) are used directly
to index into the data->pwm_present and data->fan_present arrays without any
bounds validation.


[Severity: High]
This is a pre-existing issue, but is the fan module index calculated
correctly here?

In npcm7xx_fan_isr():

	module = irq - data->fan_irq[0];
	spin_lock_irqsave(&data->fan_lock[module], flags);

Virtual IRQ numbers assigned by the kernel's irqdomain are dynamically
allocated and not guaranteed to be contiguous. If fan_irq[1] is not exactly
fan_irq[0] + 1, the calculated module index could exceed the bounds of the
data->fan_lock array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728075535.251-1-getshell@seu.edu.cn?part=1