Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 12 Mar 2018 21:05:36 -0700
From: Kees Cook <>
To: "Tobin C. Harding" <>
Cc: "Gustavo A. R. Silva" <>, Tycho Andersen <>, 
	Kernel Hardening <>
Subject: Re: VLA commit log

On Sun, Mar 11, 2018 at 10:44 PM, Tobin C. Harding <> 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

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


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


What do you think?


Kees Cook
Pixel Security

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.