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

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 <[email protected]>

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/[email protected]?part=1