VLA commit log

VLA commit log

[Tobin C. Harding]

Hi,

I got some push back on the commit log we have all started to use
(copying Kees' initial commit log).  If we are going to do hundreds of
these patches should we write a perfectly correct commit log that can be
included as the start of the 'why' of each VLA removal patch?  Here is
my attempt, I am quite bad at writing commit logs so would love someone
to fix it up.

    Kernel stack size is limited.  Variable Length Arrays (VLA) open the
    kernel up to stack abuse in a couple of ways;
    
    1. If the variable can be controlled by an attacker.
    2. Not having the size of the stack right there in plain site makes it
    harder to maintain the code base because changes in one place can effect
    the stack in another place (i.e in another function).
    
    It would be nice to be able to build the kernel with -Wvla.  There has
    been some consensus on this already [1].

    ...

    [1]: https://lkml.org/lkml/2018/3/7/621

The '...' would of course be different for each patch.  In case you
missed it here is the catalyst for this email

	On Mon, Mar 12, 2018 at 03:49:40PM +1100, Tobin C. Harding wrote:
	> The kernel would like to have all stack VLA usage removed[1].

	Can you please stop writing this?  The Linux kernel isn't
	sentient; it doesn't "like" anything.  You need to explain why
	*you* (and other people) believe these changes should be made.


Perhaps we should add a summary of all the gcc discussion i.e why const
variables still cause gcc to emit a VLA warning.


thanks,
Tobin.

Re: VLA commit log

[Gustavo A. R. Silva]

On 03/12/2018 12:26 AM, Tobin C. Harding wrote:
> Hi,
> 
> I got some push back on the commit log we have all started to use
> (copying Kees' initial commit log).  If we are going to do hundreds of
> these patches should we write a perfectly correct commit log that can be
> included as the start of the 'why' of each VLA removal patch?  Here is
> my attempt, I am quite bad at writing commit logs so would love someone
> to fix it up.
> 

The same thing happened to me once and then I wrote this:

In preparation to enabling -Wvla, remove VLA and replace it
with a fixed-length array instead.

 From a security viewpoint, the use of Variable Length Arrays can be
a vector for stack overflow attacks. Also, in general, as the code
evolves it is easy to lose track of how big a VLA can get. Thus, we
can end up having segfaults 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

The maintainer lived happily ever after. :)

--
Gustavo

>      Kernel stack size is limited.  Variable Length Arrays (VLA) open the
>      kernel up to stack abuse in a couple of ways;
>      
>      1. If the variable can be controlled by an attacker.
>      2. Not having the size of the stack right there in plain site makes it
>      harder to maintain the code base because changes in one place can effect
>      the stack in another place (i.e in another function).
>      
>      It would be nice to be able to build the kernel with -Wvla.  There has
>      been some consensus on this already [1].
> 
>      ...
> 
>      [1]: https://lkml.org/lkml/2018/3/7/621
> 
> The '...' would of course be different for each patch.  In case you
> missed it here is the catalyst for this email
> 
> 	On Mon, Mar 12, 2018 at 03:49:40PM +1100, Tobin C. Harding wrote:
> 	> The kernel would like to have all stack VLA usage removed[1].
> 
> 	Can you please stop writing this?  The Linux kernel isn't
> 	sentient; it doesn't "like" anything.  You need to explain why
> 	*you* (and other people) believe these changes should be made.
> 
> 
> Perhaps we should add a summary of all the gcc discussion i.e why const
> variables still cause gcc to emit a VLA warning.
> 
> 
> thanks,
> Tobin.
> 

Re: VLA commit log

[Tobin C. Harding]

On Mon, Mar 12, 2018 at 12:38:04AM -0500, Gustavo A. R. Silva wrote:
> 
> 
> On 03/12/2018 12:26 AM, Tobin C. Harding wrote:
> >Hi,
> >
> >I got some push back on the commit log we have all started to use
> >(copying Kees' initial commit log).  If we are going to do hundreds of
> >these patches should we write a perfectly correct commit log that can be
> >included as the start of the 'why' of each VLA removal patch?  Here is
> >my attempt, I am quite bad at writing commit logs so would love someone
> >to fix it up.
> >
> 
> The same thing happened to me once and then I wrote this:

I had a feeling this had happened to you but I couldn't find the patch
that made me think that when writing this.

> In preparation to enabling -Wvla, remove VLA and replace it
> with a fixed-length array instead.

---

> From a security viewpoint, the use of Variable Length Arrays can be
> a vector for stack overflow attacks. Also, in general, as the code
> evolves it is easy to lose track of how big a VLA can get. Thus, we
> can end up having segfaults 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

---

Cool, I like this (between ---).  Kees can you ack this since I'm going
to cut and paste it about 300 times :)

> The maintainer lived happily ever after. :)

lol

> --
> Gustavo

thanks,
Tobin.

Re: VLA commit log

[Kees Cook]

On Sun, Mar 11, 2018 at 10:44 PM, Tobin C. Harding <tobin@apporbit.com> wrote:
> On Mon, Mar 12, 2018 at 12:38:04AM -0500, Gustavo A. R. Silva wrote:
>>
>>
>> On 03/12/2018 12:26 AM, Tobin C. Harding wrote:
>> >Hi,
>> >
>> >I got some push back on the commit log we have all started to use
>> >(copying Kees' initial commit log).  If we are going to do hundreds of
>> >these patches should we write a perfectly correct commit log that can be
>> >included as the start of the 'why' of each VLA removal patch?  Here is
>> >my attempt, I am quite bad at writing commit logs so would love someone
>> >to fix it up.
>> >
>>
>> The same thing happened to me once and then I wrote this:
>
> I had a feeling this had happened to you but I couldn't find the patch
> that made me think that when writing this.
>
>> In preparation to enabling -Wvla, remove VLA and replace it
>> with a fixed-length array instead.
>
> ---
>
>> From a security viewpoint, the use of Variable Length Arrays can be

I avoid starting lines with "From" due to age-old mbox-format email
hilarity. Also, if we're going to be pedantic, we should say "stack
VLA".

>> a vector for stack overflow attacks. Also, in general, as the code

Many people confused "stack buffer overflow" with "stack exhaustion".
In this case, we should use the latter terminology.

>> evolves it is easy to lose track of how big a VLA can get. Thus, we
>> can end up having segfaults 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
>
> ---
>
> Cool, I like this (between ---).  Kees can you ack this since I'm going
> to cut and paste it about 300 times :)

We've got two cases we're dealing with:

1) Actual VLA in use, etc.

2) -Wvla warns about a non-constant-expression, but it's a
constant-value ("const int foo = 5"). In this case, it appears no VLA
is generated, but we get the warning. So we're fixing VLA warnings
that aren't real. But we still want the coverage of -Wvla, so we need
to refactor these cases too.


So, for 1, sure:

---
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
(kernel Oops) or a security flaw (overwriting memory beyond the
stack). 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. As part of the directive[1] to remove all VLAs
from the kernel, and build with -Wvla, this patch [does whatever, with
analysis of alternatives, runtime impact, or memory usage].

[1] https://lkml.org/lkml/2018/3/7/621
---

For 2, how about something like this:

---
As part of the directive[1] to remove all VLAs from the kernel, it
would be nice to build with -Wvla. This warning is, however, overly
pessimistic in that it warns about not using constant expressions when
declaring stack array size: it still warns about constant values, even
though those do not appear to generate actual VLAs. This patch adjusts
the stack array declarations to use a constant expression to remove
this false positive by [whatever, with analysys of alternatives, etc].

[1] https://lkml.org/lkml/2018/3/7/621
---


What do you think?

-Kees

-- 
Kees Cook
Pixel Security

Re: VLA commit log

[Salvatore Mesoraca]

2018-03-12 6:26 GMT+01:00 Tobin C. Harding <tobin@apporbit.com>:
> Hi,
>
> I got some push back on the commit log we have all started to use
> (copying Kees' initial commit log).  If we are going to do hundreds of
> these patches should we write a perfectly correct commit log that can be
> included as the start of the 'why' of each VLA removal patch?  Here is
> my attempt, I am quite bad at writing commit logs so would love someone
> to fix it up.
>
>     Kernel stack size is limited.  Variable Length Arrays (VLA) open the
>     kernel up to stack abuse in a couple of ways;
>
>     1. If the variable can be controlled by an attacker.
>     2. Not having the size of the stack right there in plain site makes it
>     harder to maintain the code base because changes in one place can effect
>     the stack in another place (i.e in another function).
>
>     It would be nice to be able to build the kernel with -Wvla.  There has
>     been some consensus on this already [1].
>
>     ...
>
>     [1]: https://lkml.org/lkml/2018/3/7/621
>
> The '...' would of course be different for each patch.  In case you
> missed it here is the catalyst for this email
>
>         On Mon, Mar 12, 2018 at 03:49:40PM +1100, Tobin C. Harding wrote:
>         > The kernel would like to have all stack VLA usage removed[1].
>
>         Can you please stop writing this?  The Linux kernel isn't
>         sentient; it doesn't "like" anything.  You need to explain why
>         *you* (and other people) believe these changes should be made.
>
>
> Perhaps we should add a summary of all the gcc discussion i.e why const
> variables still cause gcc to emit a VLA warning.

Maybe it will be useful to update the doc (e.g.
Documentation/process/coding-style.rst or a new
Documentation/process/vla-considered-harmful.rst) with an extensive
explanation of why VLAs shouldn't be used.
And then we can just refer to that.

Salvatore

Re: VLA commit log

[Tycho Andersen]

On Mon, Mar 12, 2018 at 11:28:19AM +0100, Salvatore Mesoraca wrote:
> 2018-03-12 6:26 GMT+01:00 Tobin C. Harding <tobin@apporbit.com>:
> > Hi,
> >
> > I got some push back on the commit log we have all started to use
> > (copying Kees' initial commit log).  If we are going to do hundreds of
> > these patches should we write a perfectly correct commit log that can be
> > included as the start of the 'why' of each VLA removal patch?  Here is
> > my attempt, I am quite bad at writing commit logs so would love someone
> > to fix it up.
> >
> >     Kernel stack size is limited.  Variable Length Arrays (VLA) open the
> >     kernel up to stack abuse in a couple of ways;
> >
> >     1. If the variable can be controlled by an attacker.
> >     2. Not having the size of the stack right there in plain site makes it
> >     harder to maintain the code base because changes in one place can effect
> >     the stack in another place (i.e in another function).
> >
> >     It would be nice to be able to build the kernel with -Wvla.  There has
> >     been some consensus on this already [1].
> >
> >     ...
> >
> >     [1]: https://lkml.org/lkml/2018/3/7/621
> >
> > The '...' would of course be different for each patch.  In case you
> > missed it here is the catalyst for this email
> >
> >         On Mon, Mar 12, 2018 at 03:49:40PM +1100, Tobin C. Harding wrote:
> >         > The kernel would like to have all stack VLA usage removed[1].
> >
> >         Can you please stop writing this?  The Linux kernel isn't
> >         sentient; it doesn't "like" anything.  You need to explain why
> >         *you* (and other people) believe these changes should be made.
> >
> >
> > Perhaps we should add a summary of all the gcc discussion i.e why const
> > variables still cause gcc to emit a VLA warning.
> 
> Maybe it will be useful to update the doc (e.g.
> Documentation/process/coding-style.rst or a new
> Documentation/process/vla-considered-harmful.rst) with an extensive
> explanation of why VLAs shouldn't be used.
> And then we can just refer to that.

This seems like a great idea. Perhaps we can combine Kees' recent
reply + a link to the original Linus mail into something? There's also
a similar thread from about four months ago when I originally started
looking at this that we could grab stuff from.

Cheers,

Tycho

Re: VLA commit log

[Tobin C. Harding]

On Mon, Mar 12, 2018 at 10:53:44PM -0600, Tycho Andersen wrote:
> On Mon, Mar 12, 2018 at 11:28:19AM +0100, Salvatore Mesoraca wrote:
> > 2018-03-12 6:26 GMT+01:00 Tobin C. Harding <tobin@apporbit.com>:
> > > Hi,
> > >
> > > I got some push back on the commit log we have all started to use
> > > (copying Kees' initial commit log).  If we are going to do hundreds of
> > > these patches should we write a perfectly correct commit log that can be
> > > included as the start of the 'why' of each VLA removal patch?  Here is
> > > my attempt, I am quite bad at writing commit logs so would love someone
> > > to fix it up.
> > >
> > >     Kernel stack size is limited.  Variable Length Arrays (VLA) open the
> > >     kernel up to stack abuse in a couple of ways;
> > >
> > >     1. If the variable can be controlled by an attacker.
> > >     2. Not having the size of the stack right there in plain site makes it
> > >     harder to maintain the code base because changes in one place can effect
> > >     the stack in another place (i.e in another function).
> > >
> > >     It would be nice to be able to build the kernel with -Wvla.  There has
> > >     been some consensus on this already [1].
> > >
> > >     ...
> > >
> > >     [1]: https://lkml.org/lkml/2018/3/7/621
> > >
> > > The '...' would of course be different for each patch.  In case you
> > > missed it here is the catalyst for this email
> > >
> > >         On Mon, Mar 12, 2018 at 03:49:40PM +1100, Tobin C. Harding wrote:
> > >         > The kernel would like to have all stack VLA usage removed[1].
> > >
> > >         Can you please stop writing this?  The Linux kernel isn't
> > >         sentient; it doesn't "like" anything.  You need to explain why
> > >         *you* (and other people) believe these changes should be made.
> > >
> > >
> > > Perhaps we should add a summary of all the gcc discussion i.e why const
> > > variables still cause gcc to emit a VLA warning.
> > 
> > Maybe it will be useful to update the doc (e.g.
> > Documentation/process/coding-style.rst or a new
> > Documentation/process/vla-considered-harmful.rst) with an extensive
> > explanation of why VLAs shouldn't be used.
> > And then we can just refer to that.
> 
> This seems like a great idea. Perhaps we can combine Kees' recent
> reply + a link to the original Linus mail into something? There's also
> a similar thread from about four months ago when I originally started
> looking at this that we could grab stuff from.

We should include an explanation of why the warning is misleading and
all the stuff about the front end and back end of the compiler and
which end knows about vla's


	Tobin