[PATCH 0/2] iio: adc: ad4695: fix out of bounds array access

[PATCH 0/2] iio: adc: ad4695: fix out of bounds array access

[David Lechner]

I was doing some more testing and found a bug with the
in_temp_sampling_frequency attribute being corrupted. This is a patch
to fix that, plus a bonus patch that cleans up some related code (it
wasn't strictly part of the fix, hence the separate patch).

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
David Lechner (2):
      iio: adc: ad4695: fix out of bounds array access
      iio: adc: ad4695: simplify getting oversampling_ratio

 drivers/iio/adc/ad4695.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)
---
base-commit: ac856912f210bcff6a1cf8cf9cb2f6a1dfe85798
change-id: 20250218-iio-adc-ad4695-fix-out-of-bounds-array-access-7f16ebc3be14

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/2] iio: adc: ad4695: fix out of bounds array access

[David Lechner]

Fix some out of bounds array access of st->channels_cfg in the ad4695
driver. This array only has elements for voltage channels, but it was
also being accessed for the temperature channel in a few cases causing
reading past the end of the array.

In some cases, this was harmless because the value was read but not
used. However, the in_temp_sampling_frequency attribute shares code
with the in_voltageY_sampling_frequency attributes and was trying to
read the oversampling ratio from the st->channels_cfg array. This
resulted in a garbage value being used in the calculation and the
resulting in_temp_sampling_frequency value was incorrect.

To fix, make sure we always check that we are dealing with a voltage
channel before accessing the st->channels_cfg array and use an
oversampling ratio of 1 for the temperature channel (multiplicative
identity value) since that channel doesn't support oversampling.

Fixes: 67d63185db79 ("iio: adc: ad4695: add offload-based oversampling support")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 3a1a6f96480fd383d32397f4d3c979069111c5c9..8721cbd2af34c53f0cea32e307b9ef2da46b0cfb 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -1035,12 +1035,14 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad4695_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
-	struct ad4695_channel_config *cfg = &st->channels_cfg[chan->scan_index];
-	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
+	struct ad4695_channel_config *cfg;
 	unsigned int reg_val;
 	int ret, tmp;
 	u8 realbits;
 
+	if (chan->type == IIO_VOLTAGE)
+		cfg = &st->channels_cfg[chan->scan_index];
+
 	scan_type = iio_get_current_scan_type(indio_dev, chan);
 	if (IS_ERR(scan_type))
 		return PTR_ERR(scan_type);
@@ -1169,6 +1171,10 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 		}
 	case IIO_CHAN_INFO_SAMP_FREQ: {
 		struct pwm_state state;
+		unsigned int osr = 1;
+
+		if (chan->type == IIO_VOLTAGE)
+			osr = cfg->oversampling_ratio;
 
 		ret = pwm_get_state_hw(st->cnv_pwm, &state);
 		if (ret)
@@ -1261,7 +1267,10 @@ static int ad4695_write_raw(struct iio_dev *indio_dev,
 {
 	struct ad4695_state *st = iio_priv(indio_dev);
 	unsigned int reg_val;
-	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
+	unsigned int osr = 1;
+
+	if (chan->type == IIO_VOLTAGE)
+		osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
 
 	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
 		switch (mask) {
@@ -1361,7 +1370,10 @@ static int ad4695_read_avail(struct iio_dev *indio_dev,
 		},
 	};
 	struct ad4695_state *st = iio_priv(indio_dev);
-	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
+	unsigned int osr = 1;
+
+	if (chan->type == IIO_VOLTAGE)
+		osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBSCALE:
@@ -1713,7 +1725,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
 
 	for (i = 0; i < indio_dev->num_channels; i++) {
 		struct iio_chan_spec *chan = &st->iio_chan[i];
-		struct ad4695_channel_config *cfg = &st->channels_cfg[i];
+		struct ad4695_channel_config *cfg;
 
 		/*
 		 * NB: When using offload support, all channels need to have the
@@ -1734,6 +1746,8 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
 		if (chan->type != IIO_VOLTAGE)
 			continue;
 
+		cfg = &st->channels_cfg[i];
+
 		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
 		chan->info_mask_separate_available |=
 			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);

-- 
2.43.0

Re: [PATCH 1/2] iio: adc: ad4695: fix out of bounds array access

[Jonathan Cameron]

On Tue, 18 Feb 2025 17:17:45 -0600
David Lechner <dlechner@baylibre.com> wrote:

> Fix some out of bounds array access of st->channels_cfg in the ad4695
> driver. This array only has elements for voltage channels, but it was
> also being accessed for the temperature channel in a few cases causing
> reading past the end of the array.
> 
> In some cases, this was harmless because the value was read but not
> used. However, the in_temp_sampling_frequency attribute shares code
> with the in_voltageY_sampling_frequency attributes and was trying to
> read the oversampling ratio from the st->channels_cfg array. This
> resulted in a garbage value being used in the calculation and the
> resulting in_temp_sampling_frequency value was incorrect.
> 
> To fix, make sure we always check that we are dealing with a voltage
> channel before accessing the st->channels_cfg array and use an
> oversampling ratio of 1 for the temperature channel (multiplicative
> identity value) since that channel doesn't support oversampling.
> 
> Fixes: 67d63185db79 ("iio: adc: ad4695: add offload-based oversampling support")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
This got a little fiddly as I applied the iio_device_claim_direct() patch
first and that includes making use of osr in calibbias for the voltage
channel. I switched that back to accessing it via cfg-> now there isn't a convenient
global copy in read_raw()


So with that tweak both patches applied to the togreg branch of iio.git

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad4695.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 3a1a6f96480fd383d32397f4d3c979069111c5c9..8721cbd2af34c53f0cea32e307b9ef2da46b0cfb 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -1035,12 +1035,14 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad4695_state *st = iio_priv(indio_dev);
>  	const struct iio_scan_type *scan_type;
> -	struct ad4695_channel_config *cfg = &st->channels_cfg[chan->scan_index];
> -	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
> +	struct ad4695_channel_config *cfg;
>  	unsigned int reg_val;
>  	int ret, tmp;
>  	u8 realbits;
>  
> +	if (chan->type == IIO_VOLTAGE)
> +		cfg = &st->channels_cfg[chan->scan_index];
> +
>  	scan_type = iio_get_current_scan_type(indio_dev, chan);
>  	if (IS_ERR(scan_type))
>  		return PTR_ERR(scan_type);
> @@ -1169,6 +1171,10 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>  		}
>  	case IIO_CHAN_INFO_SAMP_FREQ: {
>  		struct pwm_state state;
> +		unsigned int osr = 1;
> +
> +		if (chan->type == IIO_VOLTAGE)
> +			osr = cfg->oversampling_ratio;
>  
>  		ret = pwm_get_state_hw(st->cnv_pwm, &state);
>  		if (ret)
> @@ -1261,7 +1267,10 @@ static int ad4695_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad4695_state *st = iio_priv(indio_dev);
>  	unsigned int reg_val;
> -	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
> +	unsigned int osr = 1;
> +
> +	if (chan->type == IIO_VOLTAGE)
> +		osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
>  
>  	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>  		switch (mask) {
> @@ -1361,7 +1370,10 @@ static int ad4695_read_avail(struct iio_dev *indio_dev,
>  		},
>  	};
>  	struct ad4695_state *st = iio_priv(indio_dev);
> -	unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
> +	unsigned int osr = 1;
> +
> +	if (chan->type == IIO_VOLTAGE)
> +		osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBSCALE:
> @@ -1713,7 +1725,7 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
>  
>  	for (i = 0; i < indio_dev->num_channels; i++) {
>  		struct iio_chan_spec *chan = &st->iio_chan[i];
> -		struct ad4695_channel_config *cfg = &st->channels_cfg[i];
> +		struct ad4695_channel_config *cfg;
>  
>  		/*
>  		 * NB: When using offload support, all channels need to have the
> @@ -1734,6 +1746,8 @@ static int ad4695_probe_spi_offload(struct iio_dev *indio_dev,
>  		if (chan->type != IIO_VOLTAGE)
>  			continue;
>  
> +		cfg = &st->channels_cfg[i];
> +
>  		chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
>  		chan->info_mask_separate_available |=
>  			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO);
> 

[PATCH 2/2] iio: adc: ad4695: simplify getting oversampling_ratio

[David Lechner]

We already have a local variable that holds a pointer to
st->channels_cfg[chan->scan_index]. Use that to simplify the code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 8721cbd2af34c53f0cea32e307b9ef2da46b0cfb..b38d2b3ccbfca10dfe5b05c3a96ba00f8838b89c 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -1164,7 +1164,7 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		switch (chan->type) {
 		case IIO_VOLTAGE:
-			*val = st->channels_cfg[chan->scan_index].oversampling_ratio;
+			*val = cfg->oversampling_ratio;
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;

-- 
2.43.0

Re: [PATCH 0/2] iio: adc: ad4695: fix out of bounds array access

[Trevor Gamblin]

On 2025-02-18 18:17, David Lechner wrote:
> I was doing some more testing and found a bug with the
> in_temp_sampling_frequency attribute being corrupted. This is a patch
> to fix that, plus a bonus patch that cleans up some related code (it
> wasn't strictly part of the fix, hence the separate patch).
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> David Lechner (2):
>        iio: adc: ad4695: fix out of bounds array access
>        iio: adc: ad4695: simplify getting oversampling_ratio
>
>   drivers/iio/adc/ad4695.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
> ---
> base-commit: ac856912f210bcff6a1cf8cf9cb2f6a1dfe85798
> change-id: 20250218-iio-adc-ad4695-fix-out-of-bounds-array-access-7f16ebc3be14
>
> Best regards,

Re: [PATCH 0/2] iio: adc: ad4695: fix out of bounds array access

[Nuno Sá]

On Tue, 2025-02-18 at 17:17 -0600, David Lechner wrote:
> I was doing some more testing and found a bug with the
> in_temp_sampling_frequency attribute being corrupted. This is a patch
> to fix that, plus a bonus patch that cleans up some related code (it
> wasn't strictly part of the fix, hence the separate patch).
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

> David Lechner (2):
>       iio: adc: ad4695: fix out of bounds array access
>       iio: adc: ad4695: simplify getting oversampling_ratio
> 
>  drivers/iio/adc/ad4695.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> ---
> base-commit: ac856912f210bcff6a1cf8cf9cb2f6a1dfe85798
> change-id: 20250218-iio-adc-ad4695-fix-out-of-bounds-array-access-7f16ebc3be14
> 
> Best regards,