* [PATCH] iio: potentiometer: ds1803: remove VLA usage
@ 2018-03-13 15:23 Gustavo A. R. Silva
2018-03-13 16:24 ` Himanshu Jha
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-13 15:23 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler
Cc: linux-iio, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular
case use macro ARRAY_SIZE so the length of array _result_ can be
computed at preprocessing time.
The use of stack Variable Length Arrays needs to be avoided, as they
can be a vector for stack exhaustion, which can be both a runtime bug
or a security flaw. Also, in general, as code evolves it is easy to
lose track of how big a VLA can get. Thus, we can end up having runtime
failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from
the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/iio/potentiometer/ds1803.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index 9b0ff4a..6bf12c9 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
struct ds1803_data *data = iio_priv(indio_dev);
int pot = chan->channel;
int ret;
- u8 result[indio_dev->num_channels];
+ u8 result[ARRAY_SIZE(ds1803_channels)];
switch (mask) {
case IIO_CHAN_INFO_RAW:
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] iio: potentiometer: ds1803: remove VLA usage
2018-03-13 15:23 [PATCH] iio: potentiometer: ds1803: remove VLA usage Gustavo A. R. Silva
@ 2018-03-13 16:24 ` Himanshu Jha
2018-03-13 16:31 ` Gustavo A. R. Silva
0 siblings, 1 reply; 10+ messages in thread
From: Himanshu Jha @ 2018-03-13 16:24 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel
Hi Gustavo,
On Tue, Mar 13, 2018 at 10:23:43AM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wvla, remove VLA. In this particular
> case use macro ARRAY_SIZE so the length of array _result_ can be
> computed at preprocessing time.
>
> The use of stack Variable Length Arrays needs to be avoided, as they
> can be a vector for stack exhaustion, which can be both a runtime bug
> or a security flaw. Also, in general, as code evolves it is easy to
> lose track of how big a VLA can get. Thus, we can end up having runtime
> failures that are hard to debug.
>
> Also, fixed as part of the directive to remove all VLAs from
> the kernel: https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
It is already applied as I had sent the patch few days ago.
https://lkml.org/lkml/2018/3/10/164
I specifically CC'ed you and Kees to avoid the patch collisions.
--
Thanks
Himanshu Jha
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: potentiometer: ds1803: remove VLA usage
2018-03-13 16:24 ` Himanshu Jha
@ 2018-03-13 16:31 ` Gustavo A. R. Silva
2018-03-13 16:59 ` Himanshu Jha
0 siblings, 1 reply; 10+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-13 16:31 UTC (permalink / raw)
To: Himanshu Jha
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel
On 03/13/2018 11:24 AM, Himanshu Jha wrote:
> Hi Gustavo,
>
> On Tue, Mar 13, 2018 at 10:23:43AM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wvla, remove VLA. In this particular
>> case use macro ARRAY_SIZE so the length of array _result_ can be
>> computed at preprocessing time.
>>
>> The use of stack Variable Length Arrays needs to be avoided, as they
>> can be a vector for stack exhaustion, which can be both a runtime bug
>> or a security flaw. Also, in general, as code evolves it is easy to
>> lose track of how big a VLA can get. Thus, we can end up having runtime
>> failures that are hard to debug.
>>
>> Also, fixed as part of the directive to remove all VLAs from
>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>
> It is already applied as I had sent the patch few days ago.
> https://lkml.org/lkml/2018/3/10/164
>
> I specifically CC'ed you and Kees to avoid the patch collisions.
>
I see. Can you please update this spreadsheet:
https://docs.google.com/spreadsheets/d/1OcfyKK8pJ24esYhSEsW4Q2boZE7UTGbYsSEEtFXf7U0/edit
Thanks
--
Gustavo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: potentiometer: ds1803: remove VLA usage
2018-03-13 16:31 ` Gustavo A. R. Silva
@ 2018-03-13 16:59 ` Himanshu Jha
2018-03-13 17:05 ` Gustavo A. R. Silva
2018-03-13 17:07 ` Gustavo A. R. Silva
0 siblings, 2 replies; 10+ messages in thread
From: Himanshu Jha @ 2018-03-13 16:59 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel
On Tue, Mar 13, 2018 at 11:31:19AM -0500, Gustavo A. R. Silva wrote:
>
>
> On 03/13/2018 11:24 AM, Himanshu Jha wrote:
> >Hi Gustavo,
> >
> >On Tue, Mar 13, 2018 at 10:23:43AM -0500, Gustavo A. R. Silva wrote:
> >>In preparation to enabling -Wvla, remove VLA. In this particular
> >>case use macro ARRAY_SIZE so the length of array _result_ can be
> >>computed at preprocessing time.
> >>
> >>The use of stack Variable Length Arrays needs to be avoided, as they
> >>can be a vector for stack exhaustion, which can be both a runtime bug
> >>or a security flaw. Also, in general, as code evolves it is easy to
> >>lose track of how big a VLA can get. Thus, we can end up having runtime
> >>failures that are hard to debug.
> >>
> >>Also, fixed as part of the directive to remove all VLAs from
> >>the kernel: https://lkml.org/lkml/2018/3/7/621
> >>
> >>Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >>---
> >
> >It is already applied as I had sent the patch few days ago.
> >https://lkml.org/lkml/2018/3/10/164
> >
> >I specifically CC'ed you and Kees to avoid the patch collisions.
> >
>
> I see. Can you please update this spreadsheet:
>
> https://docs.google.com/spreadsheets/d/1OcfyKK8pJ24esYhSEsW4Q2boZE7UTGbYsSEEtFXf7U0/edit
Updated!
Also,
drivers/iio/humidity/hts221_i2c.c:43:2: warning: ISO C90
forbids variable length array ‘send’ [-Wvla]
This was already removed in recent commit when regmap API was used.
"6217792 iio: humidity: hts221: add regmap API support"
For this I added a short note in the *Notes* column.
--
Thanks
Himanshu Jha
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: potentiometer: ds1803: remove VLA usage
2018-03-13 16:59 ` Himanshu Jha
@ 2018-03-13 17:05 ` Gustavo A. R. Silva
2018-03-13 17:07 ` Gustavo A. R. Silva
1 sibling, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-13 17:05 UTC (permalink / raw)
To: Himanshu Jha
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel
On 03/13/2018 11:59 AM, Himanshu Jha wrote:
> On Tue, Mar 13, 2018 at 11:31:19AM -0500, Gustavo A. R. Silva wrote:
>>
>>
>> On 03/13/2018 11:24 AM, Himanshu Jha wrote:
>>> Hi Gustavo,
>>>
>>> On Tue, Mar 13, 2018 at 10:23:43AM -0500, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wvla, remove VLA. In this particular
>>>> case use macro ARRAY_SIZE so the length of array _result_ can be
>>>> computed at preprocessing time.
>>>>
>>>> The use of stack Variable Length Arrays needs to be avoided, as they
>>>> can be a vector for stack exhaustion, which can be both a runtime bug
>>>> or a security flaw. Also, in general, as code evolves it is easy to
>>>> lose track of how big a VLA can get. Thus, we can end up having runtime
>>>> failures that are hard to debug.
>>>>
>>>> Also, fixed as part of the directive to remove all VLAs from
>>>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>>>
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>
>>> It is already applied as I had sent the patch few days ago.
>>> https://lkml.org/lkml/2018/3/10/164
>>>
>>> I specifically CC'ed you and Kees to avoid the patch collisions.
>>>
>>
>> I see. Can you please update this spreadsheet:
>>
>> https://docs.google.com/spreadsheets/d/1OcfyKK8pJ24esYhSEsW4Q2boZE7UTGbYsSEEtFXf7U0/edit
>
> Updated!
>
> Also,
>
> drivers/iio/humidity/hts221_i2c.c:43:2: warning: ISO C90
> forbids variable length array ‘send’ [-Wvla]
>
> This was already removed in recent commit when regmap API was used.
>
> "6217792 iio: humidity: hts221: add regmap API support"
>
> For this I added a short note in the *Notes* column.
>
Awesome.
Thank you
--
Gustavo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] iio: potentiometer: ds1803: remove VLA usage
2018-03-13 16:59 ` Himanshu Jha
2018-03-13 17:05 ` Gustavo A. R. Silva
@ 2018-03-13 17:07 ` Gustavo A. R. Silva
1 sibling, 0 replies; 10+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-13 17:07 UTC (permalink / raw)
To: Himanshu Jha
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, linux-kernel
On 03/13/2018 11:59 AM, Himanshu Jha wrote:
> On Tue, Mar 13, 2018 at 11:31:19AM -0500, Gustavo A. R. Silva wrote:
>>
>>
>> On 03/13/2018 11:24 AM, Himanshu Jha wrote:
>>> Hi Gustavo,
>>>
>>> On Tue, Mar 13, 2018 at 10:23:43AM -0500, Gustavo A. R. Silva wrote:
>>>> In preparation to enabling -Wvla, remove VLA. In this particular
>>>> case use macro ARRAY_SIZE so the length of array _result_ can be
>>>> computed at preprocessing time.
>>>>
>>>> The use of stack Variable Length Arrays needs to be avoided, as they
>>>> can be a vector for stack exhaustion, which can be both a runtime bug
>>>> or a security flaw. Also, in general, as code evolves it is easy to
>>>> lose track of how big a VLA can get. Thus, we can end up having runtime
>>>> failures that are hard to debug.
>>>>
>>>> Also, fixed as part of the directive to remove all VLAs from
>>>> the kernel: https://lkml.org/lkml/2018/3/7/621
>>>>
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>
>>> It is already applied as I had sent the patch few days ago.
>>> https://lkml.org/lkml/2018/3/10/164
>>>
>>> I specifically CC'ed you and Kees to avoid the patch collisions.
>>>
>>
>> I see. Can you please update this spreadsheet:
>>
>> https://docs.google.com/spreadsheets/d/1OcfyKK8pJ24esYhSEsW4Q2boZE7UTGbYsSEEtFXf7U0/edit
>
> Updated!
>
> Also,
>
> drivers/iio/humidity/hts221_i2c.c:43:2: warning: ISO C90
> forbids variable length array ‘send’ [-Wvla]
>
> This was already removed in recent commit when regmap API was used.
>
> "6217792 iio: humidity: hts221: add regmap API support"
>
> For this I added a short note in the *Notes* column.
>
Awesome.
Thank you
--
Gustavo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] iio: potentiometer: ds1803: Remove VLA usage
@ 2018-03-08 18:45 Himanshu Jha
2018-03-08 19:39 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Himanshu Jha @ 2018-03-08 18:45 UTC (permalink / raw)
To: jic23
Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, garsilva,
keescook, Himanshu Jha
In preparation to enabling -Wvla, remove VLA usage and replace it
with fixed a fixed length array and therefore, prevent potential
stack overflow attacks.
Fixed as a part of the discussion to remove all VLAs from the kernel:
https://lkml.org/lkml/2018/3/7/621
Cc: keescook@chromium.org
Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
drivers/iio/potentiometer/ds1803.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
index 9b0ff4a..6bf12c9 100644
--- a/drivers/iio/potentiometer/ds1803.c
+++ b/drivers/iio/potentiometer/ds1803.c
@@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
struct ds1803_data *data = iio_priv(indio_dev);
int pot = chan->channel;
int ret;
- u8 result[indio_dev->num_channels];
+ u8 result[ARRAY_SIZE(ds1803_channels)];
switch (mask) {
case IIO_CHAN_INFO_RAW:
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] iio: potentiometer: ds1803: Remove VLA usage
2018-03-08 18:45 [PATCH] iio: potentiometer: ds1803: Remove " Himanshu Jha
@ 2018-03-08 19:39 ` Kees Cook
2018-03-09 11:05 ` Himanshu Jha
0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2018-03-08 19:39 UTC (permalink / raw)
To: Himanshu Jha
Cc: jic23, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, LKML, Gustavo A. R. Silva
On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha
<himanshujha199640@gmail.com> wrote:
> In preparation to enabling -Wvla, remove VLA usage and replace it
> with fixed a fixed length array and therefore, prevent potential
> stack overflow attacks.
>
> Fixed as a part of the discussion to remove all VLAs from the kernel:
> https://lkml.org/lkml/2018/3/7/621
>
> Cc: keescook@chromium.org
> Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> ---
> drivers/iio/potentiometer/ds1803.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
> index 9b0ff4a..6bf12c9 100644
> --- a/drivers/iio/potentiometer/ds1803.c
> +++ b/drivers/iio/potentiometer/ds1803.c
> @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
> struct ds1803_data *data = iio_priv(indio_dev);
> int pot = chan->channel;
> int ret;
> - u8 result[indio_dev->num_channels];
> + u8 result[ARRAY_SIZE(ds1803_channels)];
It seems like num_channels is always ARRAY_SIZE(ds1803_channels).
Could the entire field be dropped?
-Kees
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] iio: potentiometer: ds1803: Remove VLA usage
2018-03-08 19:39 ` Kees Cook
@ 2018-03-09 11:05 ` Himanshu Jha
2018-03-10 15:04 ` Jonathan Cameron
0 siblings, 1 reply; 10+ messages in thread
From: Himanshu Jha @ 2018-03-09 11:05 UTC (permalink / raw)
To: Kees Cook
Cc: jic23, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, LKML, Gustavo A. R. Silva
On Thu, Mar 08, 2018 at 11:39:15AM -0800, Kees Cook wrote:
> On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha
> <himanshujha199640@gmail.com> wrote:
> > In preparation to enabling -Wvla, remove VLA usage and replace it
> > with fixed a fixed length array and therefore, prevent potential
> > stack overflow attacks.
> >
> > Fixed as a part of the discussion to remove all VLAs from the kernel:
> > https://lkml.org/lkml/2018/3/7/621
> >
> > Cc: keescook@chromium.org
> > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> > ---
> > drivers/iio/potentiometer/ds1803.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
> > index 9b0ff4a..6bf12c9 100644
> > --- a/drivers/iio/potentiometer/ds1803.c
> > +++ b/drivers/iio/potentiometer/ds1803.c
> > @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
> > struct ds1803_data *data = iio_priv(indio_dev);
> > int pot = chan->channel;
> > int ret;
> > - u8 result[indio_dev->num_channels];
> > + u8 result[ARRAY_SIZE(ds1803_channels)];
>
> It seems like num_channels is always ARRAY_SIZE(ds1803_channels).
> Could the entire field be dropped?
If you're asking to remove num_channels then certainly it is not
possible
since it is a member of industrial I/O device struct and it is not just
a member of regular struct local to this file.
We certainly know that there are only two channels BTW and it can be
tranformed to simply:
u8 result[2];
But then I might have to add an additional comment explaining the magic
number 2.
--
Thanks
Himanshu Jha
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] iio: potentiometer: ds1803: Remove VLA usage
2018-03-09 11:05 ` Himanshu Jha
@ 2018-03-10 15:04 ` Jonathan Cameron
0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2018-03-10 15:04 UTC (permalink / raw)
To: Himanshu Jha
Cc: Kees Cook, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, linux-iio, LKML, Gustavo A. R. Silva
On Fri, 9 Mar 2018 16:35:10 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:
> On Thu, Mar 08, 2018 at 11:39:15AM -0800, Kees Cook wrote:
> > On Thu, Mar 8, 2018 at 10:45 AM, Himanshu Jha
> > <himanshujha199640@gmail.com> wrote:
> > > In preparation to enabling -Wvla, remove VLA usage and replace it
> > > with fixed a fixed length array and therefore, prevent potential
> > > stack overflow attacks.
> > >
> > > Fixed as a part of the discussion to remove all VLAs from the kernel:
> > > https://lkml.org/lkml/2018/3/7/621
> > >
> > > Cc: keescook@chromium.org
> > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > ---
> > > drivers/iio/potentiometer/ds1803.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/potentiometer/ds1803.c b/drivers/iio/potentiometer/ds1803.c
> > > index 9b0ff4a..6bf12c9 100644
> > > --- a/drivers/iio/potentiometer/ds1803.c
> > > +++ b/drivers/iio/potentiometer/ds1803.c
> > > @@ -64,7 +64,7 @@ static int ds1803_read_raw(struct iio_dev *indio_dev,
> > > struct ds1803_data *data = iio_priv(indio_dev);
> > > int pot = chan->channel;
> > > int ret;
> > > - u8 result[indio_dev->num_channels];
> > > + u8 result[ARRAY_SIZE(ds1803_channels)];
> >
> > It seems like num_channels is always ARRAY_SIZE(ds1803_channels).
> > Could the entire field be dropped?
>
> If you're asking to remove num_channels then certainly it is not
> possible
> since it is a member of industrial I/O device struct and it is not just
> a member of regular struct local to this file.
>
> We certainly know that there are only two channels BTW and it can be
> tranformed to simply:
>
> u8 result[2];
>
> But then I might have to add an additional comment explaining the magic
> number 2.
I'm happy with the exact version you proposed.
num_channels isn't there for the driver use (as it can obviously know
this) but for the core which uses this to know how big the channel array
is when creating the sysfs interfaces etc.
Applied to the togreg branch of iio.git and pushed out as testing.
Thanks,
Jonathan
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-13 17:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 15:23 [PATCH] iio: potentiometer: ds1803: remove VLA usage Gustavo A. R. Silva
2018-03-13 16:24 ` Himanshu Jha
2018-03-13 16:31 ` Gustavo A. R. Silva
2018-03-13 16:59 ` Himanshu Jha
2018-03-13 17:05 ` Gustavo A. R. Silva
2018-03-13 17:07 ` Gustavo A. R. Silva
-- strict thread matches above, loose matches on Subject: below --
2018-03-08 18:45 [PATCH] iio: potentiometer: ds1803: Remove " Himanshu Jha
2018-03-08 19:39 ` Kees Cook
2018-03-09 11:05 ` Himanshu Jha
2018-03-10 15:04 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.