Re: [PATCH] iio: cros_ec: fix an use-after-free in cros_ec_sensors_push_data()

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Aug 2023 17:43:39 +0800
Tzung-Bi Shih <tzungbi@kernel.org> wrote:

> cros_ec_sensors_push_data() reads some `indio_dev` states (e.g.
> iio_buffer_enabled() and `indio_dev->active_scan_mask`) without holding
> the `mlock`.
> 
> An use-after-free on `indio_dev->active_scan_mask` was observed.  The
> call trace:
> [...]
>  _find_next_bit
>  cros_ec_sensors_push_data
>  cros_ec_sensorhub_event
>  blocking_notifier_call_chain
>  cros_ec_irq_thread
> 
> It was caused by a race condition: one thread just freed
> `active_scan_mask` at [1]; while another thread tried to access the
> memory at [2].
> 
> Fix it by acquiring the `mlock` before accessing the `indio_dev` states.
> 
> [1]: https://elixir.bootlin.com/linux/v6.5/source/drivers/iio/industrialio-buffer.c#L1189
> [2]: https://elixir.bootlin.com/linux/v6.5/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L198
> 
> Cc: stable@vger.kernel.org
> Fixes: aa984f1ba4a4 ("iio: cros_ec: Register to cros_ec_sensorhub when EC supports FIFO")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Hi.

That's an interesting corner case and the comment above the iio_buffer_enabled() call
at least lets us know why this happens. A normal driver cannot call
iio_push_to_buffers_with_timestamp() unless the buffer is enabled
(and hence active_scan_mask is fixed), but here, for convenience the call is made anyway
if we race such that iio_buffer_enabled() is called and correct says the buffer is
enabled, but then immediately after that it is at least briefly disabled.

Now I'm not keen on mlock being touched by a driver because is an internal implementation
detail.

Can we use iio_device_claim_buffer_mode() here?  I believe that has the right handling
even though I don't think we've used it to protect iio_push_* before.  Normally it's about
enforcing we stay in the mode if the read out of a channel needs to be handled differently
in a read_raw() callback.

if (iio_device_claim_buffer_mode(indio_dev) < 0) {
	/* Not in buffer mode so fine to drop out - we got -EBUSY*/
	return 0;
}
//Otherwise mlock is held - though that's an implementation detail all we care about is we can't exit buffer mode.
...
iio_push_...
iio_device_release_buffer_mode(indio_dev);
return 0;

Thanks for the bug report and analysis.

Jonathan



> ---
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index b72d39fc2434..a514d0dbafc7 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -182,17 +182,20 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
>  			      s16 *data,
>  			      s64 timestamp)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
>  	s16 *out;
>  	s64 delta;
>  	unsigned int i;
>  
> +	mutex_lock(&iio_dev_opaque->mlock);
> +
>  	/*
>  	 * Ignore samples if the buffer is not set: it is needed if the ODR is
>  	 * set but the buffer is not enabled yet.
>  	 */
>  	if (!iio_buffer_enabled(indio_dev))
> -		return 0;
> +		goto exit;
>  
>  	out = (s16 *)st->samples;
>  	for_each_set_bit(i,
> @@ -210,6 +213,8 @@ int cros_ec_sensors_push_data(struct iio_dev *indio_dev,
>  	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
>  					   timestamp + delta);
>  
> +exit:
> +	mutex_unlock(&iio_dev_opaque->mlock);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_sensors_push_data);