Reorder the channel bounds check before using it to index into the
channels array in ad7124_release_config_slot(). This prevents reading
past the end of the array.

The value read from invalid memory was not used, so this was mostly
harmless, but we still should not be reading out of bounds in the first
place.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-iio/aPi6V-hcaKReSNWK@stanley.mountain/
Fixes: 9065197e0d41 ("iio: adc: ad7124: change setup reg allocation strategy")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9d58ced7371d0af7004a81153888714e9795d4f4..ed828a82acb71342fb2eae27abfbbd86861cba53 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -586,13 +586,18 @@ static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
 
 static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
 {
-	unsigned int slot = st->channels[channel].cfg.cfg_slot;
+	unsigned int slot;
 
 	/*
-	 * All of these conditions can happen at probe when all channels are
-	 * disabled. Otherwise, they should not happen normally.
+	 * All of these early return conditions can happen at probe when all
+	 * channels are disabled. Otherwise, they should not happen normally.
 	 */
-	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED ||
+	if (channel >= st->num_channels)
+		return;
+
+	slot = st->channels[channel].cfg.cfg_slot;
+
+	if (slot == AD7124_CFG_SLOT_UNASSIGNED ||
 	    st->cfg_slot_use_count[slot] == 0)
 		return;
 

---
base-commit: 89cba586b8b4cde09c44b1896624720ea29f0205
change-id: 20251022-iio-adc-ad7124-fix-possible-oob-array-access-239be24ac692

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

On Wed, 22 Oct 2025 10:15:05 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Reorder the channel bounds check before using it to index into the
> channels array in ad7124_release_config_slot(). This prevents reading
> past the end of the array.
> 
> The value read from invalid memory was not used, so this was mostly
> harmless, but we still should not be reading out of bounds in the first
> place.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-iio/aPi6V-hcaKReSNWK@stanley.mountain/
> Fixes: 9065197e0d41 ("iio: adc: ad7124: change setup reg allocation strategy")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied to the togreg branch of iio.git. 
thanks,

J
> ---
>  drivers/iio/adc/ad7124.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9d58ced7371d0af7004a81153888714e9795d4f4..ed828a82acb71342fb2eae27abfbbd86861cba53 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -586,13 +586,18 @@ static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
>  
>  static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
>  {
> -	unsigned int slot = st->channels[channel].cfg.cfg_slot;
> +	unsigned int slot;
>  
>  	/*
> -	 * All of these conditions can happen at probe when all channels are
> -	 * disabled. Otherwise, they should not happen normally.
> +	 * All of these early return conditions can happen at probe when all
> +	 * channels are disabled. Otherwise, they should not happen normally.
>  	 */
> -	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED ||
> +	if (channel >= st->num_channels)
> +		return;
> +
> +	slot = st->channels[channel].cfg.cfg_slot;
> +
> +	if (slot == AD7124_CFG_SLOT_UNASSIGNED ||
>  	    st->cfg_slot_use_count[slot] == 0)
>  		return;
>  
> 
> ---
> base-commit: 89cba586b8b4cde09c44b1896624720ea29f0205
> change-id: 20251022-iio-adc-ad7124-fix-possible-oob-array-access-239be24ac692
> 
> Best regards,

On Wed, Oct 22, 2025 at 10:15:05AM -0500, David Lechner wrote:
> Reorder the channel bounds check before using it to index into the
> channels array in ad7124_release_config_slot(). This prevents reading
> past the end of the array.
> 
> The value read from invalid memory was not used, so this was mostly
> harmless,

I didn't spend a lot of time looking at the callers, but an out of bounds
read will cause a KASAN warning at runtime (hopefully) and if the page
we're reading from isn't mapped then it can cause a crash.

So, it's not like we can exploit this to get root but it potentially
could be annoying.

> but we still should not be reading out of bounds in the first place.

Thanks!

regards,
dan carpenter

Hi David,

One minor question inline.
Nevertheless, the fix looks good to me.

Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

On 10/22, David Lechner wrote:
> Reorder the channel bounds check before using it to index into the
> channels array in ad7124_release_config_slot(). This prevents reading
> past the end of the array.
> 
> The value read from invalid memory was not used, so this was mostly
What is considered using the value in this context? (see other comment below)

> harmless, but we still should not be reading out of bounds in the first
> place.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-iio/aPi6V-hcaKReSNWK@stanley.mountain/
> Fixes: 9065197e0d41 ("iio: adc: ad7124: change setup reg allocation strategy")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9d58ced7371d0af7004a81153888714e9795d4f4..ed828a82acb71342fb2eae27abfbbd86861cba53 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -586,13 +586,18 @@ static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
>  
>  static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
>  {
> -	unsigned int slot = st->channels[channel].cfg.cfg_slot;
> +	unsigned int slot;
>  
>  	/*
> -	 * All of these conditions can happen at probe when all channels are
> -	 * disabled. Otherwise, they should not happen normally.
> +	 * All of these early return conditions can happen at probe when all
> +	 * channels are disabled. Otherwise, they should not happen normally.
>  	 */
> -	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED ||
> +	if (channel >= st->num_channels)
> +		return;
> +
> +	slot = st->channels[channel].cfg.cfg_slot;
> +
> +	if (slot == AD7124_CFG_SLOT_UNASSIGNED ||
>  	    st->cfg_slot_use_count[slot] == 0)
Wasn't the value potentially read from invalid memory used above?
It's fixed now, so I guess there's no point in nitpicking on that.

>  		return;

Best regards,
Marcelo

On 10/22/25 11:54 AM, Marcelo Schmitt wrote:
> Hi David,
> 
> One minor question inline.
> Nevertheless, the fix looks good to me.
> 
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> On 10/22, David Lechner wrote:
>> Reorder the channel bounds check before using it to index into the
>> channels array in ad7124_release_config_slot(). This prevents reading
>> past the end of the array.
>>
>> The value read from invalid memory was not used, so this was mostly
> What is considered using the value in this context? (see other comment below)
> 
>> harmless, but we still should not be reading out of bounds in the first
>> place.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/linux-iio/aPi6V-hcaKReSNWK@stanley.mountain/
>> Fixes: 9065197e0d41 ("iio: adc: ad7124: change setup reg allocation strategy")
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/iio/adc/ad7124.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
>> index 9d58ced7371d0af7004a81153888714e9795d4f4..ed828a82acb71342fb2eae27abfbbd86861cba53 100644
>> --- a/drivers/iio/adc/ad7124.c
>> +++ b/drivers/iio/adc/ad7124.c
>> @@ -586,13 +586,18 @@ static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
>>  
>>  static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
>>  {
>> -	unsigned int slot = st->channels[channel].cfg.cfg_slot;
>> +	unsigned int slot;
>>  
>>  	/*
>> -	 * All of these conditions can happen at probe when all channels are
>> -	 * disabled. Otherwise, they should not happen normally.
>> +	 * All of these early return conditions can happen at probe when all
>> +	 * channels are disabled. Otherwise, they should not happen normally.
>>  	 */
>> -	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED ||
>> +	if (channel >= st->num_channels)
>> +		return;
>> +
>> +	slot = st->channels[channel].cfg.cfg_slot;
>> +
>> +	if (slot == AD7124_CFG_SLOT_UNASSIGNED ||
>>  	    st->cfg_slot_use_count[slot] == 0)
> Wasn't the value potentially read from invalid memory used above?
> It's fixed now, so I guess there's no point in nitpicking on that.

This code was unreachable with an undefined slot even before
this change because of the check to channel >= st->num_channels
before it.

> 
>>  		return;
> 
> Best regards,
> Marcelo

On 10/22, David Lechner wrote:
> On 10/22/25 11:54 AM, Marcelo Schmitt wrote:
> > Hi David,
> > 
> > One minor question inline.
> > Nevertheless, the fix looks good to me.
> > 
> > Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> > 
> > On 10/22, David Lechner wrote:
> >> Reorder the channel bounds check before using it to index into the
> >> channels array in ad7124_release_config_slot(). This prevents reading
> >> past the end of the array.
> >>
> >> The value read from invalid memory was not used, so this was mostly
> > What is considered using the value in this context? (see other comment below)
> > 
> >> harmless, but we still should not be reading out of bounds in the first
> >> place.
> >>
> >> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> Closes: https://lore.kernel.org/linux-iio/aPi6V-hcaKReSNWK@stanley.mountain/
> >> Fixes: 9065197e0d41 ("iio: adc: ad7124: change setup reg allocation strategy")
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---
> >>  drivers/iio/adc/ad7124.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> >> index 9d58ced7371d0af7004a81153888714e9795d4f4..ed828a82acb71342fb2eae27abfbbd86861cba53 100644
> >> --- a/drivers/iio/adc/ad7124.c
> >> +++ b/drivers/iio/adc/ad7124.c
> >> @@ -586,13 +586,18 @@ static int ad7124_request_config_slot(struct ad7124_state *st, u8 channel)
> >>  
> >>  static void ad7124_release_config_slot(struct ad7124_state *st, u8 channel)
> >>  {
> >> -	unsigned int slot = st->channels[channel].cfg.cfg_slot;
> >> +	unsigned int slot;
> >>  
> >>  	/*
> >> -	 * All of these conditions can happen at probe when all channels are
> >> -	 * disabled. Otherwise, they should not happen normally.
> >> +	 * All of these early return conditions can happen at probe when all
> >> +	 * channels are disabled. Otherwise, they should not happen normally.
> >>  	 */
> >> -	if (channel >= st->num_channels || slot == AD7124_CFG_SLOT_UNASSIGNED ||
> >> +	if (channel >= st->num_channels)
> >> +		return;
> >> +
> >> +	slot = st->channels[channel].cfg.cfg_slot;
> >> +
> >> +	if (slot == AD7124_CFG_SLOT_UNASSIGNED ||
> >>  	    st->cfg_slot_use_count[slot] == 0)
> > Wasn't the value potentially read from invalid memory used above?
> > It's fixed now, so I guess there's no point in nitpicking on that.
> 
> This code was unreachable with an undefined slot even before
> this change because of the check to channel >= st->num_channels
> before it.
> 
ah, got it. Duh, should have realized channel >= st->num_channels always true
for the invalid access case.

Thanks

> > 
> >>  		return;
> > 
> > Best regards,
> > Marcelo
>