This is an archive of the discontinued LLVM Phabricator instance.

SafeStack documentation improvements
ClosedPublic

Authored by ksvladimir on Jun 22 2015, 3:58 AM.

Details

Summary

This patch makes the following improvements to the SafeStack documentation:

  • Explicitly states the security guarantees of the SafeStack
  • Clarifies which of the security guarantees are probabilistic
  • Re-orders security limitations to put the most severe ones first
  • Explains how `__attribute__((no_sanitize("safe-stack")))` works and how to use it safely
  • Explains that SafeStack should be combined with a forward-edge protection mechanism, such as CPI, IFCC or others
  • Multiple readability and stylistic improvements

Diff Detail

Repository
rL LLVM

Event Timeline

ksvladimir updated this revision to Diff 28102.Jun 22 2015, 3:58 AM
ksvladimir retitled this revision from to SafeStack documentation improvements.
ksvladimir updated this object.
ksvladimir edited the test plan for this revision. (Show Details)
ksvladimir added reviewers: pcc, jfb.
ksvladimir added a subscriber: Unknown Object (MLST).

Updated to include the cfe-commits list as a subscriber.

jfb added inline comments.Jun 22 2015, 10:53 AM
docs/SafeStack.rst
107 ↗(On Diff #28102)

"may be not complete yet" is pretty awkward phrasing.

111 ↗(On Diff #28102)

I'd strengthen this last statement: at the moment safe stack assumes that the compiler's implementation is correct. This has not been verified except through code inspection, and could always regress in the future. It's therefore desirable to have a separate static or dynamic binary analysis / checker.

ksvladimir updated this revision to Diff 28222.Jun 23 2015, 4:43 AM

Addressed feedback by jfb:

  • Rephrased some ugly language
  • Stressed the need for end-to-end verification of SafeStack instrumentation

Thanks for the comments! I've addressed them in the updated patch.

docs/SafeStack.rst
107 ↗(On Diff #28102)

I've rephrased this and the previous paragraphs to make it sound less awkward.

111 ↗(On Diff #28102)

Great point! I think it applies to the entire SafeStack instrumentation, not just the hiding aspect, hence I added this note at the end of the Security Limitations section.

jfb edited edge metadata.Jun 23 2015, 10:08 AM

lgtm. I'll leave final signoff up to @pcc.

pcc accepted this revision.Jun 23 2015, 2:39 PM
pcc edited edge metadata.

LGTM

Do you have commit access?

docs/SafeStack.rst
177 ↗(On Diff #28222)

Nit: Integrity

This revision is now accepted and ready to land.Jun 23 2015, 2:39 PM

Thanks a lot for the review! No, I do not have commit access, please feel free to commit on my behalf.

ksvladimir updated this revision to Diff 28291.Jun 23 2015, 2:52 PM
ksvladimir edited edge metadata.

Fixed a typo Integirty -> Integrity

docs/SafeStack.rst
177 ↗(On Diff #28222)

Fixed.

This revision was automatically updated to reflect the committed changes.
pcc added a comment.Jun 23 2015, 3:32 PM

Committed. Please do consider requesting commit access (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access) if you anticipate contributing further to the project.