LKML Archive on lore.kernel.org
 help / Atom feed
From: Kees Cook <keescook@chromium.org>
To: "Uecker, Martin" <Martin.Uecker@med.uni-goettingen.de>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"torvalds@linux-foundation.org" <torvalds@linux-foundation.org>
Subject: Re: VLAs and security
Date: Sun, 2 Sep 2018 10:40:13 -0700
Message-ID: <CAGXu5jJ7kGm-DGUB8uwwhd-Hv7i4WEcSKumTf2MBsj_-yzWcqg@mail.gmail.com> (raw)
In-Reply-To: <1535875700.17858.3.camel@med.uni-goettingen.de>

On Sun, Sep 2, 2018 at 1:08 AM, Uecker, Martin
<Martin.Uecker@med.uni-goettingen.de> 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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-02  8:08 Uecker, Martin
2018-09-02 17:40 ` Kees Cook [this message]
2018-09-03  7:39   ` Uecker, Martin
2018-09-03 21:28     ` Linus Torvalds
2018-09-04  6:27       ` Uecker, Martin
2018-09-04  8:00         ` Dmitry Vyukov
2018-09-04 18:22           ` Uecker, Martin
2018-09-05  7:35             ` Dmitry Vyukov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXu5jJ7kGm-DGUB8uwwhd-Hv7i4WEcSKumTf2MBsj_-yzWcqg@mail.gmail.com \
    --to=keescook@chromium.org \
    --cc=Martin.Uecker@med.uni-goettingen.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox