Re: [PATCH] hwmon: (nct6775-core) Prevent access to unsupported weight registers

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

Pre-existing issues:
- [High] The patch addresses out-of-bounds reads for PWM weight control registers but misses an identical vulnerability for `NCT6116_REG_TEMP_SOURCE`. This array has only 3 elements but is accessed up to index 5 during the `nct6775_probe` initialization, leading to an out-of-bounds read and arbitrary hardware register corruption.
--

--- Patch [1]: [PATCH] hwmon: (nct6775-core) Prevent access to unsupported weight registers ---
commit 38ce01c21386add5f97d86c3020857301fde2125
Author: Guenter Roeck <[email protected]>

hwmon: (nct6775-core) Prevent access to unsupported weight registers

During initialization of the nct6116 chip, the driver assigns several 3-element
NCT6106 register arrays to data pointers, but subsequently iterates up to 5
times based on data->pwm_num. This causes an out-of-bounds read, fetching
garbage values that are then used as hardware register addresses. This patch
fixes the issue by extending the affected arrays with zeroed fields.

> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index d668dc390def8..87fc2a21255a9 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c
> @@ -791,12 +791,12 @@ static const u16 NCT6106_REG_TOLERANCE_H[] = { 0x112, 0x122, 0x132 };
>  
>  static const u16 NCT6106_REG_TARGET[] = { 0x111, 0x121, 0x131 };
>  
> -static const u16 NCT6106_REG_WEIGHT_TEMP_SEL[] = { 0x168, 0x178, 0x188 };
> -static const u16 NCT6106_REG_WEIGHT_TEMP_STEP[] = { 0x169, 0x179, 0x189 };
> -static const u16 NCT6106_REG_WEIGHT_TEMP_STEP_TOL[] = { 0x16a, 0x17a, 0x18a };
> -static const u16 NCT6106_REG_WEIGHT_DUTY_STEP[] = { 0x16b, 0x17b, 0x18b };
> -static const u16 NCT6106_REG_WEIGHT_TEMP_BASE[] = { 0x16c, 0x17c, 0x18c };
> -static const u16 NCT6106_REG_WEIGHT_DUTY_BASE[] = { 0x16d, 0x17d, 0x18d };
> +static const u16 NCT6106_REG_WEIGHT_TEMP_SEL[] = { 0x168, 0x178, 0x188, 0, 0 };
> +static const u16 NCT6106_REG_WEIGHT_TEMP_STEP[] = { 0x169, 0x179, 0x189, 0, 0 };
> +static const u16 NCT6106_REG_WEIGHT_TEMP_STEP_TOL[] = { 0x16a, 0x17a, 0x18a, 0, 0 };
> +static const u16 NCT6106_REG_WEIGHT_DUTY_STEP[] = { 0x16b, 0x17b, 0x18b, 0, 0 };
> +static const u16 NCT6106_REG_WEIGHT_TEMP_BASE[] = { 0x16c, 0x17c, 0x18c, 0, 0 };
> +static const u16 NCT6106_REG_WEIGHT_DUTY_BASE[] = { 0x16d, 0x17d, 0x18d, 0, 0 };

[Severity: High]
This isn't a bug introduced by this patch, but does the same problem exist
for NCT6116_REG_TEMP_SOURCE?

It appears NCT6116_REG_TEMP_SOURCE is defined with only 3 elements:

drivers/hwmon/nct6775-core.c:
static const u16 NCT6116_REG_TEMP_SOURCE[] = {
	0xb0, 0xb1, 0xb2 };

During initialization, num_reg_temp is set to 6. Later in nct6775_probe(),
the code iterates unconditionally up to num_reg_temp:

drivers/hwmon/nct6775-core.c:nct6775_probe() {
    ...
    err = nct6775_read_value(data, data->REG_TEMP_SOURCE[i], &src);
    ...
}

Could this cause a similar out-of-bounds read and subsequent corruption
when add_temp_sensors() writes the temperature source mapping to those
unintended hardware register addresses?

>  
>  static const u16 NCT6106_REG_AUTO_TEMP[] = { 0x160, 0x170, 0x180 };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1