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