[PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array

[PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array

[Mark Sercombe]

w83627hf_update_device() reads PWM registers in a loop bounded by a
hard coded "i <= 2", i.e. three iterations, for every chip type. For
the W83627HF, W836X7HF_REG_PWM() indexes regpwm_627hf[], which only has
two entries. The third iteration therefore reads regpwm_627hf[2], one
element past the end of the array, and issues a read of a non existent
PWM register.

The W83627HF has only two PWM outputs (datasheet registers CR5A and
CR5B, corresponding to W83627HF_REG_PWM1/PWM2) it has no third PWM
register, and the driver correctly does not expose pwm3 for this chip.
The function already computes num_pwms for this purpose, but the
loop did not use it, and num_pwms itself did not account for the
W83627HF having two PWMs.

Include the W83627HF in the two PWM case and bound the loop by num_pwms
so each chip only reads the PWM registers it actually has.

Found by smatch. Compile tested only, I do not have the hardware.

Signed-off-by: Mark Sercombe <sercombe.joel.mark@gmail.com>
---
 drivers/hwmon/w83627hf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/w83627hf.c b/drivers/hwmon/w83627hf.c
index bb993bb09f40..e2cb7e8aeae3 100644
--- a/drivers/hwmon/w83627hf.c
+++ b/drivers/hwmon/w83627hf.c
@@ -478,7 +478,7 @@ static struct w83627hf_data *w83627hf_update_device(struct device *dev)
 {
 	struct w83627hf_data *data = dev_get_drvdata(dev);
 	int i, num_temps = (data->type == w83697hf) ? 2 : 3;
-	int num_pwms = (data->type == w83697hf) ? 2 : 3;
+	int num_pwms = (data->type == w83627hf || data->type == w83697hf) ? 2 : 3;
 
 	mutex_lock(&data->update_lock);
 
@@ -506,7 +506,7 @@ static struct w83627hf_data *w83627hf_update_device(struct device *dev)
 			    w83627hf_read_value(data,
 					       W83627HF_REG_FAN_MIN(i));
 		}
-		for (i = 0; i <= 2; i++) {
+		for (i = 0; i < num_pwms; i++) {
 			u8 tmp = w83627hf_read_value(data,
 				W836X7HF_REG_PWM(data->type, i));
 			/* bits 0-3 are reserved  in 627THF */
-- 
2.55.0

Re: [PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message inaccurately claims to fix a runtime out-of-bounds array access and hardware register read, but this vulnerability is a static analyzer false positive and did not actually exist at runtime.
--

--- Patch [1]: [PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array ---
commit 541d179df68e1d4e07bc936b67920fc0623c9326
Author: Mark Sercombe <sercombe.joel.mark@gmail.com>

hwmon: (w83627hf) fix out of bounds read of PWM register array

This commit replaces a hardcoded loop bound with num_pwms in
w83627hf_update_device() to prevent an out-of-bounds array access reported
by a static analyzer. It also updates num_pwms to correctly account for the
W83627HF having two PWMs.

> w83627hf_update_device() reads PWM registers in a loop bounded by a
> hard coded "i <= 2", i.e. three iterations, for every chip type. For
> the W83627HF, W836X7HF_REG_PWM() indexes regpwm_627hf[], which only has
> two entries. The third iteration therefore reads regpwm_627hf[2], one
> element past the end of the array, and issues a read of a non existent
> PWM register.

[Severity: Medium]
Does the loop actually ever reach the third iteration for W83627HF at runtime?

Looking at the loop body in w83627hf_update_device():

drivers/hwmon/w83627hf.c:w83627hf_update_device() {
...
		for (i = 0; i <= 2; i++) {
			u8 tmp = w83627hf_read_value(data,
				W836X7HF_REG_PWM(data->type, i));
			/* bits 0-3 are reserved  in 627THF */
			if (data->type == w83627thf)
				tmp &= 0xf0;
			data->pwm[i] = tmp;
			if (i == 1 &&
			    (data->type == w83627hf || data->type == w83697hf))
				break;
		}
...
}

It appears there is already a conditional break that stops the loop when i is 1,
meaning the loop never proceeds to a third iteration where i would be 2.

Could the commit message be more precise that this is addressing a static
analyzer false positive rather than an actual runtime bug?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813211950.535148-1-sercombe.joel.mark@gmail.com?part=1

Re: [PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array

[Guenter Roeck]

On 8/13/26 18:19, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message inaccurately claims to fix a runtime out-of-bounds array access and hardware register read, but this vulnerability is a static analyzer false positive and did not actually exist at runtime.
> --
> 
> --- Patch [1]: [PATCH] hwmon: (w83627hf) fix out of bounds read of PWM register array ---
> commit 541d179df68e1d4e07bc936b67920fc0623c9326
> Author: Mark Sercombe <sercombe.joel.mark@gmail.com>
> 
> hwmon: (w83627hf) fix out of bounds read of PWM register array
> 
> This commit replaces a hardcoded loop bound with num_pwms in
> w83627hf_update_device() to prevent an out-of-bounds array access reported
> by a static analyzer. It also updates num_pwms to correctly account for the
> W83627HF having two PWMs.
> 
>> w83627hf_update_device() reads PWM registers in a loop bounded by a
>> hard coded "i <= 2", i.e. three iterations, for every chip type. For
>> the W83627HF, W836X7HF_REG_PWM() indexes regpwm_627hf[], which only has
>> two entries. The third iteration therefore reads regpwm_627hf[2], one
>> element past the end of the array, and issues a read of a non existent
>> PWM register.
> 
> [Severity: Medium]
> Does the loop actually ever reach the third iteration for W83627HF at runtime?
> 
> Looking at the loop body in w83627hf_update_device():
> 
> drivers/hwmon/w83627hf.c:w83627hf_update_device() {
> ...
> 		for (i = 0; i <= 2; i++) {
> 			u8 tmp = w83627hf_read_value(data,
> 				W836X7HF_REG_PWM(data->type, i));
> 			/* bits 0-3 are reserved  in 627THF */
> 			if (data->type == w83627thf)
> 				tmp &= 0xf0;
> 			data->pwm[i] = tmp;
> 			if (i == 1 &&
> 			    (data->type == w83627hf || data->type == w83697hf))
> 				break;
> 		}
> ...
> }
> 
> It appears there is already a conditional break that stops the loop when i is 1,
> meaning the loop never proceeds to a third iteration where i would be 2.
> 
> Could the commit message be more precise that this is addressing a static
> analyzer false positive rather than an actual runtime bug?
> 

I'd rather not touch the driver in the first place. A few lines further down
is similar code. We'd end up with no end of cosmetic non-functional patches
if we start to "fix" those. Sashiko finds enough real bugs. Let's fix those
instead of fixing non-bugs.

Guenter