Re: [PATCH] iio: imu: kmx61: Fix TOCTOU race condition

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 May 2026 12:37:56 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, May 12, 2026 at 12:10 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 12 May 2026 10:30:42 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> >  
> > > On Tue, May 12, 2026 at 10:25 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  
> > > >
> > > > On Tue, May 12, 2026 at 6:17 PM Maxwell Doose <m32285159@gmail.com> wrote:  
> > > > >
> > > > > On Tue May 12, 2026 at 7:03 AM CDT, Maxwell Doose wrote:  
> > > > > > A Time-of-check to Time-of-use race condition is present in
> > > > > > kmx61_write_event_config(). Move the mutex_lock() call above it to fix
> > > > > > it.  
> > > >
> > > > I think you want to elaborate a bit more on this. Id est explain why
> > > > ev_enable_state needs to be protected. Not everybody is willing to go
> > > > to some site to read some AI reports and interpreted them.
> > > >  
> > >
> > > Can do that for v2. I believe that it needs to be protected since
> > > later we set ev_enable_state to false (basically right after). Could
> > > be wrong of course, but Jonathan did confirm the TOCTOU.  
> >
> > I'd talk more about how we'd get a race.  If two calls enter the function
> > at the same time (which is easy to do) they may both pass this check before
> > getting to the lock.  Therefore we end up with at best pointless repeated
> > work, at worst a reference or similar count issue. You'd need to look closely
> > at what is protected to be sure whether it benign waste of time or a real
> > bug.
> >  
> 
> Well, since we're accessing the shared state via kmx61_get_data (by
> the way of iio_priv), we could check that value, pass the check, and
> then have the value change before we acquire the lock. TOCTOU race,
> no? If data->ev_enable_state is false then becomes true after we check
> the value but before we get the lock, then ev_enable_state changes to
> whatever the input variable state is. Anyways, just saying that this
> is certainly a bug. Would coming across it be common? Probably not,
> likely one of the much rarer ones, but still worth fixing.

It's not a bug if for instance two racing value sets result
in any random one of them being written.  That happens even with the
locking. It is only a bug if letting both of them write results
in something overflowing.  Some writes in cases like this are idempotent
- e.g. turning something on that is already on has no affect.

My guess would be that the power state transition could happen twice
in the enable direction resulting in +2 rather than +1 resulting in
a reference counter that may never go back to 0.

Let's look at the code and the various state combinations.
Ignore motion_trig_on for now.

state == true and data->enable_state == false

static int kmx61_write_event_config(struct iio_dev *indio_dev,
				    const struct iio_chan_spec *chan,
				    enum iio_event_type type,
				    enum iio_event_direction dir,
				    bool state)
{
	struct kmx61_data *data = kmx61_get_data(indio_dev);
	int ret = 0;

	if (state && data->ev_enable_state)
// Both threads pass this check
		return 0;

// Threads serialized in next section
	mutex_lock(&data->lock);

//ignore for simplicity - though I'm far from convinced this
//is right either for the state == false path when
//motion_trig_on == true - I think the runtime pm counters
// will not be decremented whereas they were incremented
// on the state == true, motion_trig_on = true path.
//	if (!state && data->motion_trig_on) {
//		data->ev_enable_state = false;
//		goto err_unlock;
//	}


//Next bit runs twice and in there we pm_runtime_resume_and_get()
//which will run twice for the same thing being enabled - hence the +2
//reference count mentioned above.
	ret = kmx61_set_power_state(data, state, KMX61_ACC);
	if (ret < 0)
		goto err_unlock;

	ret = kmx61_setup_any_motion_interrupt(data, state);
	if (ret < 0) {
		kmx61_set_power_state(data, false, KMX61_ACC);
		goto err_unlock;
	}

	data->ev_enable_state = state;

err_unlock:
	mutex_unlock(&data->lock);

	return ret;
}

Anyhow, looks like a nest of race conditions.  If you don't mind
could you take a look more generally at the state machines in here
and see if we have more bugs to close?  That will also help you
describe why this particular fix is right.

Jonathan

> 
> best regards,
> max
> 
> 
> 
> 
> > Jonathan
> >  
> > >
> > > best regards,
> > > max
> > >
> > >
> > >  
> > > >  
> > > > > Reported-by: sashiko <sashiko-bot@kernel.org>
> > > > > Closes: https://sashiko.dev/#/patchset/20260507223337.48437-1-m32285159%40gmail.com  
> > > >
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko  
> >