[PATCH] ad7923: fix array out of bounds in ad7923_update_scan_mode()

[PATCH] ad7923: fix array out of bounds in ad7923_update_scan_mode()

[Zicheng Qu]

In the ad7923_update_scan_mode() , the variable len may exceed the length
of the st->tx_buf array, leading to an array overflow issue. The final
value of len depends on active_scan_mask (an unsigned long) and
num_channels-1 (an integer), with an upper limit of num_channels-1. In
the ad7923_probe() function, when assigning to indio_dev->num_channels,
its  size is not checked. Therefore, in ad7923_update_scan_mode(), since
active_scan_mask is an unsigned long and num_channels has no set upper
limit, an overflow might occur.

Fixes: 0eac259db28f ("IIO ADC support for AD7923")
Cc: <stable@vger.kernel.org>
Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
---
 drivers/iio/adc/ad7923.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index 09680015a7ab..82b341709a64 100644
--- a/drivers/iio/adc/ad7923.c
+++ b/drivers/iio/adc/ad7923.c
@@ -170,6 +170,8 @@ static int ad7923_update_scan_mode(struct iio_dev *indio_dev,
 	 * skip that one.
 	 */
 	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) {
+		if (len >= 4)
+			return -EINVAL;
 		cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) |
 			AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
 			st->settings;
-- 
2.34.1

Re: [PATCH] ad7923: fix array out of bounds in ad7923_update_scan_mode()

[Jonathan Cameron]

On Mon, 28 Oct 2024 14:23:57 +0000
Zicheng Qu <quzicheng@huawei.com> wrote:

> In the ad7923_update_scan_mode() , the variable len may exceed the length
> of the st->tx_buf array, leading to an array overflow issue. The final
> value of len depends on active_scan_mask (an unsigned long) and
> num_channels-1 (an integer), with an upper limit of num_channels-1. In
> the ad7923_probe() function, when assigning to indio_dev->num_channels,
> its  size is not checked. Therefore, in ad7923_update_scan_mode(), since
> active_scan_mask is an unsigned long and num_channels has no set upper
> limit, an overflow might occur.
> 
> Fixes: 0eac259db28f ("IIO ADC support for AD7923")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
Thanks.
This looks to be a valid bug but a wrong fix. Fairly sure the number of channels
supported has changed at somepoint (probably with addition of more parts)
and the size of tx has not increased to match.

Nuno, could you take a look?

Jonathan


> ---
>  drivers/iio/adc/ad7923.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> index 09680015a7ab..82b341709a64 100644
> --- a/drivers/iio/adc/ad7923.c
> +++ b/drivers/iio/adc/ad7923.c
> @@ -170,6 +170,8 @@ static int ad7923_update_scan_mode(struct iio_dev *indio_dev,
>  	 * skip that one.
>  	 */
>  	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels - 1) {
> +		if (len >= 4)
> +			return -EINVAL;
>  		cmd = AD7923_WRITE_CR | AD7923_CHANNEL_WRITE(i) |
>  			AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
>  			st->settings;

Re: [PATCH] ad7923: fix array out of bounds in ad7923_update_scan_mode()

[Nuno Sá]

On Mon, 2024-10-28 at 20:50 +0000, Jonathan Cameron wrote:
> On Mon, 28 Oct 2024 14:23:57 +0000
> Zicheng Qu <quzicheng@huawei.com> wrote:
> 
> > In the ad7923_update_scan_mode() , the variable len may exceed the length
> > of the st->tx_buf array, leading to an array overflow issue. The final
> > value of len depends on active_scan_mask (an unsigned long) and
> > num_channels-1 (an integer), with an upper limit of num_channels-1. In
> > the ad7923_probe() function, when assigning to indio_dev->num_channels,
> > its  size is not checked. Therefore, in ad7923_update_scan_mode(), since
> > active_scan_mask is an unsigned long and num_channels has no set upper
> > limit, an overflow might occur.
> > 
> > Fixes: 0eac259db28f ("IIO ADC support for AD7923")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> Thanks.
> This looks to be a valid bug but a wrong fix. Fairly sure the number of
> channels
> supported has changed at somepoint (probably with addition of more parts)
> and the size of tx has not increased to match.
> 
> Nuno, could you take a look?

Hi Jonathan,

Yes, the fix seems to be the wrong one (and incomplete). In

commit 851644a60d20 ("iio: adc: ad7923: Add support for the
ad7908/ad7918/ad7928")

devices with 8 channels were added but the buffers not updated. Then, you
actually partially fixed the problem in

commit 01fcf129f61b ("iio: adc: ad7923: Fix undersized rx buffer.") but only for
the rx buffer.

So to me this is the right fix (if nothing else missed):

diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index 09680015a7ab..acc44cb34f82 100644
--- a/drivers/iio/adc/ad7923.c
+++ b/drivers/iio/adc/ad7923.c
@@ -48,7 +48,7 @@

 struct ad7923_state {
        struct spi_device               *spi;
-       struct spi_transfer             ring_xfer[5];
+       struct spi_transfer             ring_xfer[9];
        struct spi_transfer             scan_single_xfer[2];
        struct spi_message              ring_msg;
        struct spi_message              scan_single_msg;
@@ -64,7 +64,7 @@ struct ad7923_state {
         * Length = 8 channels + 4 extra for 8 byte timestamp
         */
        __be16                          rx_buf[12] __aligned(IIO_DMA_MINALIGN);
-       __be16                          tx_buf[4];
+       __be16                          tx_buf[8];
};

- Nuno Sá

[PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Zicheng Qu]

The AD7923 was updated to support devices with 8 channels, but the size
of tx_buf and ring_xfer was not increased accordingly, leading to a
potential buffer overflow in ad7923_update_scan_mode().

Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928")
Cc: <stable@vger.kernel.org>
Signed-off-by: Nuno Sá <noname.nuno@gmail.com>
Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
---
v2:
- Fixed: Addressed buffer overflow in ad7923_update_scan_mode() due to 
insufficient tx_buf and ring_xfer size for 8-channel devices.
- Issue: Original patch attempted to fix the overflow by limiting the 
length, but did not address the root cause of buffer size mismatch.
- Solution: Increased tx_buf and ring_xfer sizes recommended by Nuno to 
support all 8 channels, ensuring adequate buffer capacity.
- Previous patch link: 
https://lore.kernel.org/linux-iio/20241028142357.1032380-1-quzicheng@huawei.com/T/#u
 drivers/iio/adc/ad7923.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index 09680015a7ab..acc44cb34f82 100644
--- a/drivers/iio/adc/ad7923.c
+++ b/drivers/iio/adc/ad7923.c
@@ -48,7 +48,7 @@
 
 struct ad7923_state {
 	struct spi_device		*spi;
-	struct spi_transfer		ring_xfer[5];
+	struct spi_transfer		ring_xfer[9];
 	struct spi_transfer		scan_single_xfer[2];
 	struct spi_message		ring_msg;
 	struct spi_message		scan_single_msg;
@@ -64,7 +64,7 @@ struct ad7923_state {
 	 * Length = 8 channels + 4 extra for 8 byte timestamp
 	 */
 	__be16				rx_buf[12] __aligned(IIO_DMA_MINALIGN);
-	__be16				tx_buf[4];
+	__be16				tx_buf[8];
 };
 
 struct ad7923_chip_info {
-- 
2.34.1

Re: [PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Zicheng Qu]

Gentle ping.

Re: [PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Nuno Sá]

On Tue, 2024-10-29 at 13:46 +0000, Zicheng Qu wrote:
> The AD7923 was updated to support devices with 8 channels, but the size
> of tx_buf and ring_xfer was not increased accordingly, leading to a
> potential buffer overflow in ad7923_update_scan_mode().
> 
> Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Nuno Sá <noname.nuno@gmail.com>
> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> ---

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

> v2:
> - Fixed: Addressed buffer overflow in ad7923_update_scan_mode() due to 
> insufficient tx_buf and ring_xfer size for 8-channel devices.
> - Issue: Original patch attempted to fix the overflow by limiting the 
> length, but did not address the root cause of buffer size mismatch.
> - Solution: Increased tx_buf and ring_xfer sizes recommended by Nuno to 
> support all 8 channels, ensuring adequate buffer capacity.
> - Previous patch link: 
> https://lore.kernel.org/linux-iio/20241028142357.1032380-1-quzicheng@huawei.com/T/#u
>  drivers/iio/adc/ad7923.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> index 09680015a7ab..acc44cb34f82 100644
> --- a/drivers/iio/adc/ad7923.c
> +++ b/drivers/iio/adc/ad7923.c
> @@ -48,7 +48,7 @@
>  
>  struct ad7923_state {
>  	struct spi_device		*spi;
> -	struct spi_transfer		ring_xfer[5];
> +	struct spi_transfer		ring_xfer[9];
>  	struct spi_transfer		scan_single_xfer[2];
>  	struct spi_message		ring_msg;
>  	struct spi_message		scan_single_msg;
> @@ -64,7 +64,7 @@ struct ad7923_state {
>  	 * Length = 8 channels + 4 extra for 8 byte timestamp
>  	 */
>  	__be16				rx_buf[12] __aligned(IIO_DMA_MINALIGN);
> -	__be16				tx_buf[4];
> +	__be16				tx_buf[8];
>  };
>  
>  struct ad7923_chip_info {

Re: [PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Jonathan Cameron]

On Thu, 31 Oct 2024 15:20:24 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2024-10-29 at 13:46 +0000, Zicheng Qu wrote:
> > The AD7923 was updated to support devices with 8 channels, but the size
> > of tx_buf and ring_xfer was not increased accordingly, leading to a
> > potential buffer overflow in ad7923_update_scan_mode().
> > 
> > Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Nuno Sá <noname.nuno@gmail.com>
> > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> > ---  
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 

Confusing one. I'll fix the authorship up for your analog address

Zicheng, usually a Suggested-by after checking with the author if it's
a patch in a review thread.

You can't really give someone elses' SoB without them explicitly sending it.
If Nuno let you know that was fine off the list, then just mention that under
---

This time I'm going to take Nuno's RB as fine to indicate no objection
to the SoB. Nuno, feel free to shout if you want to handle this differently.

Applied.

Jonathan


> > v2:
> > - Fixed: Addressed buffer overflow in ad7923_update_scan_mode() due to 
> > insufficient tx_buf and ring_xfer size for 8-channel devices.
> > - Issue: Original patch attempted to fix the overflow by limiting the 
> > length, but did not address the root cause of buffer size mismatch.
> > - Solution: Increased tx_buf and ring_xfer sizes recommended by Nuno to 
> > support all 8 channels, ensuring adequate buffer capacity.
> > - Previous patch link: 
> > https://lore.kernel.org/linux-iio/20241028142357.1032380-1-quzicheng@huawei.com/T/#u
> >  drivers/iio/adc/ad7923.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> > index 09680015a7ab..acc44cb34f82 100644
> > --- a/drivers/iio/adc/ad7923.c
> > +++ b/drivers/iio/adc/ad7923.c
> > @@ -48,7 +48,7 @@
> >  
> >  struct ad7923_state {
> >  	struct spi_device		*spi;
> > -	struct spi_transfer		ring_xfer[5];
> > +	struct spi_transfer		ring_xfer[9];
> >  	struct spi_transfer		scan_single_xfer[2];
> >  	struct spi_message		ring_msg;
> >  	struct spi_message		scan_single_msg;
> > @@ -64,7 +64,7 @@ struct ad7923_state {
> >  	 * Length = 8 channels + 4 extra for 8 byte timestamp
> >  	 */
> >  	__be16				rx_buf[12] __aligned(IIO_DMA_MINALIGN);
> > -	__be16				tx_buf[4];
> > +	__be16				tx_buf[8];
> >  };
> >  
> >  struct ad7923_chip_info {  
> 

Re: [PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Zicheng Qu]

Hi Jonathan and Nuno,

Thank you for pointing that out. I included Nuno's name because I think 
the final correct solution came from Nuno, and I wanted to acknowledge 
the contribution. However, I didn't realize I needed to confirm with 
Nuno before adding the sign-off.

In the future, I will ensure to discuss with Nuno or anyone else 
involved to avoid similar issues, or use "suggested by" instead. I 
apologize to Nuno for any confusion this may have caused.

Thanks for the guidance and apologize again.

Best regards,
Zicheng Qu


On 2024/11/1 5:05, Jonathan Cameron wrote:
> On Thu, 31 Oct 2024 15:20:24 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
>> On Tue, 2024-10-29 at 13:46 +0000, Zicheng Qu wrote:
>>> The AD7923 was updated to support devices with 8 channels, but the size
>>> of tx_buf and ring_xfer was not increased accordingly, leading to a
>>> potential buffer overflow in ad7923_update_scan_mode().
>>>
>>> Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the ad7908/ad7918/ad7928")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Nuno Sá <noname.nuno@gmail.com>
>>> Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
>>> ---
>> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>>
> Confusing one. I'll fix the authorship up for your analog address
>
> Zicheng, usually a Suggested-by after checking with the author if it's
> a patch in a review thread.
>
> You can't really give someone elses' SoB without them explicitly sending it.
> If Nuno let you know that was fine off the list, then just mention that under
> ---
>
> This time I'm going to take Nuno's RB as fine to indicate no objection
> to the SoB. Nuno, feel free to shout if you want to handle this differently.
>
> Applied.
>
> Jonathan
>
>
>>> v2:
>>> - Fixed: Addressed buffer overflow in ad7923_update_scan_mode() due to
>>> insufficient tx_buf and ring_xfer size for 8-channel devices.
>>> - Issue: Original patch attempted to fix the overflow by limiting the
>>> length, but did not address the root cause of buffer size mismatch.
>>> - Solution: Increased tx_buf and ring_xfer sizes recommended by Nuno to
>>> support all 8 channels, ensuring adequate buffer capacity.
>>> - Previous patch link:
>>> https://lore.kernel.org/linux-iio/20241028142357.1032380-1-quzicheng@huawei.com/T/#u
>>>   drivers/iio/adc/ad7923.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
>>> index 09680015a7ab..acc44cb34f82 100644
>>> --- a/drivers/iio/adc/ad7923.c
>>> +++ b/drivers/iio/adc/ad7923.c
>>> @@ -48,7 +48,7 @@
>>>   
>>>   struct ad7923_state {
>>>   	struct spi_device		*spi;
>>> -	struct spi_transfer		ring_xfer[5];
>>> +	struct spi_transfer		ring_xfer[9];
>>>   	struct spi_transfer		scan_single_xfer[2];
>>>   	struct spi_message		ring_msg;
>>>   	struct spi_message		scan_single_msg;
>>> @@ -64,7 +64,7 @@ struct ad7923_state {
>>>   	 * Length = 8 channels + 4 extra for 8 byte timestamp
>>>   	 */
>>>   	__be16				rx_buf[12] __aligned(IIO_DMA_MINALIGN);
>>> -	__be16				tx_buf[4];
>>> +	__be16				tx_buf[8];
>>>   };
>>>   
>>>   struct ad7923_chip_info {

Re: [PATCH v2] iio: adc: ad7923: Fix buffer overflow for tx_buf and ring_xfer

[Nuno Sá]

On Thu, 2024-10-31 at 21:05 +0000, Jonathan Cameron wrote:
> On Thu, 31 Oct 2024 15:20:24 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Tue, 2024-10-29 at 13:46 +0000, Zicheng Qu wrote:
> > > The AD7923 was updated to support devices with 8 channels, but the size
> > > of tx_buf and ring_xfer was not increased accordingly, leading to a
> > > potential buffer overflow in ad7923_update_scan_mode().
> > > 
> > > Fixes: 851644a60d20 ("iio: adc: ad7923: Add support for the
> > > ad7908/ad7918/ad7928")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Nuno Sá <noname.nuno@gmail.com>
> > > Signed-off-by: Zicheng Qu <quzicheng@huawei.com>
> > > ---  
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > 
> 
> Confusing one. I'll fix the authorship up for your analog address
> 
> Zicheng, usually a Suggested-by after checking with the author if it's
> a patch in a review thread.
> 
> You can't really give someone elses' SoB without them explicitly sending it.
> If Nuno let you know that was fine off the list, then just mention that under
> ---
> 
> This time I'm going to take Nuno's RB as fine to indicate no objection
> to the SoB. Nuno, feel free to shout if you want to handle this differently.
> 

Oh, TBH, I did not realized by SOB tag was there. I'm fine with it even though I
agree a Suggested-by would likely make more sense.

- Nuno Sá
  
> Applied.
> 
> Jonathan
> 
> 
> > > v2:
> > > - Fixed: Addressed buffer overflow in ad7923_update_scan_mode() due to 
> > > insufficient tx_buf and ring_xfer size for 8-channel devices.
> > > - Issue: Original patch attempted to fix the overflow by limiting the 
> > > length, but did not address the root cause of buffer size mismatch.
> > > - Solution: Increased tx_buf and ring_xfer sizes recommended by Nuno to 
> > > support all 8 channels, ensuring adequate buffer capacity.
> > > - Previous patch link: 
> > > https://lore.kernel.org/linux-iio/20241028142357.1032380-1-quzicheng@huawei.com/T/#u
> > >  drivers/iio/adc/ad7923.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
> > > index 09680015a7ab..acc44cb34f82 100644
> > > --- a/drivers/iio/adc/ad7923.c
> > > +++ b/drivers/iio/adc/ad7923.c
> > > @@ -48,7 +48,7 @@
> > >  
> > >  struct ad7923_state {
> > >  	struct spi_device		*spi;
> > > -	struct spi_transfer		ring_xfer[5];
> > > +	struct spi_transfer		ring_xfer[9];
> > >  	struct spi_transfer		scan_single_xfer[2];
> > >  	struct spi_message		ring_msg;
> > >  	struct spi_message		scan_single_msg;
> > > @@ -64,7 +64,7 @@ struct ad7923_state {
> > >  	 * Length = 8 channels + 4 extra for 8 byte timestamp
> > >  	 */
> > >  	__be16				rx_buf[12]
> > > __aligned(IIO_DMA_MINALIGN);
> > > -	__be16				tx_buf[4];
> > > +	__be16				tx_buf[8];
> > >  };
> > >  
> > >  struct ad7923_chip_info {  
> > 
>