Message-ID: <3f5feb87-330c-4342-88a1-d5076538a86d@roeck-us.net>
Date: Thu, 5 Jun 2025 07:33:22 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Gui-Dong Han <hanguidong02@...il.com>
Cc: vt8231@...denengine.co.uk, steve.glendinning@...well.net,
	jdelvare@...e.com, linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon
 subsystem

On Thu, Jun 05, 2025 at 07:49:37PM +0800, Gui-Dong Han wrote:
> Hello maintainers,
> 
> I would like to report a class of pervasive Time-of-Check to
> Time-of-Use (TOCTOU) bugs within the drivers/hwmon/ subsystem. These
> issues can lead to various problems, including kernel panics and
> incorrect hardware behavior, due to race conditions when accessing
> shared data without proper synchronization.
> 
> The TOCTOU vulnerabilities stem from several common patterns:
> 
> 1.  Use of macros that access contended variables multiple times
> without locking.
>     Many macros, such as FAN_FROM_REG, FAN_TO_REG, RPM_FROM_REG,
> IN_FROM_REG, and TEMP_OFFSET_FROM_REG, access their arguments (which
> are often contended variables) multiple times.
>     For example, in drivers/hwmon/vt8231.c (v6.15-rc7), the fan_show
> function uses FAN_FROM_REG:
> 
>     /* Fans */
>     static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
>     char *buf)
>     {
>     struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>     int nr = sensor_attr->index;
>     struct vt8231_data *data = vt8231_update_device(dev);
>     return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr],
>     DIV_FROM_REG(data->fan_div[nr])));
>     }
> 
>     The FAN_FROM_REG macro, typically defined as #define
> FAN_FROM_REG(val, div) ((val) == 0 ? 0 : 1310720 / ((val) * (div))),
> checks val for zero and then uses val in a division. If data->fan[nr]
> (which becomes val) is a contended variable, it could be non-zero at
> the check but modified to zero by another thread before its use in the
> division, leading to a division-by-zero error.
> 
> 2.  Checking and then using contended variables without locking.
>     Some code paths check a contended variable and then use it,
> assuming its state hasn't changed. This assumption is unsafe without
> locks.
>     For example, in drivers/hwmon/emc2103.c (v6.15-rc7),
> fan1_input_show contains:
> 
>     static ssize_t
>     fan1_input_show(struct device *dev, struct device_attribute *da, char *buf)
>     {
>     struct emc2103_data *data = emc2103_update_device(dev);
>     int rpm = 0;
>     if (data->fan_tach != 0) // Check
>     rpm = (FAN_RPM_FACTOR * data->fan_multiplier) / data->fan_tach; // Use
>     return sprintf(buf, "%d\n", rpm);
>     }
> 
>     Here, data->fan_tach might be non-zero during the check but
> changed to zero by a concurrent operation before the division, again
> risking a division-by-zero.
> 
> 3.  Insufficient lock scope when updating contended variables.
>     In some update functions, locks are taken too late, after shared
> data has already been read.
>     For example, in drivers/hwmon/vt8231.c (v6.15-rc7), the
> fan_div_store function reads old and calculates min before acquiring
> data->update_lock:
> 
>     static ssize_t fan_div_store(struct device *dev,
>          struct device_attribute *attr, const char *buf,
>          size_t count)
>     {
>     struct vt8231_data *data = dev_get_drvdata(dev);
>     struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>     unsigned long val;
>     int nr = sensor_attr->index;
>     // 'old' and 'min' are read/calculated from shared data before lock
>     int old = vt8231_read_value(data, VT8231_REG_FANDIV);
>     long min = FAN_FROM_REG(data->fan_min[nr],
>     DIV_FROM_REG(data->fan_div[nr]));
>     int err;
> 
>     err = kstrtoul(buf, 10, &val);
>     if (err)
>     return err;
> 
>     mutex_lock(&data->update_lock);
>     // ... (rest of the function)
>     data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
>     vt8231_write_value(data, VT8231_REG_FAN_MIN(nr), data->fan_min[nr]);
>     // ...
>     mutex_unlock(&data->update_lock);
>     return count;
>     }
> 
>     If there's contention for data->update_lock, old and min can
> become stale by the time the lock is acquired. Subsequent operations
> based on these stale values (e.g., calculating data->fan_min[nr]) can
> lead to incorrect hardware configuration. Furthermore, the calculation
> of min itself is unsynchronized and might use inconsistent
> data->fan_min[nr] and data->fan_div[nr] values if another thread is
> concurrently updating these fields. While Linux kernel coding style
> often encourages declaring variables at the beginning of a block,
> their values, if derived from shared data, should be obtained under
> lock protection.
> 
> Consequences of these bugs:
> 
> Kernel Panics: Division-by-zero errors, particularly in macros like
> FAN_FROM_REG, FAN_TO_REG, and RPM_FROM_REG.
> Incorrect Data: For macros not involving division (e.g.,
> TEMP_OFFSET_FROM_REG), TOCTOU on variable values can lead to incorrect
> calculations and reporting of sensor data.
> Hardware Misconfiguration: Updating hardware registers based on stale
> or inconsistently read data (due to insufficient lock scope) can lead
> to illegal values being written to ports.
> 
> Scope and Prevalence:
> 
> These TOCTOU issues are unfortunately widespread across the hwmon subsystem.
> For instance, the FAN_FROM_REG macro pattern is found in numerous
> files, including but not limited to:
> adm1026.c, adm1031.c, gl518sm.c, gl520sm.c, it87.c, lm63.c, lm80.c,
> lm85.c, lm87.c, max6639.c, pc87360.c, smsc47m1.c, via686a.c, vt8231.c,
> w83627hf.c, w83791d.c, w83792d.c, w83l786ng.c.
> 
> Other instances of manually coded TOCTOU include functions like
> fan1_input_show and fan1_target_show in drivers/hwmon/emc2103.c
> (v6.15-rc7), and max6620_read in drivers/hwmon/max6620.c. I have not
> been able to catalogue all instances.
> 
> Many of these bugs appear to have been introduced very early. For
> example, the issues in drivers/hwmon/vt8231.c seem to date back to the
> driver's introduction around kernel version 2.6.
> 
> Suggested Fixes and Discussion:
> 
> I am not certain what the most appropriate strategy for fixing these
> widespread issues would be (e.g., per-file fixes or a more generalized
> approach) and would appreciate your guidance. However, some potential
> remediation steps include:
> 
> 1.  Convert multi-access macros to inline functions: Macros like
> FAN_FROM_REG that access their arguments multiple times should ideally
> be converted to static inline functions. This would ensure that
> arguments are evaluated only once, mitigating races related to
> multiple accesses of the same underlying variable within the macro's
> expansion.
> 2.  Expand lock scopes: For functions that update shared data, such as
> fan_div_store in vt8231.c, the critical sections protected by locks
> need to be carefully reviewed and potentially expanded. All reads of
> shared data that are used in subsequent calculations or writes under
> the lock should occur after the lock is acquired.
> 3.  Ensure consistent reads for multiple related variables: For read
> operations involving multiple distinct but related pieces of shared
> data (e.g., fan_show in vt8231.c which uses both data->fan[nr] and
> data->fan_div[nr]), merely converting macros to functions might not be
> sufficient if these variables can be updated non-atomically relative
> to each other. Such scenarios might require acquiring a lock to ensure
> a consistent snapshot of all related data is read and used.
> 
> I would like to discuss these issues further and collaborate on the
> best way to address them comprehensively.
> 

I'd suggest to start submitting patches, with the goal of minimizing
the scope of changes. Sometimes that may mean expanding the scope of
locks, sometimes it may mean converting macros to functions. When
converting to functions, it doesn't have to be inline functions: I'd
leave that up to the compiler to decide. None of that code is performance
critical.

Also, it might make sense to add a note to
Documentation/hwmon/submitting-patches.rst, to "avoid calculations in
macros", explaining that this may result in the kind of problem explained
here.

Thanks,
Guenter

Powered by blists - more mailing lists