Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] I2C read errors are silently ignored due to implicit type conversion (signed `int` compared to `size_t`), causing the driver to process uninitialized stack data.
- [High] Partial I2C reads cause a positive return value to bypass caller error checks, leaking uninitialized kernel stack memory to userspace.
- [High] The `vdd` regulator is leaked during device unbind due to reversed devres execution order.
- [High] The data polling logic instantly terminates in continuous mode, failing to wait for readings and silently swallowing I2C communication failures.
- [High] Probe returns success (0) instead of an error code when device match data is missing, leading to an uninitialized device state and a subsequent NULL pointer dereference.
- [Medium] I2C write errors are ignored when changing the data rate or scale, silently leaving the software state out of sync with hardware.
--

commit 08fffdde41976a365855c8545a761a652c4906e0
Author: jszczudlo <jakubszczudlo40@gmail.com>

iio: adc: Add ti-ads1110 support to ti-ads1100 driver
    
add ADS1100 support
make changing gain and datarate wait for new reading
fix unbalanced regulator disable when removing in singleshot mode

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d6..11d6fe1e8abc2 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
[ ... ]
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < sizeof(buffer)) {

[Severity: High]
Does this implicit type conversion mask negative error codes?

`sizeof(buffer)` evaluates to a `size_t`. When `ret` is compared to 
`sizeof(buffer)`, it gets promoted to an unsigned value. 

If `i2c_master_recv()` returns a negative error code like `-EIO`, it becomes
a massive positive number, bypassing the error check and allowing the driver
to process uninitialized stack data.

This same signedness comparison issue appears in `ads1100_get_adc_result()` 
and `ads1100_setup()`.

> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return 0;
> +	}
> +
> +	int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> +	return return_val;

[Severity: High]
Will this polling logic ever actually wait?

According to the datasheet, the ST/BSY bit is always read as 0 in continuous 
conversion mode. If `ads11x0_new_data_ready()` always extracts a 0, 
`data_ready` will be 0.

When `read_poll_timeout()` checks `!data_ready`, it evaluates to true
immediately on the first iteration, causing the loop to exit without waiting.

> +}
[ ... ]
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
[ ... ]
>  	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>  
> +	pm_runtime_mark_last_busy(&data->client->dev);
>  	pm_runtime_put_autosuspend(&data->client->dev);
>  
> -	if (ret < 0) {
> +	if (ret < sizeof(buffer)) {
>  		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>  		return ret;

[Severity: High]
If a partial read occurs, could this leak uninitialized stack memory to
userspace?

If `i2c_master_recv()` returns a positive short read length (like 1), this
error path correctly catches it and returns `ret` (which is 1).

However, the caller `ads1100_read_raw()` expects negative error codes:

drivers/iio/adc/ti-ads1100.c:ads1100_read_raw() {
    ...
    ret = ads1100_get_adc_result(data, chan->address, val);
    iio_device_release_direct(indio_dev);
    if (ret < 0)
        return ret;
        
    return IIO_VAL_INT;
}

Because `ret` is positive, `ads1100_read_raw()` will bypass its error check
and return `IIO_VAL_INT` (success) while `*val` remains uninitialized.

>  	}
[ ... ]
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
[ ... ]
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads11x0_poll_data_ready(data);

[Severity: Medium]
Is it intentional to discard the return value of `ads1100_set_config_bits()`
here?

If the I2C write fails, the hardware and software state will be out of sync,
but the driver will proceed to poll and falsely report successful
configuration changes. The same issue appears in `ads1100_set_data_rate()`.

> +
> +	pm_runtime_mark_last_busy(&data->client->dev);
> +	pm_runtime_put_autosuspend(&data->client->dev);
> +
> +	return ret;
>  }
[ ... ]
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
>  	return 0;
>  }
>  
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
>  {
> -	regulator_disable(reg);
> +	struct ads1100_data *ads1100_data = data;
> +
> +	/* Disable when not already disabled by the driver */
> +	if (!(ads1100_data->config & ADS1100_CFG_SC))
> +		regulator_disable(ads1100_data->reg_vdd);
>  }

[Severity: High]
Could this leak the `vdd` regulator during device unbind due to the devres
execution order?

Devres executes registered actions in reverse order (LIFO). During unbind,
`ads1100_disable_continuous()` executes first and unconditionally sets
`ADS1100_CFG_SC`.

When `ads1100_reg_disable()` runs immediately after, the `ADS1100_CFG_SC` bit
is already set, causing this condition to fail and permanently skipping the
`regulator_disable()` call.

>  
>  static void ads1100_disable_continuous(void *data)
[ ... ]
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
[ ... ]
> +	model = device_get_match_data(dev);
> +	if (!model)
> +		return dev_err_probe(dev, ret,
> +				"Can't set device data\n");

[Severity: High]
Does this return a success code when `device_get_match_data()` fails?

The preceding call to `devm_add_action_or_reset()` succeeds, leaving `ret`
as exactly 0. 

Passing a 0 `ret` to `dev_err_probe()` will cause the probe function to
return 0 (success) instead of a valid error code (like `-ENODEV`). The
device registers successfully with uninitialized device data, leading to a
NULL pointer dereference when userspace tries to read from the device.

> +
> +	data->data_rate = model->data_rate;
> +	data->has_reference_voltage = model->has_reference_voltage;
> +
>  	ret = ads1100_setup(data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607183542.368184-1-jakubszczudlo40@gmail.com?part=3