[PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[Oleksij Rempel]

On one side we have indio_dev->num_channels includes all physical channels +
timestamp channel. On other side we have an array allocated only for
physical channels. So, fix memory corruption by ARRAY_SIZE() instead of
num_channels variable.

Fixes: 9374e8f5a38d ("iio: adc: add ADC driver for the TI TSC2046 controller")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/iio/adc/ti-tsc2046.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
index 91f6bd5effe7..8126084616e6 100644
--- a/drivers/iio/adc/ti-tsc2046.c
+++ b/drivers/iio/adc/ti-tsc2046.c
@@ -395,7 +395,7 @@ static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
 	mutex_lock(&priv->slock);
 
 	size = 0;
-	for_each_set_bit(ch_idx, active_scan_mask, indio_dev->num_channels) {
+	for_each_set_bit(ch_idx, active_scan_mask, ARRAY_SIZE(priv->l)) {
 		size += tsc2046_adc_group_set_layout(priv, group, ch_idx);
 		tsc2046_adc_group_set_cmd(priv, group, ch_idx);
 		group++;
@@ -561,7 +561,7 @@ static int tsc2046_adc_setup_spi_msg(struct tsc2046_adc_priv *priv)
 	 * enabled.
 	 */
 	size = 0;
-	for (ch_idx = 0; ch_idx < priv->dcfg->num_channels; ch_idx++)
+	for (ch_idx = 0; ch_idx < ARRAY_SIZE(priv->l); ch_idx++)
 		size += tsc2046_adc_group_set_layout(priv, ch_idx, ch_idx);
 
 	priv->tx = devm_kzalloc(&priv->spi->dev, size, GFP_KERNEL);
-- 
2.30.2

Re: [PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[Jonathan Cameron]

On Fri,  7 Jan 2022 09:14:01 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On one side we have indio_dev->num_channels includes all physical channels +
> timestamp channel. On other side we have an array allocated only for
> physical channels. So, fix memory corruption by ARRAY_SIZE() instead of
> num_channels variable.
> 
> Fixes: 9374e8f5a38d ("iio: adc: add ADC driver for the TI TSC2046 controller")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Hi Olesij,

Have you managed to make this occur, or is it inspection only?

I 'think' (it's been a while since I looked at the particular code) that the timestamp
bit in active_scan_mask will never actually be set because we handle that as a
separate flag.

So it is indeed an efficiency improvement to not check that bit but I don't think
it's a bug to do so.  More than possible I'm missing something though!

This one had me quite worried when I first read it because this is a very common
pattern to see in IIO drivers.

Jonathan

> ---
>  drivers/iio/adc/ti-tsc2046.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-tsc2046.c b/drivers/iio/adc/ti-tsc2046.c
> index 91f6bd5effe7..8126084616e6 100644
> --- a/drivers/iio/adc/ti-tsc2046.c
> +++ b/drivers/iio/adc/ti-tsc2046.c
> @@ -395,7 +395,7 @@ static int tsc2046_adc_update_scan_mode(struct iio_dev *indio_dev,
>  	mutex_lock(&priv->slock);
>  
>  	size = 0;
> -	for_each_set_bit(ch_idx, active_scan_mask, indio_dev->num_channels) {
> +	for_each_set_bit(ch_idx, active_scan_mask, ARRAY_SIZE(priv->l)) {
>  		size += tsc2046_adc_group_set_layout(priv, group, ch_idx);
>  		tsc2046_adc_group_set_cmd(priv, group, ch_idx);
>  		group++;
> @@ -561,7 +561,7 @@ static int tsc2046_adc_setup_spi_msg(struct tsc2046_adc_priv *priv)
>  	 * enabled.
>  	 */
>  	size = 0;
> -	for (ch_idx = 0; ch_idx < priv->dcfg->num_channels; ch_idx++)
> +	for (ch_idx = 0; ch_idx < ARRAY_SIZE(priv->l); ch_idx++)
>  		size += tsc2046_adc_group_set_layout(priv, ch_idx, ch_idx);
>  
>  	priv->tx = devm_kzalloc(&priv->spi->dev, size, GFP_KERNEL);

Re: [PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[Oleksij Rempel]

Hi Jonathan,

On Sun, Jan 09, 2022 at 03:25:57PM +0000, Jonathan Cameron wrote:
> On Fri,  7 Jan 2022 09:14:01 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > On one side we have indio_dev->num_channels includes all physical channels +
> > timestamp channel. On other side we have an array allocated only for
> > physical channels. So, fix memory corruption by ARRAY_SIZE() instead of
> > num_channels variable.
> > 
> > Fixes: 9374e8f5a38d ("iio: adc: add ADC driver for the TI TSC2046 controller")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Hi Olesij,
> 
> Have you managed to make this occur, or is it inspection only?

Yes, this bug has eaten my rx_one and tx_one pointers on probe. I wonted
to use this buffers for read_raw and noticed that they do not exist.

> I 'think' (it's been a while since I looked at the particular code) that the timestamp
> bit in active_scan_mask will never actually be set because we handle that as a
> separate flag.

I didn't tested if active_scan_mask will trigger this issue as well, but
It it looked safer to me, to avoid this issue in both places. Even if on
of it is only theoretical.

> So it is indeed an efficiency improvement to not check that bit but I don't think
> it's a bug to do so.  More than possible I'm missing something though!
> 
> This one had me quite worried when I first read it because this is a very common
> pattern to see in IIO drivers.

I was thinking about this as well, because big part of this code was
inspired by other drivers. But i didn't reviewed other places so far.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[Jonathan Cameron]

On Mon, 10 Jan 2022 08:19:45 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Jonathan,
> 
> On Sun, Jan 09, 2022 at 03:25:57PM +0000, Jonathan Cameron wrote:
> > On Fri,  7 Jan 2022 09:14:01 +0100
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >   
> > > On one side we have indio_dev->num_channels includes all physical channels +
> > > timestamp channel. On other side we have an array allocated only for
> > > physical channels. So, fix memory corruption by ARRAY_SIZE() instead of
> > > num_channels variable.
> > > 
> > > Fixes: 9374e8f5a38d ("iio: adc: add ADC driver for the TI TSC2046 controller")
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>  
> > Hi Olesij,
> > 
> > Have you managed to make this occur, or is it inspection only?  
> 
> Yes, this bug has eaten my rx_one and tx_one pointers on probe. I wonted
> to use this buffers for read_raw and noticed that they do not exist.

I got hung up on the first case and failed to notice the second one was
entirely different :(

> 
> > I 'think' (it's been a while since I looked at the particular code) that the timestamp
> > bit in active_scan_mask will never actually be set because we handle that as a
> > separate flag.  
> 
> I didn't tested if active_scan_mask will trigger this issue as well, but
> It it looked safer to me, to avoid this issue in both places. Even if on
> of it is only theoretical.

It certainly does no harm to not check a bit that is never set, so I'm fine with
the change - just don't want to have lots of 'fixes' for this in other drivers
adding noise and pointless backports.  This one is fine because we need the
other part of the patch anyway.

Jonathan


> 
> > So it is indeed an efficiency improvement to not check that bit but I don't think
> > it's a bug to do so.  More than possible I'm missing something though!
> > 
> > This one had me quite worried when I first read it because this is a very common
> > pattern to see in IIO drivers.  
> 
> I was thinking about this as well, because big part of this code was
> inspired by other drivers. But i didn't reviewed other places so far.
> 
> Regards,
> Oleksij

Re: [PATCH v1 1/1] iio: adc: tsc2046: fix memory corruption by preventing array overflow

[Jonathan Cameron]

On Sat, 15 Jan 2022 18:16:59 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 10 Jan 2022 08:19:45 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > Hi Jonathan,
> > 
> > On Sun, Jan 09, 2022 at 03:25:57PM +0000, Jonathan Cameron wrote:  
> > > On Fri,  7 Jan 2022 09:14:01 +0100
> > > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > >     
> > > > On one side we have indio_dev->num_channels includes all physical channels +
> > > > timestamp channel. On other side we have an array allocated only for
> > > > physical channels. So, fix memory corruption by ARRAY_SIZE() instead of
> > > > num_channels variable.
> > > > 
> > > > Fixes: 9374e8f5a38d ("iio: adc: add ADC driver for the TI TSC2046 controller")
> > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>    
> > > Hi Olesij,
> > > 
> > > Have you managed to make this occur, or is it inspection only?    
> > 
> > Yes, this bug has eaten my rx_one and tx_one pointers on probe. I wonted
> > to use this buffers for read_raw and noticed that they do not exist.  
> 
> I got hung up on the first case and failed to notice the second one was
> entirely different :(
> 
> >   
> > > I 'think' (it's been a while since I looked at the particular code) that the timestamp
> > > bit in active_scan_mask will never actually be set because we handle that as a
> > > separate flag.    
> > 
> > I didn't tested if active_scan_mask will trigger this issue as well, but
> > It it looked safer to me, to avoid this issue in both places. Even if on
> > of it is only theoretical.  
> 
> It certainly does no harm to not check a bit that is never set, so I'm fine with
> the change - just don't want to have lots of 'fixes' for this in other drivers
> adding noise and pointless backports.  This one is fine because we need the
> other part of the patch anyway.
> 
> Jonathan

On that note, applied to the fixes-togreg branch of iio.git and marked for stable
with a minor addition to the patch description to make
sure we don't get the first case cargo picked up by anyone who doesn't read this
thread as something to 'fix' everywhere else.

Thanks,

Jonathan

> 
> 
> >   
> > > So it is indeed an efficiency improvement to not check that bit but I don't think
> > > it's a bug to do so.  More than possible I'm missing something though!
> > > 
> > > This one had me quite worried when I first read it because this is a very common
> > > pattern to see in IIO drivers.    
> > 
> > I was thinking about this as well, because big part of this code was
> > inspired by other drivers. But i didn't reviewed other places so far.
> > 
> > Regards,
> > Oleksij  
>