Now, if we want to really preserve this check logic under safestack, we will have to do something safestack-specific. There is no way to keep pretending that there is a single, continuous stack region and still get realistic results.
1. Use __builtin_frame_pointer and __builtin___get_unsafe_stack_ptr(). They are supported whenever safestack is supported and can be protected with simple preprocessor guards. 2. Rely on safestack semantics to know which of the two stacks a variable gets allocated on. This is embedding some knowledge about safestack implementation (not just the ABI) into the application, but it relies on the fundamental security promise of safestack and very unlikely to change. For example, this line in my original patch: intptr_t volatile stackaddr = (intptr_t)&which; leaks the address of "which" into a volatile location. Such variables are guaranteed to be allocated on the "unsafe" stack. (2) does not seem to have any advantage over (1). Would (1) be acceptable? On Mon, May 9, 2016 at 2:23 PM, Evgenii Stepanov <[email protected]> wrote: > Does https://android-review.googlesource.com/#/c/223875/ look OK? > It disables the stack depth check if toybox is configured with no recursion. > > > On Sat, May 7, 2016 at 11:30 AM, enh <[email protected]> wrote: >> On Fri, May 6, 2016 at 10:58 PM, Rob Landley <[email protected]> wrote: >>> On 05/07/2016 12:16 AM, Evgenii Stepanov wrote: >>>> Sorry, I did not look at the problem hard enough. >>>> The real issue is interaction of this code with safestack >>>> (http://clang.llvm.org/docs/SafeStack.html), which splits the stack in >>>> 2 disjoint memory regions. If the two variables are allocated on >>>> different stacks, the comparison result is truly undefined. >>>> >>>> I don't really understand what this code is tying to do. Is it >>>> catching unlimited stack growth? Why does the comment speak about >>>> heap? >>> >>> It's a heuristic that enables an optimization. You could select >>> CONFIG_TOYBOX_NORECURSE to disable this optimization. >> >> i thought we were still using that, but it looks like i undid my >> setting of that option here: >> https://googleplex-android.git.corp.google.com/platform/external/toybox/+/a729fc8373ade0ced1cd0dd5ad43ef6a61a5cd24 >> >> https://android-review.googlesource.com/223514 (unsubmitted) turns it >> back on, since that might be easier than trying to come up with a >> heuristic that works well in a SafeStack world. as long as we're still >> using mksh i don't think it matters much anyway. >> >>> The help text of that option describes it a little: when one toybox >>> command calls another, it can either recurse into the new command's >>> main() function, or call the actual execve() to relaunch the toybox >>> binary with a fresh environment. Recursing is much faster, but has the >>> downside that if you do enough in a row you tend to accumulate debris >>> (open filehandles and unfreed mallocs and such from being halfway >>> through another program). (Plus if you do it _forever_, you'll actualy >>> run out of stack.) So it checks how much stack we've used as a simple >>> heuristic to see whether we should recurse or should exec. >>> >>> This heuristic has not been particularly tuned, that's one of my toysh >>> todo items. (toysh is likely to be the heaviest user.) >>> >>> Rob >> >> >> >> -- >> Elliott Hughes - http://who/enh - http://jessies.org/~enh/ >> Android native code/tools questions? Mail me/drop by/add me as a reviewer. _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
