The function max6620_read checks shared data (tach and target) for zero before passing it to max6620_fan_tach_to_rpm, which uses it as a divisor. These accesses are currently lockless. If the data changes to zero between the check and the division, it causes a divide-by-zero error.
Explicitly acquire the update lock around these checks and calculations to ensure the data remains stable, preventing Time-of-Check to Time-of-Use (TOCTOU) race conditions.
This change also aligns the locking behavior with the hwmon_fan_alarm case, which already uses the update lock.
Link: https://lore.kernel.org/all/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8... Fixes: e8ac01e5db32 ("hwmon: Add Maxim MAX6620 hardware monitoring driver") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han hanguidong02@gmail.com --- Based on the discussion in the link, I will submit a series of patches to address TOCTOU issues in the hwmon subsystem by converting macros to functions or adjusting locking where appropriate. --- drivers/hwmon/max6620.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/hwmon/max6620.c b/drivers/hwmon/max6620.c index 13201fb755c9..0dce2f5cb61b 100644 --- a/drivers/hwmon/max6620.c +++ b/drivers/hwmon/max6620.c @@ -290,20 +290,24 @@ max6620_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, *val = max6620_fan_div_from_reg(data->fandyn[channel]); break; case hwmon_fan_input: + mutex_lock(&data->update_lock); if (data->tach[channel] == 0) { *val = 0; } else { div = max6620_fan_div_from_reg(data->fandyn[channel]); *val = max6620_fan_tach_to_rpm(div, data->tach[channel]); } + mutex_unlock(&data->update_lock); break; case hwmon_fan_target: + mutex_lock(&data->update_lock); if (data->target[channel] == 0) { *val = 0; } else { div = max6620_fan_div_from_reg(data->fandyn[channel]); *val = max6620_fan_tach_to_rpm(div, data->target[channel]); } + mutex_unlock(&data->update_lock); break; default: return -EOPNOTSUPP;
On Fri, Nov 28, 2025 at 08:43:51PM +0800, Gui-Dong Han wrote:
The function max6620_read checks shared data (tach and target) for zero before passing it to max6620_fan_tach_to_rpm, which uses it as a divisor. These accesses are currently lockless. If the data changes to zero between the check and the division, it causes a divide-by-zero error.
Explicitly acquire the update lock around these checks and calculations to ensure the data remains stable, preventing Time-of-Check to Time-of-Use (TOCTOU) race conditions.
This change also aligns the locking behavior with the hwmon_fan_alarm case, which already uses the update lock.
Link: https://lore.kernel.org/all/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8... Fixes: e8ac01e5db32 ("hwmon: Add Maxim MAX6620 hardware monitoring driver") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han hanguidong02@gmail.com
Based on the discussion in the link, I will submit a series of patches to address TOCTOU issues in the hwmon subsystem by converting macros to functions or adjusting locking where appropriate.
This patch is not necessary. The driver registers with the hwmon subsystem using devm_hwmon_device_register_with_info(). That means the hwmon subsystem handles the necessary locking. On top of that, removing the existing driver internal locking code is queued for v6.19.
Thanks, Guenter
On Sat, Nov 29, 2025 at 12:34 AM Guenter Roeck linux@roeck-us.net wrote:
On Fri, Nov 28, 2025 at 08:43:51PM +0800, Gui-Dong Han wrote:
The function max6620_read checks shared data (tach and target) for zero before passing it to max6620_fan_tach_to_rpm, which uses it as a divisor. These accesses are currently lockless. If the data changes to zero between the check and the division, it causes a divide-by-zero error.
Explicitly acquire the update lock around these checks and calculations to ensure the data remains stable, preventing Time-of-Check to Time-of-Use (TOCTOU) race conditions.
This change also aligns the locking behavior with the hwmon_fan_alarm case, which already uses the update lock.
Link: https://lore.kernel.org/all/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8... Fixes: e8ac01e5db32 ("hwmon: Add Maxim MAX6620 hardware monitoring driver") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han hanguidong02@gmail.com
Based on the discussion in the link, I will submit a series of patches to address TOCTOU issues in the hwmon subsystem by converting macros to functions or adjusting locking where appropriate.
This patch is not necessary. The driver registers with the hwmon subsystem using devm_hwmon_device_register_with_info(). That means the hwmon subsystem handles the necessary locking. On top of that, removing the existing driver internal locking code is queued for v6.19.
Hi Guenter,
Thanks for the information. I missed the new hwmon subsystem locking implementation earlier as it wasn't present in v6.17.9. I have since studied the code in v6.18-rc, and it looks like an excellent improvement. I will focus exclusively on drivers not using devm_hwmon_device_register_with_info() going forward.
In our previous discussion, you also suggested adding a note to submitting-patches.rst about "avoiding calculations in macros" to explicitly explain the risk of race conditions. Is this something you would still like to see added? If so, I would be happy to prepare a patch.
Best regards, Gui-Dong Han
On 11/28/25 09:59, Gui-Dong Han wrote:
On Sat, Nov 29, 2025 at 12:34 AM Guenter Roeck linux@roeck-us.net wrote:
On Fri, Nov 28, 2025 at 08:43:51PM +0800, Gui-Dong Han wrote:
The function max6620_read checks shared data (tach and target) for zero before passing it to max6620_fan_tach_to_rpm, which uses it as a divisor. These accesses are currently lockless. If the data changes to zero between the check and the division, it causes a divide-by-zero error.
Explicitly acquire the update lock around these checks and calculations to ensure the data remains stable, preventing Time-of-Check to Time-of-Use (TOCTOU) race conditions.
This change also aligns the locking behavior with the hwmon_fan_alarm case, which already uses the update lock.
Link: https://lore.kernel.org/all/CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8... Fixes: e8ac01e5db32 ("hwmon: Add Maxim MAX6620 hardware monitoring driver") Cc: stable@vger.kernel.org Signed-off-by: Gui-Dong Han hanguidong02@gmail.com
Based on the discussion in the link, I will submit a series of patches to address TOCTOU issues in the hwmon subsystem by converting macros to functions or adjusting locking where appropriate.
This patch is not necessary. The driver registers with the hwmon subsystem using devm_hwmon_device_register_with_info(). That means the hwmon subsystem handles the necessary locking. On top of that, removing the existing driver internal locking code is queued for v6.19.
Hi Guenter,
Thanks for the information. I missed the new hwmon subsystem locking implementation earlier as it wasn't present in v6.17.9. I have since studied the code in v6.18-rc, and it looks like an excellent improvement. I will focus exclusively on drivers not using devm_hwmon_device_register_with_info() going forward.
In our previous discussion, you also suggested adding a note to submitting-patches.rst about "avoiding calculations in macros" to explicitly explain the risk of race conditions. Is this something you would still like to see added? If so, I would be happy to prepare a patch.
Yes, that would be great. Thanks!
Guenter
On Sat, 29 Nov 2025 01:59:51 +0800 Gui-Dong Han hanguidong02@gmail.com wrote:
...
In our previous discussion, you also suggested adding a note to submitting-patches.rst about "avoiding calculations in macros" to explicitly explain the risk of race conditions. Is this something you would still like to see added? If so, I would be happy to prepare a patch.
The real problem with #defines evaluating their parameters more than once is just side effects of the expansion. Even if the current users just pass a simple variable, you never really know what is going to happen in the future.
There is also a secondary issue of pre-processor output 'bloat'. This happens when large #define expansions get nested. With the current headers FIELD_PREP(GENMASK(8, 5), val) expands to about 18kB [1] (even though it is just (val >> 5) & 15). I think one of your #defines get passed one of those - and then expands it several times. As well as the massive line, the compiler may well generate the code multiple times. (CSE will normally stop foo->bar[x] being executed multiple times).
[1] Nothing like the 30MB that triple nested min() generated for a while.
David
linux-stable-mirror@lists.linaro.org
-
david laight -
Guenter Roeck -
Gui-Dong Han