Re: VLAs and security

[Kees Cook]

On Sun, Sep 2, 2018 at 1:08 AM, Uecker, Martin
<Martin.Uecker@xxxxxxxxxxxxxxxxxxxxx> wrote:
> I do not agree that VLAs are generally bad for security.
> I think the opposite is true. A VLA with the right size
> allows the compiler to automatically perform or insert
> meaningful bounds checks, while a fixed upper bound does not.

While I see what you mean, the trouble is that the compiler has no
idea what the upper bounds of the _available_ stack is. This means
that a large VLA might allow code to read/write beyond the stack
allocation, which also bypasses the "standard" stack buffer overflow
checks. Additionally, VLAs bypass the existing stack-size checks we've
added to the kernel.

> For example:
>
> char buf[N];
> buf[n] = 1;
>
> Here, a compiler / analysis tool can for  n < N  using
> static analysis or insert a run-time check.
>
> Replacing this with
>
> char buf[MAX_SIZE]
>
> hides the information about the true upper bound
> from automatic tools.

While this may be true for some tools, I don't agree VLAs are better
in general. For example, the compiler actually knows the upper bound
at build time now, and things like the printf format size checks and
CONFIG_FORTIFY_SOURCE are now able to produce compile-time warnings
(since "sizeof(buf)" isn't a runtime value). With a VLA, this is
hidden from those tools, and detection depends on runtime analysis.

It should be noted that VLAs are also slow[1], so removing them not
only improves robustness but also improves performance.

> Limiting the stack usage can also be achieved in
> the following way:
>
> assert(N <= MAX_SIZE)
> char buf[N];

If you look through the various VLA removals that have been landing,
there is a common pattern of performing these checks where it might be
possible for an "n" to be larger than the fixed size. (Many removals
can be compile-time checked as callers are usually just in a specific
range -- it's not really a "runtime" size that was changing, since all
callers used different but hard-coded sizes.)

char buf[N];
...
if (WARN_ON(n > N))
    return -EINVAL;
...

> Of course, having predictable stack usage might be more
> important in the kernel and might be a good argument
> to still prefer the constant bound.

Between improved compile-time checking, faster runtime performance,
and improved robustness against stack exhaustion, I strongly believe
the kernel to be better off with VLAs entirely removed. And we are
close: only 6 remain (out of the 115 I counted in v4.15).

> But loosing the tighter bounds is clearly a disadvantage
> with respect to security that one should keep it mind.

Yes: without VLAs, stack array usage is reduced to "standard" stack
buffer overflow concerns. Removing the VLA doesn't introduce a new
risk: we already had to worry about fixed-size arrays. Removing VLAs
means we don't have to worry about the VLA-specific risks anymore.

-Kees

[1] https://git.kernel.org/linus/02361bc77888

-- 
Kees Cook
Pixel Security