Re: [PATCH] hwmon: (it87) Add support for IT8607E

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hwmon: (it87) NULL pointer dereference in it87_reg_pwm() due to missing reg_pwm
- [High] hwmon: (it87) broken PWM temp mapping for it8607 due to missing num_temp_map
--

commit fa2803179a2b666c8fd1b5211259f3072f2f691f
Author: Ismael Ojeda Perez <iojedaperez@proton.me>

hwmon: (it87) Add support for IT8607E

This commit adds hardware monitoring support for the ITE IT8607E Super I/O
chip found on Intel NUC6i3SYB and NUC6i5SYB boards, configuring its features
based on the IT8603E.

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 2757d428a23db..bdef0b78332b7 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c

[ ... ]

> @@ -530,6 +532,14 @@ static const struct it87_devices it87_devices[] = {
>  		.num_temp_map = 6,
>  		.peci_mask = 0x07,
>  	},
> +	[it8607] = {
> +		.name = "it8607",
> +		.model = "IT8607E",
> +		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
> +		  | FEAT_AVCC3 | FEAT_PWM_FREQ2,
> +		.peci_mask = 0x07,
> +	},

[Severity: High]
Does the it8607 entry in it87_devices[] need to initialize .reg_pwm (such as
.reg_pwm = IT87_REG_PWM, as in it8603)?

Without .reg_pwm initialized, chip->reg_pwm is NULL and it87_probe() copies
that NULL pointer into data->reg_pwm:

drivers/hwmon/it87.c:it87_probe() {
    ...
    data->reg_pwm = chip->reg_pwm;
    ...
}

When any PWM channel is active in data->has_pwm (such as pwm2), reading any
hwmon sysfs attribute or running it87_resume() calls:

  it87_update_device()
    -> it87_update_pwm_ctrl()
      -> it87_reg_pwm(data, nr)

where it87_reg_pwm() dereferences data->reg_pwm[nr]:

static inline u16 it87_reg_pwm(const struct it87_data *data, int nr)
{
	return data->reg_pwm[nr];
}

Writing to pwmX, pwmX_enable, or pwmX_auto_channels_temp via set_pwm(),
set_pwm_enable(), or set_pwm_temp_map() also calls it87_reg_pwm() and will
dereference NULL.

[Severity: High]
Should the it8607 entry in it87_devices[] also initialize .num_temp_map (for
example, .num_temp_map = 4 to match it8603)?

Leaving .num_temp_map unset initializes data->num_temp_map to 0 in
it87_probe(). Because 0 != IT87_PWM_OLD_NUM_TEMP (3), uses_global_temp_map()
returns true:

static inline bool uses_global_temp_map(const struct it87_data *data)
{
	return has_new_tempmap(data) ||
		pwm_num_temp_map(data) != IT87_PWM_OLD_NUM_TEMP;
}

In it87_update_pwm_ctrl() and pwm_temp_channel(), the comparison against
pwm_num_temp_map(data) (0) is always true and resets data->pwm_temp_map[nr]
to 0:

	if (uses_global_temp_map(data) &&
	    data->pwm_temp_map[nr] >= pwm_num_temp_map(data))
		data->pwm_temp_map[nr] = 0;

Similarly, in pwm_temp_map_from_channel(), channel >= num (where num is 0) is
always true for unsigned int channel, causing all writes to
pwmX_auto_channels_temp via set_pwm_temp_map() to fail with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179357707375.834227.595814448085536071@proton.me?part=1