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

["Nuno Sá" <noname.nuno@gmail.com>]

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á