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

On Thu, Apr 16, 2026 at 7:44 PM <[email protected]> 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 <[email protected]>
>
> 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/[email protected]?part=2