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

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

[Gustavo A. R. Silva]

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>

--
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;
>  	int ret;
>  
> -	ret = irsd200_read_data(data, &buf);
> +	ret = irsd200_read_data(data, (s16 *)&buf);
>  	if (ret)
>  		goto end;
>  
> -- 
> 2.41.0
> 

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

[Jonathan Cameron]

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
> >   

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

[Jonathan Cameron]

On Wed, 9 Aug 2023 09:37:29 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> 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.
> 
I was going to pick the patch up and modify it, but I think you managed
to send it out as an html email so it didn't reach the mailing list archives.
If you could send a v2 with s64 buf[2]; that would be great.
Due to some travel I need to send a pull request shortly but this won't be in
a release for some time (as pull is targetting 6.6) so not a problem as long
as we make sure to address in soon.

Thanks,

Jonathan

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

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

[Waqar Hameed]

On Wed, Aug 09, 2023 at 09:37 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> 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.

Yeah, this one slipped through!

[...]

>> > 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.

Yes, that would actually be helpful.