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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <474af450-fe05-46d6-90d6-d5ac3408cf68@gmx.de>
Date: Mon, 9 Jun 2025 17:06:57 +0200
From: Armin Wolf <W_Armin@....de>
To: Guenter Roeck <linux@...ck-us.net>, 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, baijiaju1990@...il.com
Subject: Re: [BUG] hwmon: Widespread TOCTOU vulnerabilities in the hwmon
 subsystem

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