linux-kernel - Re: [PATCH] ad7923: fix array out of bounds in ad7923_update_scan

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1ebd05590051015e6ceb0d2fb4b239ed9959a67.camel@gmail.com>
Date: Tue, 29 Oct 2024 09:40:37 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, Zicheng Qu <quzicheng@...wei.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, patrick.vasseur@....fr, 
	christophe.leroy@....fr, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	tanghui20@...wei.com,
 zhangqiao22@...wei.com, judy.chenhui@...wei.com, Nuno Sá
	 <nuno.sa@...log.com>
Subject: Re: [PATCH] ad7923: fix array out of bounds in
 ad7923_update_scan_mode()

On Mon, 2024-10-28 at 20:50 +0000, Jonathan Cameron wrote:
> On Mon, 28 Oct 2024 14:23:57 +0000
> Zicheng Qu <quzicheng@...wei.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@...r.kernel.org>
> > Signed-off-by: Zicheng Qu <quzicheng@...wei.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á