Re: [PATCH v2 4/7] hwmon: (npcm750-pwm-fan) Remove redundant dev_err()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The local stack array `name` is passed as the `devname` parameter to `devm_request_irq()`, resulting in a dangling pointer when the probe function returns.
- [High] The driver registers a repeating timer `data->fan_timer` but fails to cancel it when the device is unbound, leading to a use-after-free.
- [High] Missing bounds checking for Device Tree properties `reg` and `fan-tach-ch` leads to potential out-of-bounds memory accesses.
- [High] A data race exists when reading `fan_cnt` in sysfs, potentially leading to a division by zero.
- [High] The interrupt handler `npcm7xx_fan_isr` incorrectly assumes Linux virtual IRQ numbers are strictly contiguous, leading to out-of-bounds array accesses.
--

--- Patch [4]: [PATCH v2 4/7] hwmon: (npcm750-pwm-fan) Remove redundant dev_err() ---
commit ad51633ccf67c748dcb209308176241fe4d2d53b
Author: Pan Chuang <panchuang@vivo.com>

hwmon: (npcm750-pwm-fan) Remove redundant dev_err()
    
Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs detailed
error messages on failure. Remove the now-redundant driver-specific
dev_err() call.

> diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
> index aea0b8659f5f6..df24dec78e770 100644
> --- a/drivers/hwmon/npcm750-pwm-fan.c
> +++ b/drivers/hwmon/npcm750-pwm-fan.c
> @@ -991,10 +991,8 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  		sprintf(name, "NPCM7XX-FAN-MD%d", i);
>  		ret = devm_request_irq(dev, data->fan_irq[i], npcm7xx_fan_isr,
>  				       0, name, (void *)data);

[Severity: High]
This is a pre-existing issue, but is it safe to use a local stack array for
the devname parameter?

devm_request_irq() stores the provided devname pointer directly in the
irqaction struct without copying the string.

Because name is a locally scoped stack array in npcm7xx_pwm_fan_probe(),
it is deallocated when the probe function returns. Subsequent reads of
/proc/interrupts could dereference this dangling pointer. Additionally,
since the same array is reused in the loop, all IRQs would share the
exact same dangling pointer.

> -		if (ret) {
> -			dev_err(dev, "register IRQ fan%d failed\n", i);
> +		if (ret)
>  			return ret;
> -		}
>  	}

While reviewing this code, I noticed a few other pre-existing issues in
the driver.

drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_pwm_fan_probe() {
    ...
		timer_setup(&data->fan_timer, npcm7xx_fan_polling, 0);
		add_timer(&data->fan_timer);
    ...
}

drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_pwm_fan_driver {
    static struct platform_driver npcm7xx_pwm_fan_driver = {
        .probe		= npcm7xx_pwm_fan_probe,
        .driver		= { ...
}

[Severity: High]
This isn't a bug introduced by this patch, but could data->fan_timer expire
after the driver is unbound?

The driver registers a repeating timer but fails to cancel it when the device
is unbound. The platform driver struct lacks a .remove callback, and no devm
cleanup action is registered to call del_timer_sync(). If the driver is
unbound or the module is unloaded, the devres framework will free data,
leading to a use-after-free when the timer subsequently expires.


drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_en_pwm_fan() {
    ...
	ret = of_property_read_u32(child, "reg", &pwm_port);
	if (ret)
		return ret;

	data->pwm_present[pwm_port] = true;
    ...
	for (ch = 0; ch < fan_cnt; ch++) {
		index = fan_ch[ch];
		data->fan_present[index] = true;
		data->fan_dev[index].fan_st_flg = FAN_INIT;
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but is it possible for a malformed device tree
to trigger an out-of-bounds memory write to data->pwm_present or data->fan_dev?

The values pwm_port and index are derived directly from the device tree and
used as indices for the fixed-size arrays data->pwm_present, data->fan_present,
and data->fan_dev. If a device tree provides values exceeding the array limits,
the driver will write out of bounds.


drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_read_fan() {
    ...
		if (data->fan_dev[channel].fan_cnt > 0 &&
		    data->fan_dev[channel].fan_pls_per_rev > 0)
			*val = ((data->input_clk_freq * 60) /
				(data->fan_dev[channel].fan_cnt *
				 data->fan_dev[channel].fan_pls_per_rev));
    ...
}

drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_check_cmp() {
    ...
		data->fan_dev[fan_id].fan_cnt = 0;
    ...
}

[Severity: High]
This is a pre-existing issue, but could concurrent execution lead to a
division by zero panic on data->fan_dev[channel].fan_cnt?

In npcm7xx_read_fan(), the shared variable fan_cnt is evaluated and then used
as a denominator. If the interrupt handler updates fan_cnt to 0 after the
greater-than-zero check but before the division, this would trigger a kernel
panic.


drivers/hwmon/npcm750-pwm-fan.c:npcm7xx_fan_isr() {
    ...
	module = irq - data->fan_irq[0];
	spin_lock_irqsave(&data->fan_lock[module], flags);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation
incorrectly assume that the irq virqs are contiguous?

Linux dynamically allocates virtual IRQ numbers, providing no guarantee that
they are allocated contiguously. If the virqs are sparse or out of order, the
calculated module index could be negative or exceed the bounds of the
fan_lock array, resulting in out-of-bounds memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720063745.309331-1-panchuang@vivo.com?part=4