| Message ID | 20241028142357.1032380-1-quzicheng@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ad7923: fix array out of bounds in ad7923_update_scan_mode() | expand |
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;
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á
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;
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(+)