Re: [PATCH] hwmon: (nct6694) do not expose enable on DTIN temperature channels

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

Pre-existing issues:
- [High] The driver ignores the return value of `nct6694_read_msg()` in `nc=
t6694_temp_write()` during `hwmon_temp_max_hyst` writes, risking silent har=
dware state corruption.
- [High] Incorrect clamping of negative hysteresis offset due to unsigned t=
ype in `nct6694_temp_write()`.
- [High] The embedded structure `hwmon_en` is passed to USB functions that =
map it for DMA, risking cacheline sharing memory corruption.
--

--- Patch [1]: [PATCH] hwmon: (nct6694) do not expose enable on DTIN temper=
ature channels ---
commit ef55d968d34b204a77782df9efb5f40d8d167ebe
Author: Ali Ahmet Memis <[email protected]>
Subject: hwmon: (nct6694) do not expose enable on DTIN temperature channels

The driver registers 26 temperature channels, all advertising HWMON_T_ENABL=
E,
and indexes the enable bitmap with the raw channel. This commit removes the
enable configuration for DTIN channels to prevent out-of-bounds indexing th=
at
toggles the fan enable state instead.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c
> index 6dcf22ca5018a..9a9a4db434c4f 100644
> --- a/drivers/hwmon/nct6694-hwmon.c
> +++ b/drivers/hwmon/nct6694-hwmon.c
[ ... ]
> @@ -159,6 +159,9 @@ static inline s8 temp_to_reg(long val)
>  #define NCT6694_HWMON_TEMP_CONFIG (HWMON_T_INPUT | HWMON_T_ENABLE |	\
>  				   HWMON_T_MAX | HWMON_T_MAX_HYST |	\
>  				   HWMON_T_MAX_ALARM)
> +#define NCT6694_HWMON_DTIN_CONFIG (HWMON_T_INPUT |			\
> +				   HWMON_T_MAX | HWMON_T_MAX_HYST |	\
> +				   HWMON_T_MAX_ALARM)
>  #define NCT6694_HWMON_FAN_CONFIG (HWMON_F_INPUT | HWMON_F_ENABLE |	\
>  				  HWMON_F_MIN | HWMON_F_MIN_ALARM)
>  #define NCT6694_HWMON_PWM_CONFIG (HWMON_PWM_INPUT | HWMON_PWM_ENABLE |	\

While reviewing this patch, I noticed a few pre-existing issues in the same
file that might be worth addressing.

> drivers/hwmon/nct6694-hwmon.c:nct6694_temp_write()
> 	case hwmon_temp_max_hyst:
> 		cmd_hd =3D (struct nct6694_cmd_header) {
> 			.mod =3D NCT6694_HWMON_MOD,
> 			.cmd =3D NCT6694_HWMON_ALARM,
> 			.sel =3D NCT6694_HWMON_ALARM_SEL,
> 			.len =3D cpu_to_le16(sizeof(data->msg->hwmon_alarm))
> 		};
> 		ret =3D nct6694_read_msg(data->nct6694, &cmd_hd,
> 				       &data->msg->hwmon_alarm);
>=20
> 		val =3D clamp_val(val, -127000, 127000);

[Severity: High]
This isn't a bug introduced by this patch, but does this code ignore the
return value of nct6694_read_msg()?

If nct6694_read_msg() fails due to an error, such as a USB problem, it
appears the driver proceeds to modify uninitialized or stale data in
data->msg->hwmon_alarm and writes it back to the device, which could
corrupt the hardware state.

> 		temp_max =3D data->msg->hwmon_alarm.tin_cfg[channel].hl;
> 		temp_hyst =3D temp_max - temp_to_reg(val);
> 		temp_hyst =3D clamp_val(temp_hyst, 0, 7);

[Severity: High]
This is a pre-existing issue, but could a negative hysteresis offset cause
an underflow here?

If a user requests a hysteresis limit higher than the current max limit,
temp_max will be smaller than temp_to_reg(val) and the subtraction results
in a negative number. Since temp_hyst is declared as an unsigned char, this
negative value will underflow into a large positive number. The clamp_val()
call will then clamp it to the upper bound of 7 instead of the lower bound
of 0, configuring the hardware with the maximum possible hysteresis.

> drivers/hwmon/nct6694-hwmon.c:struct nct6694_hwmon_data
> 	struct nct6694 *nct6694;
> 	struct mutex lock;
> 	struct nct6694_hwmon_control hwmon_en;
> 	union nct6694_hwmon_rpt *rpt;
> 	union nct6694_hwmon_msg *msg;
[ ... ]
> drivers/hwmon/nct6694-hwmon.c:nct6694_hwmon_init()
> 	ret =3D nct6694_read_msg(data->nct6694, &cmd_hd,
> 			       &data->hwmon_en);

[Severity: High]
This is a pre-existing issue, but is it safe to pass the embedded
hwmon_en structure for USB messaging?

The hwmon_en struct is embedded directly after the mutex lock in
struct nct6694_hwmon_data, meaning they share the same cacheline.
On non-cache-coherent architectures, DMA cache maintenance operations
on this buffer could invalidate or flush the cacheline, potentially
clobbering concurrent CPU updates to the mutex lock.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803102148.1419=
[email protected]?part=3D1