[BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Gui-Dong Han]

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

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Guenter Roeck]

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

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Guenter Roeck]

On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
> > 
> > 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.
> 
Actualy, that makes me wonder if it would make sense to introduce
subsystem-level locking. We could introduce a lock in struct
hwmon_device_attribute and lock it whenever a show or store function
executes in drivers/hwmon/hwmon.c. That would only help for drivers
using the _with_info API, but it would simplify driver code a lot.
Any thoughts on that ?

Thanks,
Guenter

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Gui-Dong Han]

> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
> > >
> > > 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.
> >
> Actualy, that makes me wonder if it would make sense to introduce
> subsystem-level locking. We could introduce a lock in struct
> hwmon_device_attribute and lock it whenever a show or store function
> executes in drivers/hwmon/hwmon.c. That would only help for drivers
> using the _with_info API, but it would simplify driver code a lot.
> Any thoughts on that ?

Hi Guenter,

Thanks for your quick and insightful feedback!

I agree with your suggestion. Adding a note to
Documentation/hwmon/submitting-patches.rst about avoiding calculations
in macros is also a great idea to prevent this class of bugs in the
future.

Regarding your thoughts on subsystem-level locking, it sounds like a
promising approach to simplify the drivers using the _with_info API.
As you mentioned, some drivers don't use this API, so they would still
require manual fixes.

For the subsystem-level lock itself, I was wondering if a read-write
semaphore might be more appropriate than a standard mutex. This would
prevent a single show operation from blocking other concurrent reads.
I'm not entirely sure about all the implications, but it might be
worth considering to maintain read performance.

Thanks again for your guidance.

Best regards,
Gui-Dong Han

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Armin Wolf]

Am 06.06.25 um 09:03 schrieb Gui-Dong Han:

>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>> 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.
>>>
>> Actualy, that makes me wonder if it would make sense to introduce
>> subsystem-level locking. We could introduce a lock in struct
>> hwmon_device_attribute and lock it whenever a show or store function
>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>> using the _with_info API, but it would simplify driver code a lot.
>> Any thoughts on that ?

Hi,

i am against adding a subsystem lock just to work around buggy drivers. Many drivers
should use fine-grained locking to avoid high latencies when reading a single attribute.

Thanks,
Armin Wolf

> Hi Guenter,
>
> Thanks for your quick and insightful feedback!
>
> I agree with your suggestion. Adding a note to
> Documentation/hwmon/submitting-patches.rst about avoiding calculations
> in macros is also a great idea to prevent this class of bugs in the
> future.
>
> Regarding your thoughts on subsystem-level locking, it sounds like a
> promising approach to simplify the drivers using the _with_info API.
> As you mentioned, some drivers don't use this API, so they would still
> require manual fixes.
>
> For the subsystem-level lock itself, I was wondering if a read-write
> semaphore might be more appropriate than a standard mutex. This would
> prevent a single show operation from blocking other concurrent reads.
> I'm not entirely sure about all the implications, but it might be
> worth considering to maintain read performance.
>
> Thanks again for your guidance.
>
> Best regards,
> Gui-Dong Han
>

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Guenter Roeck]

On 6/6/25 14:30, Armin Wolf wrote:
> Am 06.06.25 um 09:03 schrieb Gui-Dong Han:
> 
>>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>>> 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.
>>>>
>>> Actualy, that makes me wonder if it would make sense to introduce
>>> subsystem-level locking. We could introduce a lock in struct
>>> hwmon_device_attribute and lock it whenever a show or store function
>>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>>> using the _with_info API, but it would simplify driver code a lot.
>>> Any thoughts on that ?
> 
> Hi,
> 
> i am against adding a subsystem lock just to work around buggy drivers. Many drivers
> should use fine-grained locking to avoid high latencies when reading a single attribute.
> 

The point would be driver code simplification and increasing robustness, not
working around buggy drivers.

Anyway, what high latency are you talking about ? Serialization of attribute
accesses would only increase latency if multiple processes read attributes from
the same driver at the same time, which is hardly a typical use case.

Guenter

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Guenter Roeck]

On 6/6/25 00:03, Gui-Dong Han wrote:
>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>>
>>>> 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.
>>>
>> Actualy, that makes me wonder if it would make sense to introduce
>> subsystem-level locking. We could introduce a lock in struct
>> hwmon_device_attribute and lock it whenever a show or store function
>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>> using the _with_info API, but it would simplify driver code a lot.
>> Any thoughts on that ?
> 
> Hi Guenter,
> 
> Thanks for your quick and insightful feedback!
> 
> I agree with your suggestion. Adding a note to
> Documentation/hwmon/submitting-patches.rst about avoiding calculations
> in macros is also a great idea to prevent this class of bugs in the
> future.
> 
> Regarding your thoughts on subsystem-level locking, it sounds like a
> promising approach to simplify the drivers using the _with_info API.
> As you mentioned, some drivers don't use this API, so they would still
> require manual fixes.
> 
> For the subsystem-level lock itself, I was wondering if a read-write
> semaphore might be more appropriate than a standard mutex. This would
> prevent a single show operation from blocking other concurrent reads.
> I'm not entirely sure about all the implications, but it might be
> worth considering to maintain read performance.
> 

Various drivers need write locks when reading attributes, so that would not
work well. We'd need some flag indicating "this driver needs write locks
when reading data", and then things become complicated again, defeating the
benefit.

Guenter

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Armin Wolf]

Am 07.06.25 um 01:20 schrieb Guenter Roeck:

> On 6/6/25 14:30, Armin Wolf wrote:
>> Am 06.06.25 um 09:03 schrieb Gui-Dong Han:
>>
>>>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>>>> 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.
>>>>>
>>>> Actualy, that makes me wonder if it would make sense to introduce
>>>> subsystem-level locking. We could introduce a lock in struct
>>>> hwmon_device_attribute and lock it whenever a show or store function
>>>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>>>> using the _with_info API, but it would simplify driver code a lot.
>>>> Any thoughts on that ?
>>
>> Hi,
>>
>> i am against adding a subsystem lock just to work around buggy 
>> drivers. Many drivers
>> should use fine-grained locking to avoid high latencies when reading 
>> a single attribute.
>>
>
> The point would be driver code simplification and increasing 
> robustness, not
> working around buggy drivers.
>
> Anyway, what high latency are you talking about ? Serialization of 
> attribute
> accesses would only increase latency if multiple processes read 
> attributes from
> the same driver at the same time, which is hardly a typical use case.
>
> Guenter

Some drivers read all registers (fan, temperature, etc) when updating the readings, and depending
on the underlying bus system this might take some time. With a global lock reading a single value will thus
take as much time as reading all values.

Thanks,
Armin Wolf

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Armin Wolf]

Am 07.06.25 um 01:22 schrieb Guenter Roeck:

> On 6/6/25 00:03, Gui-Dong Han wrote:
>>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>>>
>>>>> 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.
>>>>
>>> Actualy, that makes me wonder if it would make sense to introduce
>>> subsystem-level locking. We could introduce a lock in struct
>>> hwmon_device_attribute and lock it whenever a show or store function
>>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>>> using the _with_info API, but it would simplify driver code a lot.
>>> Any thoughts on that ?
>>
>> Hi Guenter,
>>
>> Thanks for your quick and insightful feedback!
>>
>> I agree with your suggestion. Adding a note to
>> Documentation/hwmon/submitting-patches.rst about avoiding calculations
>> in macros is also a great idea to prevent this class of bugs in the
>> future.
>>
>> Regarding your thoughts on subsystem-level locking, it sounds like a
>> promising approach to simplify the drivers using the _with_info API.
>> As you mentioned, some drivers don't use this API, so they would still
>> require manual fixes.
>>
>> For the subsystem-level lock itself, I was wondering if a read-write
>> semaphore might be more appropriate than a standard mutex. This would
>> prevent a single show operation from blocking other concurrent reads.
>> I'm not entirely sure about all the implications, but it might be
>> worth considering to maintain read performance.
>>
>
> Various drivers need write locks when reading attributes, so that 
> would not
> work well. We'd need some flag indicating "this driver needs write locks
> when reading data", and then things become complicated again, 
> defeating the
> benefit.
>
> Guenter

I agree, different drivers need different locks. From my point of view drivers using the
with_info-API can easily implement such a global lock themself by using guard(). This also
allows them to choose the type of lock to use.

Thanks,
Armin Wolf

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Guenter Roeck]

On 6/9/25 08:03, Armin Wolf wrote:
> Am 07.06.25 um 01:20 schrieb Guenter Roeck:
> 
>> On 6/6/25 14:30, Armin Wolf wrote:
>>> Am 06.06.25 um 09:03 schrieb Gui-Dong Han:
>>>
>>>>> On Thu, Jun 05, 2025 at 07:33:24AM -0700, Guenter Roeck wrote:
>>>>>>> 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.
>>>>>>
>>>>> Actualy, that makes me wonder if it would make sense to introduce
>>>>> subsystem-level locking. We could introduce a lock in struct
>>>>> hwmon_device_attribute and lock it whenever a show or store function
>>>>> executes in drivers/hwmon/hwmon.c. That would only help for drivers
>>>>> using the _with_info API, but it would simplify driver code a lot.
>>>>> Any thoughts on that ?
>>>
>>> Hi,
>>>
>>> i am against adding a subsystem lock just to work around buggy drivers. Many drivers
>>> should use fine-grained locking to avoid high latencies when reading a single attribute.
>>>
>>
>> The point would be driver code simplification and increasing robustness, not
>> working around buggy drivers.
>>
>> Anyway, what high latency are you talking about ? Serialization of attribute
>> accesses would only increase latency if multiple processes read attributes from
>> the same driver at the same time, which is hardly a typical use case.
>>
>> Guenter
> 
> Some drivers read all registers (fan, temperature, etc) when updating the readings, and depending
> on the underlying bus system this might take some time. With a global lock reading a single value will thus
> take as much time as reading all values.
> 

Those very same drivers acquire a driver lock when reading all values,
and they typically do that when reading a single value, so there is no
difference. The real fix for that problem is to avoid driver-internal
caching and only read values which are actually needed.
On top of that, synchronization for the most part already happens
on the regmap (if used) or bus level, so adding another lock (or,
rather, replacing the driver lock with a subsystem lock) does not
make a difference either.

Guenter

Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon subsystem

[Andy Shevchenko]

On Thu, Jun 05, 2025 at 07:33:22AM -0700, Guenter Roeck wrote:
> On Thu, Jun 05, 2025 at 07:49:37PM +0800, Gui-Dong Han wrote:

...

> 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.

No, it might not. Do not blame a messenger (calculations), the problem is
accessing data several times. That one makes sense to add.

-- 
With Best Regards,
Andy Shevchenko