Re: [PATCH v2 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: adc: ti-tla2528: uninitialized stack read in tla2528_read_sample()
- [Low] iio: adc: ti-tla2528: positional initializer in struct i2c_device_id table
--

commit 25a176a55c9c7afc00ecb1f3b6cd5d101cdeee77
Author: Maxime Chevallier <maxime.chevallier@bootlin.com>

iio: adc: add driver for Texas Instruments TLA2528 adc

This commit adds an IIO ADC driver for the Texas Instruments TLA2528 8-channel
12-bit I2C ADC supporting raw channel reads.

> diff --git a/drivers/iio/adc/ti-tla2528.c b/drivers/iio/adc/ti-tla2528.c
> new file mode 100644
> index 0000000000000..287e52dc9d6d2
> --- /dev/null
> +++ b/drivers/iio/adc/ti-tla2528.c
> @@ -0,0 +1,245 @@

[ ... ]

> +static int tla2528_read_sample(const struct i2c_client *client)
> +{
> +	__be16 data;
> +	int ret;
> +
> +	ret = i2c_master_recv(client, (char *)&data, sizeof(data));
> +	if (ret < 0)
> +		return ret;
> +
> +	return be16_to_cpu(data) >> 4;
> +}

[Severity: Medium]
Can tla2528_read_sample() read uninitialized stack memory from data when
called via tla2528_read_raw() -> tla2528_read() -> tla2528_read_sample()?

i2c_master_recv() calls i2c_transfer_buffer_flags():

drivers/i2c/i2c-core-base.c:i2c_transfer_buffer_flags() {
	...
	ret = i2c_transfer(client->adapter, &msg, 1);

	/*
	 * If everything went ok (i.e. 1 msg transferred), return #bytes
	 * transferred, else error code.
	 */
	return (ret == 1) ? count : ret;
}

If the underlying I2C adapter returns 0 messages transferred from
i2c_transfer() without populating the receive buffer, i2c_master_recv()
returns 0. Because tla2528_read_sample() only checks for ret < 0, it will
proceed to shift and return the uninitialized stack variable data to
userspace.

Should this check if (ret != sizeof(data)) and return -EIO when a short read
occurs?

[ ... ]

> +static const struct i2c_device_id tla2528_id[] = {
> +	{ "tla2528" },
> +	{ }
> +};

[Severity: Low]
This isn't a bug, but should the new entry in tla2528_id in
drivers/iio/adc/ti-tla2528.c use a named initializer ({ .name = "tla2528" })
to follow the I2C subsystem convention?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030164705.297354-1-maxime.chevallier@bootlin.com?part=2