Use PAGE_NOACCESS for guard pages in Windows.

Use PAGE_NOACCESS for guard pages in Windows.

Up until now we used PAGE_GUARD for guard pages in Windows, which
will raise a STATUS_GUARD_PAGE_VIOLATION exception on first access
and grant regular access afterwards. This behavior is required to
implement automatic stack checking, or more generally to implement
applications that monitor the growth of large dynamic data structures.

However, this is not what we want for our guard pages, which are
used as a security mechanism. What we really want is PAGE_NOACCESS
here, which is the Windows-equivalent of PROT_NONE that we use on
all other platforms.

R=cdn@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16604

[Benedikt Meurer, 2013-09-05 08:39:37]

Hey Michi,
Here's the change we discussed offline to use PAGE_NOACCESS on Windows, PTAL.
-- Benedikt

[Vyacheslav Egorov (Google), 2013-09-05 08:51:05]

+cdn

Original OS::Guard was submitted by Cris Neckar. I always assumed that choice of
PROT_GUARD was intentional.

[Michael Starzinger, 2013-09-05 09:22:12]

Looking OK from my end. I'll leave the final decision up to Cris.

[Cris Neckar, 2013-09-06 17:48:46]

On 2013/09/05 09:22:12, Michael Starzinger wrote:
> Looking OK from my end. I'll leave the final decision up to Cris.

Yes, the reason we used PAGE_GUARD is so that we would get an actionable
exception in crash reports. This allows us to filter crashes for
STATUS_GUARD_PAGE_VIOLATION and know with some degree of certainty that a given
crash will have security relevance. Changing this to PAGE_NOACCESS would mean we
will simply get a standard ACCESS_VIOLATION and will not have the additional
context that we are writing off the end (or before the beginning of a guarded
segment). 

That being said are their cases that you found where we are handling this
exception and it is not causing a crash? If so then I agree this is bad and we
need to fix it. Otherwise I would ask that we keep these pages with the current
protection so that we retain the additional context in crash reports.

Make sense?

[Benedikt Meurer, 2013-09-09 05:38:48]

On 2013/09/06 17:48:46, Cris Neckar wrote:
> On 2013/09/05 09:22:12, Michael Starzinger wrote:
> > Looking OK from my end. I'll leave the final decision up to Cris.
> 
> Yes, the reason we used PAGE_GUARD is so that we would get an actionable
> exception in crash reports. This allows us to filter crashes for
> STATUS_GUARD_PAGE_VIOLATION and know with some degree of certainty that a
given
> crash will have security relevance. Changing this to PAGE_NOACCESS would mean
we
> will simply get a standard ACCESS_VIOLATION and will not have the additional
> context that we are writing off the end (or before the beginning of a guarded
> segment). 
> 
> That being said are their cases that you found where we are handling this
> exception and it is not causing a crash? If so then I agree this is bad and we
> need to fix it. Otherwise I would ask that we keep these pages with the
current
> protection so that we retain the additional context in crash reports.
> 
> Make sense?

No, from what I know we don't handle this exception. My point is that we should
not misuse the PAGE_GUARD mechanism here. If someone embedder (or even Chrome)
uses PAGE_GUARD in the way it was designed to, he'll have a hard time figuring
out what's going on when the exception handler is entered due to an (accidential
exploit/bug) in V8. I discovered this issue during general code review, but if
we run into such an issue in 1-2 years noone will think of this and we'll have
to trace that down for several days (it's totally non-obvious). Especially since
this issue is Windows-only then, because all other platforms use PROT_NONE here.

So I vote to change this to PAGE_NOACCESS or even better, just uncommit the
guard pages. They are easy to spot already.

[Benedikt Meurer, 2013-09-09 05:39:25]

CC'ing Sven for additional feedback

[Sven Panne, 2013-09-09 07:40:39]

Hmmm, actually I would prefer PAGE_NOACCESS, too. MSDN explicitly describes
PAGE_GUARD as a mechanism for growable data structures, e.g.
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366549(v=vs.85).aspx.
Therefore I think it is a terrible idea to use this for security, this is
exactly what PAGE_NOACCESS is for. It is very weird that we depend on the
embedder's handling of PAGE_GUARD, consider e.g. using v8 from another language
implementation (Haskell, OCaml, etc.) where stacks grown on demand via
PAGE_GUARD. Furthermore I would really like to know how much information we
actually gather from the crash dumps because of PAGE_GUARD: I am not aware of
any bug report/issue mentioning this in the last years. I doubt that the trouble
of a weird use of page flags is paying off, it can only detect a tiny fraction
of bugs/security issues, and I am not convinced why these kind of issues should
be more important than others.

[Cris Neckar, 2013-09-09 20:31:19]

On 2013/09/09 07:40:39, Sven Panne wrote:
> Hmmm, actually I would prefer PAGE_NOACCESS, too. MSDN explicitly describes
> PAGE_GUARD as a mechanism for growable data structures, e.g.
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366549%28v=vs.85%29....
> Therefore I think it is a terrible idea to use this for security, this is
> exactly what PAGE_NOACCESS is for. It is very weird that we depend on the
> embedder's handling of PAGE_GUARD, consider e.g. using v8 from another
language
> implementation (Haskell, OCaml, etc.) where stacks grown on demand via
> PAGE_GUARD. Furthermore I would really like to know how much information we
> actually gather from the crash dumps because of PAGE_GUARD: I am not aware of
> any bug report/issue mentioning this in the last years. I doubt that the
trouble
> of a weird use of page flags is paying off, it can only detect a tiny fraction
> of bugs/security issues, and I am not convinced why these kind of issues
should
> be more important than others.

SOunds good. I am fine with changing this to PAGE_NOACCESS. Thanks guys

[Cris Neckar, 2013-09-09 20:31:53]

lgtm

[Benedikt Meurer, 2013-09-10 05:54:29]

Committed patchset #1 manually as r16604 (presubmit successful).