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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250222145841.06b3417c@jic23-huawei>
Date: Sat, 22 Feb 2025 14:58:41 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Michael Hennerich <michael.hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, Trevor Gamblin
 <tgamblin@...libre.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad4695: fix out of bounds array access

On Tue, 18 Feb 2025 17:17:45 -0600
David Lechner <dlechner@...libre.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@...libre.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);
>