Have callees set up their shadow stack frame. #220
Conversation
| // interpreter loop (which is long-lived) contains a commonly jumped-to | ||
| // setjump(), so we probably do want to fix this soon. | ||
| // | ||
| // YKFIXME: If you wanted to fix this you'd have to reload the shadow stack |
There was a problem hiding this comment.
This is what I wanted to discuss.
I wonder if we should try to fix this right off the bat. That's assuming you agree with the reasoning outlined in the comment.
It requires a little bit of implementation effort, but should be doable.
There was a problem hiding this comment.
From memory when we last discussed this, I believe all we have to do to reset the shadow stack after a longjump is to duplicate this line
store ptr %1, ptr @shadowstack_0, align 8
and put it right after the setjump call.
There was a problem hiding this comment.
Right, so I think what this PR does is already "correct" but (at least temporarily) can leak arbitrary amounts of shadow stack space. I'm fine with that temporarily: I think the fix for the leak is for setjmp (not longjmp!) to be nobbled in the way Lukas is suggesting.
There was a problem hiding this comment.
and put it right after the setjump call.
my idea was to reload the shadow pointer immediately before and then update all later uses to the reload, but in hindsight, I think Lukas' solution is simpler to implement.
There was a problem hiding this comment.
but (at least temporarily) can leak arbitrary amounts of shadow stack space
Indeed. That's what I concluded too.
There was a problem hiding this comment.
my idea was to reload the shadow pointer immediately before and then update all later uses to the reload
I think that would only reset the local shadow stack pointer, but as you correctly pointed out in case 1) this is already done automatically by the long jump. Storing that value back into the shadow stack global ensures subsequent calls also load the correct pointer value from the global.
There was a problem hiding this comment.
I don't think you can easily load it back before you jump: you don't know where the right value will be in the jmpbuf. But, if you could do that, it would I think be equivalent to doing it after the setjmp. Either way, the basic idea is solid!
| // | ||
| // Returns a pointer to the result of the call to malloc that was used to | ||
| // heap allocate memory for a shadow stack. | ||
| CallInst *insertMainPrologue(Function *Main, GlobalVariable *SSGlobal, |
There was a problem hiding this comment.
I'm fine with this for now but I suspect the "right" long term fix will also fix threading. That probably means dynamically intercepting process / thread creation, rather than inserting code into main directly.
|
I think this should go in as-is: there are things we could improve, yes, but this will already give us an important boost. |
|
I suggest we undraft this, and get it in. We can of course do an even better version in the coming days! |
|
Are there any little nitpicks I need to fix? Typos? Style? I'd be surprised if you couldn't find anything at all :) |
|
If there were, I'd have said so, I promise! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Before this change we'd have caller's allocate shadow space. Besides being an unconventional strategy, this also means that we allocate shadow space for things which will never require shadow space. We even had to conservatively allocate shadow space before external calls just in case they call-back into functions that do need shadow space. This change makes each callee requiring a shadow frame allocate it itself. Read the comment at the top of ShadowStack.cpp for implementation details. Measuring about a 22% speedup on bigloop.
vext01
force-pushed
the
new-sstack-allocs
branch
from
d7689df to
7ea4e19
Compare
|
This was just a name clash. Force pushed a fix. |
There is some nuance to this, so raising as a draft for now. I will comment on an item I want to discuss before we merge
Before this change we'd have caller's allocate shadow space. Besides being an unconventional strategy, this also means that we allocate shadow space for things which will never require shadow space. We even had to conservatively allocate shadow space before external calls just in case they call-back into functions that do need shadow space.
This change makes each callee requiring a shadow frame allocate it itself.
Read the comment at the top of ShadowStack.cpp for implementation details.
Measuring about a 22% speedup on bigloop.