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

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

[Ali Ahmet Memis]

The driver registers 26 temperature channels, all advertising
HWMON_T_ENABLE, and indexes the enable bitmap with the raw channel:

	data->hwmon_en.tin_en[channel / 8] |= BIT(channel % 8);

tin_en is two bytes and only covers the 5 THR and 5 TDP channels
(index 0-9). The 16 DTIN channels (index 10-25) are enabled by the
firmware and were never meant to carry an enable bit. Because the
control structure is packed, writing temp17_enable and above indexes
past tin_en into the fin_en bytes that follow it, so it toggles fan
enable state instead; nct6694_hwmon_init() then sends the whole
structure back to the device, and reads report fan state as temperature
state. It stays within the structure, so this is not a memory safety
problem, but on a board that uses the fan channels it is not harmless.

Give the DTIN channels a temperature config without HWMON_T_ENABLE so
the core never creates their enable attribute. The enable path is then
reachable only for the first 10 channels, which stay within tin_en, and
fin_en is left alone. The DTIN input and limit attributes are unchanged.

Fixes: 197e779d29d8 ("hwmon: Add Nuvoton NCT6694 HWMON support")
Suggested-by: Ming Yu <tmyu0@nuvoton.com>
Link: https://lore.kernel.org/all/20260802124730.20387-1-ali@iusegentoo.com/
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 drivers/hwmon/nct6694-hwmon.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c
index 6dcf22ca5018..9a9a4db434c4 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 |	\
@@ -193,22 +196,22 @@ static const struct hwmon_channel_info *nct6694_info[] = {
 			   NCT6694_HWMON_TEMP_CONFIG,	/* TDP2 */
 			   NCT6694_HWMON_TEMP_CONFIG,	/* TDP3 */
 			   NCT6694_HWMON_TEMP_CONFIG,	/* TDP4 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN0 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN1 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN2 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN3 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN4 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN5 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN6 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN7 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN8 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN9 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN10 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN11 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN12 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN13 */
-			   NCT6694_HWMON_TEMP_CONFIG,	/* DTIN14 */
-			   NCT6694_HWMON_TEMP_CONFIG),	/* DTIN15 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN0 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN1 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN2 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN3 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN4 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN5 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN6 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN7 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN8 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN9 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN10 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN11 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN12 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN13 */
+			   NCT6694_HWMON_DTIN_CONFIG,	/* DTIN14 */
+			   NCT6694_HWMON_DTIN_CONFIG),	/* DTIN15 */
 
 	HWMON_CHANNEL_INFO(fan,
 			   NCT6694_HWMON_FAN_CONFIG,	/* FIN0 */
-- 
2.55.0

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

[sashiko-bot]

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 `nct6694_temp_write()` during `hwmon_temp_max_hyst` writes, risking silent hardware state corruption.
- [High] Incorrect clamping of negative hysteresis offset due to unsigned type 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 temperature channels ---
commit ef55d968d34b204a77782df9efb5f40d8d167ebe
Author: Ali Ahmet Memis <ali@iusegentoo.com>
Subject: hwmon: (nct6694) do not expose enable on DTIN temperature channels

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

Link: https://lore.kernel.org/all/20260802124730.20387-1-ali@iusegentoo.com/

> 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 = (struct nct6694_cmd_header) {
> 			.mod = NCT6694_HWMON_MOD,
> 			.cmd = NCT6694_HWMON_ALARM,
> 			.sel = NCT6694_HWMON_ALARM_SEL,
> 			.len = cpu_to_le16(sizeof(data->msg->hwmon_alarm))
> 		};
> 		ret = nct6694_read_msg(data->nct6694, &cmd_hd,
> 				       &data->msg->hwmon_alarm);
> 
> 		val = 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 = data->msg->hwmon_alarm.tin_cfg[channel].hl;
> 		temp_hyst = temp_max - temp_to_reg(val);
> 		temp_hyst = 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 = 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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803102148.14196-1-ali@iusegentoo.com?part=1