Checking location of the instruction pointer to see if it is

Checking location of the instruction pointer to see if it is
in valid code for Linux exploitability rating.

This CL adds to the Linux exploitability checker by verifying that the
instruction pointer is in valid code. Verification is done by obtaining a
memory mapping of the crash and checking if the instruction pointer lies in
an executable region. If there is no memory mapping, the instruction pointer
is checked to determine if it lies within a known module.

R=ivanpe@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1464

[liuandrew, 2015-06-24 18:50:43]

liu.andrew.x@gmail.com changed reviewers:
+ mattdr.breakpad@gmail.com

[liuandrew, 2015-06-24 18:51:14]

liu.andrew.x@gmail.com changed reviewers:
+ ivanpe@chromium.org

[liuandrew, 2015-06-24 18:51:50]

liu.andrew.x@gmail.com changed reviewers:
+ cdn@chromium.org

[ivanpe, 2015-06-24 19:14:06]

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:112: default:
Please, add a TODO to handle ARM and arm64.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:118: return EXPLOITABILITY_HIGH;
Please, check with Cris Neckar or other security folks whether this
classification  makes sense.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:145: // approximating if the instruction
pointer is on the stack via
As discussed offline, please remove this check, since it is not necessary.  If
the instruction pointer is in a module it can't be on the stack.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:53: bool InstructionPointerInCode(uint64_t
instruction_ptr, uint64_t stack_ptr);
Please, add a description.  Describe input parameters and return value.

[liuandrew, 2015-06-24 20:59:36]

liu.andrew.x@gmail.com changed reviewers:
- cdn@chromium.org, mattdr.breakpad@gmail.com

[liuandrew, 2015-06-24 20:59:37]

I made the changes discussed earlier today locally, but the changes do not
appear on this issue. `gcl change exploitability` seems to do nothing, and `gcl
upload exploitability` makes a new issue. Do you have any ideas of what I should
be doing?

Mattdr@ did not have a valid email in this CL, and Cris Neckar seems to have
retired, so I've removed both of their emails.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:112: default:
On 2015/06/24 19:14:06, ivanpe wrote:
> Please, add a TODO to handle ARM and arm64.

Done.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:118: return EXPLOITABILITY_HIGH;
On 2015/06/24 19:14:06, ivanpe wrote:
> Please, check with Cris Neckar or other security folks whether this
> classification  makes sense.

Based off an automatic email response that I received from Cris as a result of
sending this CL, it seems that he may have retired.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.cc:145: // approximating if the instruction
pointer is on the stack via
On 2015/06/24 19:14:06, ivanpe wrote:
> As discussed offline, please remove this check, since it is not necessary.  If
> the instruction pointer is in a module it can't be on the stack.

Done.

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
src/processor/exploitability_linux.h:53: bool InstructionPointerInCode(uint64_t
instruction_ptr, uint64_t stack_ptr);
On 2015/06/24 19:14:06, ivanpe wrote:
> Please, add a description.  Describe input parameters and return value.

Done.

[ivanpe, 2015-06-25 02:16:07]

On 2015/06/24 20:59:37, liuandrew wrote:
> I made the changes discussed earlier today locally, but the changes do not
> appear on this issue. `gcl change exploitability` seems to do nothing, and
`gcl
> upload exploitability` makes a new issue. Do you have any ideas of what I
should
> be doing?
> 
> Mattdr@ did not have a valid email in this CL, and Cris Neckar seems to have
> retired, so I've removed both of their emails.
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> File src/processor/exploitability_linux.cc (right):
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> src/processor/exploitability_linux.cc:112: default:
> On 2015/06/24 19:14:06, ivanpe wrote:
> > Please, add a TODO to handle ARM and arm64.
> 
> Done.
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> src/processor/exploitability_linux.cc:118: return EXPLOITABILITY_HIGH;
> On 2015/06/24 19:14:06, ivanpe wrote:
> > Please, check with Cris Neckar or other security folks whether this
> > classification  makes sense.
> 
> Based off an automatic email response that I received from Cris as a result of
> sending this CL, it seems that he may have retired.
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> src/processor/exploitability_linux.cc:145: // approximating if the instruction
> pointer is on the stack via
> On 2015/06/24 19:14:06, ivanpe wrote:
> > As discussed offline, please remove this check, since it is not necessary. 
If
> > the instruction pointer is in a module it can't be on the stack.
> 
> Done.
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> File src/processor/exploitability_linux.h (right):
> 
>
https://codereview.chromium.org/1210493003/diff/1/src/processor/exploitabilit...
> src/processor/exploitability_linux.h:53: bool
InstructionPointerInCode(uint64_t
> instruction_ptr, uint64_t stack_ptr);
> On 2015/06/24 19:14:06, ivanpe wrote:
> > Please, add a description.  Describe input parameters and return value.
> 
> Done.

I'm not sure why it creates a new issue.  As a workaround, you can use flag '-i'
and pass-in your existing issue number when doing the upload.

[liuandrew, 2015-06-25 15:52:54]

Checking location of the instruction pointer to see if it is
in valid code for Linux exploitability rating.

This CL adds to the Linux exploitability checker by verifying that the
instruction pointer is in valid code. Verification is done by obtaining a
memory mapping of the crash and checking if the instruction pointer lies in
an executable region. If there is no memory mapping, the instruction pointer
is checked to determine if it lies within a known module.

[ivanpe, 2015-06-25 20:58:39]

LGTM. Please, address my comments before submitting.

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.cc (right):

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:110: // TODO(liuandrew) support ARM and
arm64 architectures
The style should be:

// TODO(liuandrew): Add support for ARM and arm64 architectures.

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:123: // get memory mapping
same

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:131: // check if the memory mapping at the
instruction pointer is executable
same

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:138: // if the memory mapping retrieval
fails, we will approximate check
same

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.cc:140: // TODO(liuandrew) the entirety of a
module is not necessarily executable
same:

TODO(liuandrew):

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
File src/processor/exploitability_linux.h (right):

https://codereview.chromium.org/1210493003/diff/20001/src/processor/exploitab...
src/processor/exploitability_linux.h:53: // checks if the instruction pointer
lies in a valid instruction region
Please, start sentences with capital letters and finish with dots.

[liuandrew, 2015-06-25 23:05:55]

Committed patchset #2 (id:20001) manually as r1464 (presubmit successful).