* Re: [PATCH RFC] iio: irsd200: fix -Warray-bounds bug in irsd200_trigger_handler
2023-08-09 8:37 ` Jonathan Cameron
@ 2023-08-09 17:13 ` Jonathan Cameron
2023-08-21 12:52 ` Waqar Hameed
1 sibling, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2023-08-09 17:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Gustavo A. R. Silva, GONG, Ruiqi, Lars-Peter Clausen,
Waqar Hameed, Kees Cook, linux-iio, linux-kernel, linux-hardening,
Wang Weiyang, Xiu Jianfeng, gongruiqi1
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
> > >
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH RFC] iio: irsd200: fix -Warray-bounds bug in irsd200_trigger_handler
2023-08-09 8:37 ` Jonathan Cameron
2023-08-09 17:13 ` Jonathan Cameron
@ 2023-08-21 12:52 ` Waqar Hameed
1 sibling, 0 replies; 4+ messages in thread
From: Waqar Hameed @ 2023-08-21 12:52 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Gustavo A. R. Silva, GONG, Ruiqi, Jonathan Cameron,
Lars-Peter Clausen, Kees Cook, linux-iio, linux-kernel,
linux-hardening, Wang Weiyang, Xiu Jianfeng, gongruiqi1
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.
^ permalink raw reply [flat|nested] 4+ messages in thread