Re: [PATCH RFC] iio: irsd200: fix -Warray-bounds bug in irsd200_trigger_handler

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 8 Aug 2023 05:10:04 -0600
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> On Tue, Aug 08, 2023 at 04:37:19PM +0800, GONG, Ruiqi wrote:
> > From: "GONG, Ruiqi" <gongruiqi1@huawei.com>
> > 
> > When compiling with gcc 13 with -Warray-bounds enabled:
> > 
> > In file included from drivers/iio/proximity/irsd200.c:15:
> > In function ‘iio_push_to_buffers_with_timestamp’,
> >     inlined from ‘irsd200_trigger_handler’ at drivers/iio/proximity/irsd200.c:770:2:
> > ./include/linux/iio/buffer.h:42:46: error: array subscript ‘int64_t {aka long long int}[0]’
> > is partly outside array bounds of ‘s16[1]’ {aka ‘short int[1]’} [-Werror=array-bounds=]
> >    42 |                 ((int64_t *)data)[ts_offset] = timestamp;
> >       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > drivers/iio/proximity/irsd200.c: In function ‘irsd200_trigger_handler’:
> > drivers/iio/proximity/irsd200.c:763:13: note: object ‘buf’ of size 2
> >   763 |         s16 buf = 0;
> >       |             ^~~
> > 
> > The problem seems to be that irsd200_trigger_handler() is taking a s16
> > variable as an int64_t buffer. Fix it by extending the buffer to 64 bits.  
> 
> Thanks for working on this!
> 
> > 
> > Link: https://github.com/KSPP/linux/issues/331
> > Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>  
> 
> Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Good find on the bug, but the fix is wrong even if it squashes the error.

> 
> --
> Gustavo
> 
> > ---
> > 
> > RFC: It's a preliminary patch since I'm not familiar with this hardware.
> > Further comments/reviews are needed about whether this fix is correct,
> > or we should use iio_push_to_buffers() instead of the *_with_timestamp()
> > version.
> > 
> >  drivers/iio/proximity/irsd200.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> > index 5bd791b46d98..34c479881bdf 100644
> > --- a/drivers/iio/proximity/irsd200.c
> > +++ b/drivers/iio/proximity/irsd200.c
> > @@ -759,10 +759,10 @@ static irqreturn_t irsd200_trigger_handler(int irq, void *pollf)
> >  {
> >  	struct iio_dev *indio_dev = ((struct iio_poll_func *)pollf)->indio_dev;
> >  	struct irsd200_data *data = iio_priv(indio_dev);
> > -	s16 buf = 0;
> > +	int64_t buf = 0;

s64 as internal kernel type.
More importantly needs to be at least s64 buf[2]; as the offset
https://elixir.bootlin.com/linux/latest/source/include/linux/iio/buffer.h#L41
will be 1 due to this filling the timestamp in at first 8 byte aligned location
after the data that is already in the buffer.

With hindsight was a bad decision a long time ago not to force people to also
pass the size into this function so we could detect this at runtime at least.
Hard to repair now give very large number of drivers using this and the fact
that it's not always easy to work out that size.  Unfortunately occasionally
one of these slips through review :(

I suppose we could, in some cases check if the buffer was at least 16 bytes which
would get us some of the way.

Jonathan

> >  	int ret;
> >  
> > -	ret = irsd200_read_data(data, &buf);
> > +	ret = irsd200_read_data(data, (s16 *)&buf);
> >  	if (ret)
> >  		goto end;
> >  
> > -- 
> > 2.41.0
> >