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

[Armin Wolf <W_Armin@gmx.de>]

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