udpate stack guard bytes inside __guard_setup()
ClosedPublic
Actions

Authored by lffpires_ruabrasil.org on Apr 23 2018, 5:40 PM.

Details

Reviewers

nwhitehorn
jhibbits
kib

Commits

rS332940: Carefully update stack guard bytes inside __guard_setup().

Summary

This is necessary to make sure that functions that can have stack protection are not used to update the stack guard.
In that case, the stack guard check would fail when it shouldn't.

guard_setup() calls elf_aux_info(), which, in turn, calls memcpy() to update stack_chk_guard. If either elf_aux_info() or memcpy() have stack protection enabled, __stack_chk_guard will be modified before returning from them, causing the stack protection check to fail.

This change uses a temporary buffer to delay changing __stack_chk_guard until elf_aux_info() returns.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16279
Build 16226: arc lint + arc unit

Event Timeline

lffpires_ruabrasil.org created this revision.Apr 23 2018, 5:40 PM

Harbormaster completed remote builds in B16270: Diff 41773.Apr 23 2018, 5:41 PM

lffpires_ruabrasil.org retitled this revision from udpate stack guard bytes inside __guard_setup This is necessary to make sure that functions that can have stack protection are not used to update the stack guard. In that case, the stack guard check would fail when it shouldn't. to udpate stack guard bytes inside __guard_setup().Apr 23 2018, 5:50 PM

lffpires_ruabrasil.org edited the summary of this revision. (Show Details)

lffpires_ruabrasil.org added reviewers: nwhitehorn, jhibbits.

lffpires_ruabrasil.org added a reviewer: kib.Apr 23 2018, 5:58 PM

lffpires_ruabrasil.org added inline comments.

lib/libc/secure/stack_protector.c
69	I guess I should probably zero out tmp_stack_chk_guard[idx] after copying to __stack_chk_guard, to avoid leaving it on the stack longer than necessary.

kib added inline comments.Apr 23 2018, 8:20 PM

lib/libc/secure/stack_protector.c
59	You can spell it as nitems(__stack_chk_guard)).
67	Put the variable declaration into the locals block at the start of the function body.
69	I believe modern compilers can detect the copy and optimize the loop into memcpy() call. To fight this, you can use e.g. volatile specifier for tmp_stack_chk_guard.

addressed review comments

Harbormaster completed remote builds in B16279: Diff 41796.Apr 24 2018, 12:42 PM

lffpires_ruabrasil.org marked 4 inline comments as done.Apr 24 2018, 12:43 PM

Why did you added PPC guys to the review ? Does this problem appeared on Power ? Do nathanw/jhibbits want to commit this ?

lib/libc/secure/stack_protector.c
60	Please move idx into the same declaration as error.

This revision is now accepted and ready to land.Apr 24 2018, 1:02 PM

@kib, I added them just because this showed up while working on an optimization for powerpc64, and at the time I didn't know who I should add as reviewer. Please feel free to commit this.

Closed by commit rS332940: Carefully update stack guard bytes inside __guard_setup(). (authored by kib). · Explain WhyApr 24 2018, 3:59 PM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: imp. · View Herald TranscriptApr 24 2018, 3:59 PM

lffpires_ruabrasil.org mentioned this in D15118: memcpy optimization for powerpc64.Apr 24 2018, 6:57 PM

lffpires_ruabrasil.org added a subscriber: breno.leitao_gmail.com.Apr 25 2018, 6:57 PM

lffpires_ruabrasil.org mentioned this in D15208: update __stack_chk_guard safely.Apr 26 2018, 12:26 PM