[PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[Gui-Dong Han]

The macro FAN_FROM_REG evaluates its argument multiple times. When used
in lockless code accessing shared driver data, this can cause a
Time-of-Check to Time-of-Use (TOCTOU) race and potentially a
divide-by-zero error.

Convert the macro to a static function so that the register value is
evaluated only once.

Check the other conversion macros in the driver as well. Keep them
unchanged because they either do not evaluate arguments multiple times
or are only used from locked code paths.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
While learning the hwmon driver code, I found a few more potential
TOCTOU problems in drivers still using the older non-_with_info() APIs.
Fix them.
---
 drivers/hwmon/lm63.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 035176a98ce9..da132b267c58 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -109,8 +109,14 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
  * adapted accordingly.
  */
 
-#define FAN_FROM_REG(reg)	((reg) == 0xFFFC || (reg) == 0 ? 0 : \
-				 5400000 / (reg))
+static int fan_from_reg(int reg)
+{
+	if (reg == 0xFFFC || reg == 0)
+		return 0;
+
+	return 5400000 / reg;
+}
+
 #define FAN_TO_REG(val)		((val) <= 82 ? 0xFFFC : \
 				 (5400000 / (val)) & 0xFFFC)
 #define TEMP8_FROM_REG(reg)	((reg) * 1000)
@@ -333,7 +339,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct lm63_data *data = lm63_update_device(dev);
-	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index]));
+	return sprintf(buf, "%d\n", fan_from_reg(data->fan[attr->index]));
 }
 
 static ssize_t set_fan(struct device *dev, struct device_attribute *dummy,
-- 
2.43.0

[PATCH 2/2] hwmon: (lm63) Add locking to avoid TOCTOU

[Gui-Dong Han]

The functions show_pwm1() and show_temp11() access shared cached data
multiple times without holding the update lock. This can cause TOCTOU
races if the cached values change between the checks and the later
calculations.

Those cached values are updated in lm63_update_device(). For example,
show_pwm1() may read data->pwm1[nr] and data->pwm1_freq while a
concurrent lm63_update_device() refresh is updating them, resulting in a
mixed old/new snapshot.

Hold the update lock across the whole read and calculation sequence so
that the values remain stable.

Check the other functions in the driver as well. Keep them unchanged
because they either do not access shared cached values multiple times
or already do so under lock.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Fixes: e872c91e726e ("hwmon: (lm63) Add support for unsigned upper temperature limits")
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
While learning the hwmon driver code, I found a few more potential
TOCTOU problems in drivers still using the older non-_with_info() APIs.
Fix them.
---
 drivers/hwmon/lm63.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index da132b267c58..25b871695cb3 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -372,12 +372,14 @@ static ssize_t show_pwm1(struct device *dev, struct device_attribute *devattr,
 	int nr = attr->index;
 	int pwm;
 
+	mutex_lock(&data->update_lock);
 	if (data->pwm_highres)
 		pwm = data->pwm1[nr];
 	else
 		pwm = data->pwm1[nr] >= 2 * data->pwm1_freq ?
 		       255 : (data->pwm1[nr] * 255 + data->pwm1_freq) /
 		       (2 * data->pwm1_freq);
+	mutex_unlock(&data->update_lock);
 
 	return sprintf(buf, "%d\n", pwm);
 }
@@ -535,6 +537,7 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
 	int nr = attr->index;
 	int temp;
 
+	mutex_lock(&data->update_lock);
 	if (!nr) {
 		/*
 		 * Use unsigned temperature unless its value is zero.
@@ -550,7 +553,10 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
 		else
 			temp = TEMP11_FROM_REG(data->temp11[nr]);
 	}
-	return sprintf(buf, "%d\n", temp + data->temp2_offset);
+	temp += data->temp2_offset;
+	mutex_unlock(&data->update_lock);
+
+	return sprintf(buf, "%d\n", temp);
 }
 
 static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
-- 
2.43.0

Re: [PATCH 2/2] hwmon: (lm63) Add locking to avoid TOCTOU

[sashiko-bot]

Sashiko AI review found 3 potential issue(s):
- [High] Pre-existing Torn Read Race Condition in show_fan() causes massive fan speed spikes
- [High] Pre-existing Uninitialized Cache Read in temp2_crit_hyst_store() corrupts hardware thermal limits
- [Medium] Incomplete Fix: Unaddressed TOCTOU Races in temp2_crit_hyst_show() and show_lut_temp_hyst()
--

commit 1233dd514546736dfbbf09561bf360360c5933ca
Author: Gui-Dong Han <hanguidong02@gmail.com>

hwmon: (lm63) Add locking to avoid TOCTOU

This commit adds locking to show_pwm1() and show_temp11() to prevent
TOCTOU races when reading multiple cached values. It also states that
other functions in the driver were checked for similar issues.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/

> Check the other functions in the driver as well. Keep them unchanged
> because they either do not access shared cached values multiple times
> or already do so under lock.

Is this description accurate?

Looking at temp2_crit_hyst_show(), it calculates its value by reading
both data->temp8[2] and data->temp2_crit_hyst. Similarly,
show_lut_temp_hyst() reads data->temp8[attr->index] and
data->lut_temp_hyst. Because neither function holds data->update_lock,
can a concurrent cache update cause them to fetch a mixed snapshot (e.g., a
newly updated base temperature but a stale hysteresis offset), resulting in
corrupted absolute hysteresis values returned to userspace?

Also, looking at show_fan(), it accesses data->fan[attr->index] without
holding data->update_lock. In lm63_update_device(), data->fan[0] is
populated via two separate non-atomic assignments. Can show_fan() read the
intermediate torn state where the MSB is still 0, causing the calculation
(5400000 / reg) to report massive, incorrect fan speed spikes?

Finally, in temp2_crit_hyst_store(), data is retrieved via
dev_get_drvdata(dev) without calling lm63_update_device() to populate the
cache. If a user writes to the sysfs node before any sensor read operation
has occurred, data->temp8[2] will be 0. Does this cause the driver to
calculate the relative hysteresis based on a critical temperature of 0
degrees instead of the real thermal limit, corrupting the chip's thermal
protection configuration when written to the hardware register?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416090752.97392-1-hanguidong02@gmail.com?part=2

Re: [PATCH 2/2] hwmon: (lm63) Add locking to avoid TOCTOU

[Gui-Dong Han]

On Thu, Apr 16, 2026 at 7:44 PM <sashiko-bot@kernel.org> wrote:
>
> Sashiko AI review found 3 potential issue(s):
> - [High] Pre-existing Torn Read Race Condition in show_fan() causes massive fan speed spikes
> - [High] Pre-existing Uninitialized Cache Read in temp2_crit_hyst_store() corrupts hardware thermal limits
> - [Medium] Incomplete Fix: Unaddressed TOCTOU Races in temp2_crit_hyst_show() and show_lut_temp_hyst()

Thanks for the detailed review.

I will add the missing locks for show_fan() and the hysteresis show
functions in v2.

The potential uninitialized cache read in temp2_crit_hyst_store()
needs further investigation. If it is a bug, I will address it in a
separate patch, as it is a pre-existing issue unrelated to this
series.

> --
>
> commit 1233dd514546736dfbbf09561bf360360c5933ca
> Author: Gui-Dong Han <hanguidong02@gmail.com>
>
> hwmon: (lm63) Add locking to avoid TOCTOU
>
> This commit adds locking to show_pwm1() and show_temp11() to prevent
> TOCTOU races when reading multiple cached values. It also states that
> other functions in the driver were checked for similar issues.
>
> Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
>
> > Check the other functions in the driver as well. Keep them unchanged
> > because they either do not access shared cached values multiple times
> > or already do so under lock.
>
> Is this description accurate?
>
> Looking at temp2_crit_hyst_show(), it calculates its value by reading
> both data->temp8[2] and data->temp2_crit_hyst. Similarly,
> show_lut_temp_hyst() reads data->temp8[attr->index] and
> data->lut_temp_hyst. Because neither function holds data->update_lock,
> can a concurrent cache update cause them to fetch a mixed snapshot (e.g., a
> newly updated base temperature but a stale hysteresis offset), resulting in
> corrupted absolute hysteresis values returned to userspace?
>
> Also, looking at show_fan(), it accesses data->fan[attr->index] without
> holding data->update_lock. In lm63_update_device(), data->fan[0] is
> populated via two separate non-atomic assignments. Can show_fan() read the
> intermediate torn state where the MSB is still 0, causing the calculation
> (5400000 / reg) to report massive, incorrect fan speed spikes?
>
> Finally, in temp2_crit_hyst_store(), data is retrieved via
> dev_get_drvdata(dev) without calling lm63_update_device() to populate the
> cache. If a user writes to the sysfs node before any sensor read operation
> has occurred, data->temp8[2] will be 0. Does this cause the driver to
> calculate the relative hysteresis based on a critical temperature of 0
> degrees instead of the real thermal limit, corrupting the chip's thermal
> protection configuration when written to the hardware register?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260416090752.97392-1-hanguidong02@gmail.com?part=2

Re: [PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[sashiko-bot]

Sashiko AI review found 2 potential issue(s):
- [High] Incomplete TOCTOU fix allows compiler-induced divide-by-zero crash in `show_fan()`.
- [Medium] Lockless sysfs read in `show_fan()` causes torn reads and violates hwmon subsystem locking guidelines.
--

commit ebc10731137639981842ea19f80f0b969db71842
Author: Gui-Dong Han <hanguidong02@gmail.com>

hwmon: (lm63) Convert macro to function to avoid TOCTOU

The patch converts the FAN_FROM_REG macro to a static function
fan_from_reg() to evaluate its argument only once, avoiding a TOCTOU
race and potential divide-by-zero error.

Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/

> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 035176a98ce9c..da132b267c584 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -109,8 +109,14 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>   * adapted accordingly.
>   */
>  
> -#define FAN_FROM_REG(reg)	((reg) == 0xFFFC || (reg) == 0 ? 0 : \
> -				 5400000 / (reg))
> +static int fan_from_reg(int reg)
> +{
> +	if (reg == 0xFFFC || reg == 0)
> +		return 0;
> +
> +	return 5400000 / reg;
> +}
> +

[ ... ]

> @@ -333,7 +339,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct lm63_data *data = lm63_update_device(dev);
> -	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index]));
> +	return sprintf(buf, "%d\n", fan_from_reg(data->fan[attr->index]));

Since fan_from_reg() is a small static function, won't the compiler likely
inline it?

If it gets inlined, couldn't the compiler optimize the code by reloading
data->fan[attr->index] from memory multiple times (once for the 0 check, and
again for the division)?

If lm63_update_device() concurrently writes to data->fan[0] in two non-atomic
steps (where the first step can momentarily set the value to 0), and a reload
happens exactly when the value is 0, could this still cause a divide-by-zero
panic?

Note that the subsequent patch in the series ('hwmon: (lm63) Add locking
to avoid TOCTOU') explicitly skips show_fan(), leaving this unfixed.

Additionally, is it safe to read data->fan locklessly here?

According to the hwmon subsystem guidelines, drivers must implement locking
for attributes registered outside the core-serialized info API. Because this
driver registers its attributes using devm_hwmon_device_register_with_groups(),
it should lock its attribute accesses.

Since lm63_update_device() releases the update_lock before returning, and
data->fan[0] is updated non-atomically in two steps, couldn't a concurrent
sysfs read observe a partially updated 16-bit state?

Could this torn read result in wildly inaccurate fan speed values being
reported? The subsequent patch ('hwmon: (lm63) Add locking to avoid TOCTOU')
fixes similar locking issues but explicitly omits show_fan().

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260416090752.97392-1-hanguidong02@gmail.com?part=1

Re: [PATCH 1/2] hwmon: (lm63) Convert macro to function to avoid TOCTOU

[Gui-Dong Han]

On Thu, Apr 16, 2026 at 6:41 PM <sashiko-bot@kernel.org> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] Incomplete TOCTOU fix allows compiler-induced divide-by-zero crash in `show_fan()`.
> - [Medium] Lockless sysfs read in `show_fan()` causes torn reads and violates hwmon subsystem locking guidelines.
> --
>
> commit ebc10731137639981842ea19f80f0b969db71842
> Author: Gui-Dong Han <hanguidong02@gmail.com>
>
> hwmon: (lm63) Convert macro to function to avoid TOCTOU
>
> The patch converts the FAN_FROM_REG macro to a static function
> fan_from_reg() to evaluate its argument only once, avoiding a TOCTOU
> race and potential divide-by-zero error.
>
> Link: https://lore.kernel.org/linux-hwmon/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com/
>
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index 035176a98ce9c..da132b267c584 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -109,8 +109,14 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
> >   * adapted accordingly.
> >   */
> >
> > -#define FAN_FROM_REG(reg)    ((reg) == 0xFFFC || (reg) == 0 ? 0 : \
> > -                              5400000 / (reg))
> > +static int fan_from_reg(int reg)
> > +{
> > +     if (reg == 0xFFFC || reg == 0)
> > +             return 0;
> > +
> > +     return 5400000 / reg;
> > +}
> > +
>
> [ ... ]
>
> > @@ -333,7 +339,7 @@ static ssize_t show_fan(struct device *dev, struct device_attribute *devattr,
> >  {
> >       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >       struct lm63_data *data = lm63_update_device(dev);
> > -     return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index]));
> > +     return sprintf(buf, "%d\n", fan_from_reg(data->fan[attr->index]));
>
> Since fan_from_reg() is a small static function, won't the compiler likely
> inline it?
>
> If it gets inlined, couldn't the compiler optimize the code by reloading
> data->fan[attr->index] from memory multiple times (once for the 0 check, and
> again for the division)?

I am not an expert on compiler optimization. I consulted a few
different models and got mixed answers. Unlike a macro with a side
effect (like ++), an inlined function doesn't strictly force multiple
memory fetches, so it's a bit ambiguous.

>
> If lm63_update_device() concurrently writes to data->fan[0] in two non-atomic
> steps (where the first step can momentarily set the value to 0), and a reload
> happens exactly when the value is 0, could this still cause a divide-by-zero
> panic?
>
> Note that the subsequent patch in the series ('hwmon: (lm63) Add locking
> to avoid TOCTOU') explicitly skips show_fan(), leaving this unfixed.
>
> Additionally, is it safe to read data->fan locklessly here?
>
> According to the hwmon subsystem guidelines, drivers must implement locking
> for attributes registered outside the core-serialized info API. Because this
> driver registers its attributes using devm_hwmon_device_register_with_groups(),
> it should lock its attribute accesses.
>
> Since lm63_update_device() releases the update_lock before returning, and
> data->fan[0] is updated non-atomically in two steps, couldn't a concurrent
> sysfs read observe a partially updated 16-bit state?

Your point about data->fan[0] being updated non-atomically in two
steps is a very nice finding. This gives me a solid reason to just add
locking to show_fan() and fix the torn read properly.

>
> Could this torn read result in wildly inaccurate fan speed values being
> reported? The subsequent patch ('hwmon: (lm63) Add locking to avoid TOCTOU')
> fixes similar locking issues but explicitly omits show_fan().
>
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260416090752.97392-1-hanguidong02@gmail.com?part=1