Re: [PATCH v2 2/3] iio: health: add MAX86150 ECG and PPG biosensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] A stack-allocated buffer is passed to regmap_noinc_read(), which violates DMA mapping rules and can cause memory corruption if the I2C host controller uses DMA.
- [High] Redundant manual invocation of iio_trigger_put via a devm action causes a double put and a use-after-free when the IIO device is released.
- [Medium] The driver exposes unimplemented scale attributes to userspace, which unconditionally return an error.
- [Medium] The FIFO drain loop assigns incorrect, heavily clustered timestamps to buffered samples, destroying time-domain information.
- [Medium] The driver incorrectly uses devm_regulator_get_enable for optional supplies, erroneously assuming it gracefully handles missing regulators.
- [Medium] Uninitialized padding bytes in data->buf leak previous sensor readings to userspace when fewer than 3 channels are enabled.
- [Medium] Missing FIFO flush when enabling the triggered buffer leaves stale data to be processed immediately.
- [Medium] The driver calls iio_trigger_get() on its own trigger during probe, permanently elevating its own module reference count and preventing module unload.
--

commit 28ae0a6a433f8b0e50a4557f62c8f075a8c69870
Author: Md Shofiqul Islam <shofiqtest@gmail.com>

iio: health: add MAX86150 ECG and PPG biosensor driver

This commit adds a driver for the MAX86150 combined ECG and PPG biosensor.
It exposes three IIO channels and supports the IIO triggered buffer path for
continuous data capture.

> diff --git a/drivers/iio/health/max86150.c b/drivers/iio/health/max86150.c
> new file mode 100755
> index 0000000000000..70c51ccb4672b
> --- /dev/null
> +++ b/drivers/iio/health/max86150.c

[ ... ]

> +static const struct iio_chan_spec max86150_channels[] = {
> +	{
> +		/* PPG Red LED - optical intensity, 19-bit unsigned */
> +		.type               = IIO_INTENSITY,
> +		.modified           = 1,
> +		.channel2           = IIO_MOD_LIGHT_RED,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),

[Severity: Medium]
Will this cause sysfs reads for the scale attribute to always fail?

The driver exposes IIO_CHAN_INFO_SCALE for all channels here, but
max86150_read_raw() unconditionally returns -EINVAL for it. This breaks
the IIO ABI contract and might cause userspace tools to fail when discovering
channel properties.

[ ... ]

> +static int max86150_read_one_sample(struct max86150_data *data,
> +				    u32 *ppg_red, u32 *ppg_ir, s32 *ecg)
> +{
> +	u8 buf[MAX86150_SAMPLE_BYTES];  /* 9 bytes */
> +	int ret;
> +
> +	ret = regmap_noinc_read(data->regmap, MAX86150_REG_FIFO_DATA,
> +				buf, sizeof(buf));

[Severity: High]
Can this cause memory corruption if the I2C host controller uses DMA?

The buf array is allocated on the stack and passed directly to
regmap_noinc_read(). Since the regmap I2C implementation does not internally
bounce buffers, this passes a stack buffer directly to i2c_transfer(). When
CONFIG_VMAP_STACK is enabled, stack buffers are not physically contiguous and
are not DMA-safe.

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * TODO: return the hardware scale factor based on the
> +		 * configured PPG ADC range (PPG_ADC_RGE) and ECG PGA gain
> +		 * (ECG_PGA_GAIN).  For PPG, scale is nA/LSB; for ECG, uV/LSB.
> +		 */
> +		return -EINVAL;

[Severity: Medium]
Is it intentional to advertise the scale attribute in the channel spec but
unconditionally reject it here?

This will cause reads of the sysfs attribute to always fail.

[ ... ]

> +static int max86150_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct max86150_data *data = iio_priv(indio_dev);
> +	unsigned int mask = state ? MAX86150_INT_A_FULL : 0;
> +
> +	return regmap_write(data->regmap, MAX86150_REG_INT_ENABLE1, mask);

[Severity: Medium]
Does the hardware FIFO need to be flushed before unmasking the interrupt here?

Since the device continuously acquires data into the FIFO starting from
probe(), the FIFO might already be full when userspace enables the triggered
buffer. This would cause the interrupt to assert immediately and drain
extremely stale samples into the new capture session.

[ ... ]

> +		/*
> +		 * Pack only active channels at consecutive positions [0..j-1].
> +		 * iio_push_to_buffers_with_timestamp() uses scan_bytes (which
> +		 * accounts for the active channel count) to place the timestamp,
> +		 * so static indexing would misplace it when fewer than 3
> +		 * channels are enabled.
> +		 */
> +		j = 0;
> +		if (test_bit(MAX86150_IDX_PPG_RED, idev->active_scan_mask))
> +			data->buf[j++] = ppg_red;
> +		if (test_bit(MAX86150_IDX_PPG_IR, idev->active_scan_mask))
> +			data->buf[j++] = ppg_ir;
> +		if (test_bit(MAX86150_IDX_ECG, idev->active_scan_mask))
> +			data->buf[j++] = ecg;

[Severity: Medium]
Does this sequentially pack active channels without zeroing the entire buffer
first, leaking previous sensor readings?

If the active channel count is reduced, unused array elements in data->buf
retain stale data from previous captures. iio_push_to_buffers_with_timestamp()
then copies the entire memory block up to the timestamp offset, exposing the
uninitialized padding bytes to userspace.

> +
> +		iio_push_to_buffers_with_timestamp(idev, data->buf,
> +						   iio_get_time_ns(idev));

[Severity: Medium]
Will this assign the same batch execution time to all historical samples
pulled from the FIFO?

Inside the loop, fetching the current time for every sample means up to 32
samples will receive nearly identical timestamps separated only by I2C read
latencies. This misrepresents the true sample capture period (e.g., 100 Hz).
Should the driver backdate timestamps using the top-half IRQ timestamp
(pf->timestamp) instead?

[ ... ]

> +	/*
> +	 * Enable power supplies before any I2C access.  Both are optional
> +	 * in the device tree (devm_regulator_get_enable returns 0 when the
> +	 * supply is not described), but must be present and stable on real
> +	 * hardware before the first register read.
> +	 */
> +	ret = devm_regulator_get_enable(&client->dev, "vdd");

[Severity: Medium]
Will this cause the probe to fail on boards where the regulator is legitimately
missing?

The comment indicates the supply is optional and assumes this API returns 0
when it is not described. However, devm_regulator_get_enable() only returns 0
for missing regulators if the system provides a dummy regulator. Otherwise, it
returns -ENODEV, fatally aborting the probe. Should this use
devm_regulator_get_enable_optional() instead?

[ ... ]

> +		ret = devm_iio_trigger_register(&client->dev, data->trig);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Cannot register trigger\n");
> +
> +		indio_dev->trig = iio_trigger_get(data->trig);

[Severity: Medium]
Does this take a circular reference on the driver's own module?

Calling iio_trigger_get() on a trigger owned by this module increments the
module reference count during probe(). The rmmod command will then refuse to
unload the module because the reference count will never drop to 0, permanently
pinning it in memory.

> +		ret = devm_add_action_or_reset(&client->dev,
> +					       max86150_trigger_put,
> +					       data->trig);

[Severity: High]
Can this cause a double-put and a use-after-free when the IIO device is
released?

The IIO core automatically handles releasing the trigger during device
unregistration via iio_device_unregister_trigger_consumer() which implicitly
calls iio_trigger_put(). Because devm actions run subsequently,
max86150_trigger_put() drops the reference a second time, resulting in a
use-after-free of the trigger object.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623155556.13701-1-shofiqtest@gmail.com?part=2