Fix overflow error in breakpad for linux

Fix overflow error in breakpad for linux

A computation in the stack unwind algorithm could cause an overflow if a base
pointer read from crashed process is sufficiently close to top of address space.
This causes a memory read that causes the dump thread to crash, resulting in a
failure to generate crash dump. Check fixed to properly detect that this pointer
is greater than actual memory range of current stack.

BUG=None

[kjoswiak]

Does this change look reasonable?

I ran into this issue on x86 nexus android tv device. It was reading off a base
pointer of 0xfffffffe, an invalid pointer that was, due to this issue, not being
detected as such, and causing a crash when we tried to read its equivalent off
copied stack. Since I assume we can't assume anything about validity of stack
values in crashed state, I'm thinking faulting in the dumper is not correct
behavior =)

[Mark Mentovai]

I’ll take a look a little later, but Markus wrote this, so I’ll give him a
chance to comment too.

[markus]

LGTM

https://breakpad.appspot.com/3754003/diff/1/client/linux/dump_writer_common/s...
File client/linux/dump_writer_common/seccomp_unwinder.cc (right):

https://breakpad.appspot.com/3754003/diff/1/client/linux/dump_writer_common/s...
client/linux/dump_writer_common/seccomp_unwinder.cc:48:
thread.stack.memory.data_size - sizeof(bp) ||
Thank you very much for spotting this. Integer overflows are so subtle and very
hard to find. Your change looks like an obvious improvement, as it uses the
correct pattern for performing safe overflow checks.

Having said that, in this particular case, I am not sure I understand which bug
you are trying to fix here. Would you mind elaborating.

"sizeof(bp)" is either 4 or 8. So, in general a really small quantity. An
overflow is only possible, if the thread allocated a stack in the very last page
of addressable memory. But that page is always used for the VDSO. So, it should
never be part of the heap. What am I missing?

Apart from this nitpick, I don't mind you applying the CL. No code changes
needed, but maybe an updated summary for the CL. In general, following
well-known safe patterns is always good, as it makes the code more readable
during a security review.

[kjoswiak]

On 2015/02/05 22:34:35, markus wrote:
> LGTM
> 
>
https://breakpad.appspot.com/3754003/diff/1/client/linux/dump_writer_common/s...
> File client/linux/dump_writer_common/seccomp_unwinder.cc (right):
> 
>
https://breakpad.appspot.com/3754003/diff/1/client/linux/dump_writer_common/s...
> client/linux/dump_writer_common/seccomp_unwinder.cc:48:
> thread.stack.memory.data_size - sizeof(bp) ||
> Thank you very much for spotting this. Integer overflows are so subtle and
very
> hard to find. Your change looks like an obvious improvement, as it uses the
> correct pattern for performing safe overflow checks.
> 
> Having said that, in this particular case, I am not sure I understand which
bug
> you are trying to fix here. Would you mind elaborating.
> 
> "sizeof(bp)" is either 4 or 8. So, in general a really small quantity. An
> overflow is only possible, if the thread allocated a stack in the very last
page
> of addressable memory. But that page is always used for the VDSO. So, it
should
> never be part of the heap. What am I missing?
> 
> Apart from this nitpick, I don't mind you applying the CL. No code changes
> needed, but maybe an updated summary for the CL. In general, following
> well-known safe patterns is always good, as it makes the code more readable
> during a security review.

Ya, the 0xfffffffe I was getting was definitely not a valid base pointer,
something must not have been properly structured in the crashed process stack.
Anyway, I reworded to hopefully make this more clear

Also, how do I actually trigger the commit? I don't see the commit checkbox I
see when working with Chromium, do I have to join some group or something to get
permissions?

[Mark Mentovai]

LGTM and I’ll check this in for you. Good catch.

[Mark Mentovai]

Committed r1425.