Re: [PATCH v3 2/2] iio: magnetometer: add support for Melexis MLX90393

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] TOCTOU data race in `mlx90393_read_avail` due to static array.
- [High] Stack-allocated buffers passed to I2C transport violate DMA rules.
- [High] `mlx90393_check_status` rejects valid responses for 4-axis reads.
- [Medium] Writing to `in_temp_scale` silently corrupts magnetometer gain configuration.
- [Medium] Incorrect physical units exposed for IIO scales (ABI violation).
- [Medium] Temperature oversampling ratio exposes internal register indices (ABI violation).
- [Low] Use of floating-point literal in `fsleep` duration.
- [Low] Misuse of `MLX90393_AXIS_MAX` for scale array length.
- [Low] Data race on configuration variables in `mlx90393_read_raw`.
--

commit 615b644e2712a4949015c117e64d449d197438d8
Author: Nikhil Gautam <nikhilgtr@gmail.com>

iio: magnetometer: add support for Melexis MLX90393

Add Industrial I/O subsystem support for the Melexis MLX90393 3-axis
magnetometer and temperature sensor.

> diff --git a/drivers/iio/magnetometer/mlx90393_core.c b/drivers/iio/magnetometer/mlx90393_core.c
> index 0000000000000..78fcab8a2b566 100644
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_core.c
[ ... ]
> +static const int mlx90393_osr2_avail[MLX90393_OSR2_MAX] = {
> +	0, 1, 2, 3,
> +};

[Severity: Medium]
Should this array expose the physical oversampling ratios (e.g., 1, 2, 4, 8)
rather than the internal register values, to comply with IIO ABI standards?

[ ... ]
> +static int mlx90393_check_status(u8 cmd, u8 status)
> +{
> +	/* Datasheet: Table 12: Status byte definition */
> +
> +	/* Always validate error bit */
> +	if (status & MLX90393_STATUS_ERROR)
> +		return -EIO;
> +
> +	switch (cmd & MLX90393_CMD_MASK) {
> +	case MLX90393_CMD_RM:
> +		/*
> +		 * D1:D0 indicates response availability
> +		 * 00 means invalid/no measurement
> +		 */
> +		if ((status & MLX90393_STATUS_RESP) == 0)
> +			return -EIO;

[Severity: High]
Does this validation break 4-axis reads?

If MLX90393_MEASURE_ALL requests 4 words, could the 2-bit counter overflow
to 00, causing the driver to incorrectly reject valid responses?

[ ... ]
> +static int mlx90393_write_reg(struct mlx90393_data *data, u8 reg, u16 val)
> +{
> +	u8 tx[4];
> +	u8 status;
> +	int ret;
> +
> +	tx[0] = MLX90393_CMD_WR;
> +	put_unaligned_be16(val, &tx[1]);
> +	/* Register address is encoded in bits [7:2] */
> +	tx[3] = reg << 2;
> +
> +	ret = mlx90393_xfer(data, tx, sizeof(tx), &status, 1);

[Severity: High]
Can passing stack-allocated buffers to the I2C transport cause memory
corruption or a kernel panic?

If the underlying I2C controller uses DMA, mapping stack memory (which might
be CONFIG_VMAP_STACK) is generally not allowed. This also applies to the rx
buffer in mlx90393_read_measurement().

[ ... ]
> +static int mlx90393_get_scale(struct mlx90393_data *data,
> +			      const struct iio_chan_spec *chan,
> +			      int *val, int *val2)
> +{
> +	enum mlx90393_axis_type axis;
> +	u8 res;
> +
> +	if (chan->channel2 == IIO_MOD_Z) {
> +		axis = MLX90393_AXIS_TYPE_Z;
> +		res = data->res_z;
> +	} else {
> +		axis = MLX90393_AXIS_TYPE_XY;
> +		res = data->res_xy;
> +	}
> +
> +	*val = 0;
> +	*val2 = mlx90393_scale_table[axis][data->gain_sel][res];
> +
> +	return IIO_VAL_INT_PLUS_NANO;

[Severity: Medium]
Do these return values violate the IIO ABI?

Is the magnetometer scale exposing Tesla (nano scale) instead of Gauss?

[ ... ]
> +static int mlx90393_write_raw(struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan,
> +			      int val, int val2,
> +			      long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE: {
> +		guard(mutex)(&data->lock);
> +		return mlx90393_set_scale(data, chan, val, val2);
> +	}

[Severity: Medium]
Does this lack of chan->type checking mean writes to the temperature scale
sysfs attribute will fall through to mlx90393_set_scale()?

Could this unintentionally alter the magnetometer gain configuration?

[ ... ]
> +static int mlx90393_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&data->lock);
> +		ret = mlx90393_read_measurement(data, chan->address, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_MAGN:
> +			return mlx90393_get_scale(data, chan, val, val2);

[Severity: Low]
Does reading driver configuration fields like data->gain_sel in
mlx90393_get_scale() without holding data->lock introduce a data race with
concurrent sysfs writes in mlx90393_write_raw()?

> +
> +		case IIO_TEMP:
> +			/*
> +			 * Datasheet Table 7: Thermal Specification
> +			 */
> +			*val = 0;
> +			*val2 = 22124;
> +			return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
Does this violate the IIO ABI by exposing Celsius instead of milli-Celsius?

[ ... ]
> +static int mlx90393_read_avail(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       const int **vals,
> +			       int *type,
> +			       int *length,
> +			       long mask)
> +{
> +	struct mlx90393_data *data = iio_priv(indio_dev);
> +	static int scale_avail[MLX90393_GAIN_MAX][MLX90393_AXIS_MAX];

[Severity: High]
Is it safe to populate and return a pointer to a static array here?

Since the IIO core reads this array outside the driver's lock, couldn't a
concurrent sysfs read from another thread or device overwrite the array
contents while it's being read?

[ ... ]
> +		*vals = &scale_avail[0][0];
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +		*length = MLX90393_GAIN_MAX * MLX90393_AXIS_MAX;

[Severity: Low]
Is MLX90393_AXIS_MAX being misused here to calculate the length?

While it evaluates to 2, shouldn't length represent the number of values
(entries * 2 elements per IIO value pair) rather than axis count?

[ ... ]
> +static int mlx90393_init(struct mlx90393_data *data)
> +{
> +	int ret;
> +	u16 reg;
> +
> +	/* Exit mode */
> +	ret = mlx90393_write_cmd(data, MLX90393_CMD_EX);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
> +	 * Wait 1 ms after EX command before issuing RT.
> +	 */
> +	fsleep(1 * USEC_PER_MSEC);
> +
> +	/* Reset device */
> +	ret = mlx90393_write_cmd(data, MLX90393_CMD_RT);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
> +	 * Wait 1.5 ms for the start-up sequence to complete.
> +	 */
> +	fsleep(1.5 * USEC_PER_MSEC);

[Severity: Low]
Could using a floating-point literal (1.5) cause build or linkage errors
on architectures where hardware floating-point arithmetic is not supported
in the kernel?

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