ShadowStack#223
Conversation
elazaro-riverside
left a comment
There was a problem hiding this comment.
I like these changes. As an aside, I think it's cool in Rust how you can implement the From trait and get Into as a by-product. I think Jackson will have the final decision but these changes look good to me.
Thanks for the feedback, I just learned about the from trait, and I think it's super neat as well. |
|
I haven't tackled the CVEAssert wiring or any testing yet, but here is the new shape per our discussions. In stack object lookups there are three paths
I got rid of galloping when I was rewriting to make the stack grow downwards like it does on x86, partially also because the implementation using binary search is much simpler. |
| /// Compute the sentinel pointer value for this object, 1 past its limit | ||
| pub fn past_limit(&self) -> Vaddr { | ||
| self.limit + 1 | ||
| self.limit.saturating_add(1) |
There was a problem hiding this comment.
Clearly the existing behavior was not the right one, but what is the argument for saturating add? Can we record it in a comment?
There was a problem hiding this comment.
It was mostly just to be consistent with the other places I used saturating add for Vaddr offset computation. Happy to add a comment to each instance if you have a suggestion for what to say. Maybe "avoid overflow". Realistically though this should never actually saturate
There was a problem hiding this comment.
Could we just panic on overflow? We should probably revisit the overflow behavior everywhere...
There was a problem hiding this comment.
We could remove the saturating calls and just force it to panic with overflow-checks = true in Cargo.toml? Unsure what the correct behavior is here that doesn't cost performance.
|
Addressed everything except the two comments I left open |
This (WIP) PR implements a special path for stack objects, keeping them in a ShadowStack with various references that make it easy for us to search for and validate ShadowObjects, including invalidating and re-using slots for stack re-use.
Currently, it is untested, and needs CVEAssert to call
__resolve_push_frameon every frame entry (except that of the first frame, unless we adjust the constructor to defer that)