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

On 8/13/26 18:19, [email protected] 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 <[email protected]>
> 
> 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