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