Message-ID: <CALbr=LYJ_ehtp53HXEVkSpYoub+XYSTU8Rg=o1xxMJ8=5z8B-g@mail.gmail.com>
Date: Thu, 5 Jun 2025 19:49:37 +0800
From: Gui-Dong Han <hanguidong02@...il.com>
To: vt8231@...denengine.co.uk, steve.glendinning@...well.net, 
	jdelvare@...e.com, linux@...ck-us.net
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

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.

Thank you for your attention to this matter.

Best regards,
Gui-Dong Han

Powered by blists - more mailing lists