This is an archive of the discontinued LLVM Phabricator instance.

Implement non-trapping variant of -fsanitize=cfi* sanitizers.
ClosedPublic

Authored by pcc on Jun 4 2015, 7:51 PM.

Details

Summary

This causes programs compiled with this flag to print a diagnostic when
a control flow integrity check fails instead of aborting. Diagnostics are
printed using UBSan's runtime library.

The main motivation of this feature over -fsanitize=vptr is fidelity with
the -fsanitize=cfi implementation: the diagnostics are printed under exactly
the same conditions as those which would cause -fsanitize=cfi to abort the
program. This means that the same restrictions apply regarding compiling
all translation units with -fsanitize=cfi, cross-DSO virtual calls are
forbidden, etc.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 27171.Jun 4 2015, 7:51 PM
pcc retitled this revision from to Implement diagnostic mode for -fsanitize=cfi*, -fsanitize=cfi-diag..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: rsmith.
pcc added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Jun 9 2015, 2:02 PM

It doesn't make sense to me to add a "sanitizer" that controls how another sanitizer reports failures; that is not how the -fsanitize= flags are supposed to work at all.

I think you should follow the existing model for frontend sanitizers here: if -fsanitize-undefined-trap-on-error is specified, then trap, otherwise produce a diagnostic. If it really makes sense to have a per-sanitizer trap-on-error mode (as opposed to the existing per-sanitizer -fsanitize-recover= setting), we should enable that for all the frontend sanitizers, not just this one, but it seems (perhaps naively) to me like -fsanitize-recover= should be sufficient here already?

pcc added a comment.Jun 9 2015, 2:38 PM

My rationale for making the CFI flags work differently than the UBSan flags was that the default trapping behaviors of the sanitizers ought to be different. Because the primary purpose of the -fsanitize=cfi* flags is to enable a security hardening mechanism, they should trap by default. UBSan's primary purpose is to catch and report issues during development, so it makes sense for it to diagnose by default.

-fsanitize-recover= doesn't do exactly what we want for CFI, either. UBSan with recovery disabled still emits diagnostics, and if we wanted CFI to have the same behaviour it would introduce a dependency on RTTI and a runtime library, both things we would like to avoid in CFI in order to reduce binary size overhead.

I agree with you that it does seem weird for cfi-diag to look like a sanitizer. It does seem that something like -fsanitize-trap= (with appropriate defaults for the various sanitizers) may be the best option.

rsmith added a comment.Jun 9 2015, 5:49 PM

UBSan with recovery disabled still emits diagnostics, and if we wanted CFI to have the same behaviour it would introduce a dependency on RTTI and a runtime library, both things we would like to avoid in CFI in order to reduce binary size overhead.

I don't follow why the dependence on RTTI is linked to whether you emit diagnostics, can you explain? I would think the diagnostic handler could gracefully degrade to not using the RTTI if it's unavailable.

As for the library dependence, I would think it's preferable to produce *some* kind of diagnostic by default on the way out rather than just silently terminating, unless there are security implications to doing so (are you worried about an attacker overwriting the sanitizer runtime's callback function?). A CFI failure could show up during testing due to "normal" lifetime bugs, uninitialized variables, and the like, not just due to an attack being thwarted, and in either case I think it'd be good to have *some* indication that the bad thing happened, even if we can only print it to stderr by default. But, ultimately, I'm happy leaving the choice with you. If you think that we should trap silently by default, then I have no objection to adding a -fsanitize-trap= mechanism, and turning it on by default for CFI.

FWIW, I agree with Richard here. We already have different defaults for which sanitizers are recoverable, and which are not (-fsanitize=addres, unreachable etc.).
We may also have the defaults for which sanitizers trap-on-error (-fsanitize=cfi), and which print diagnostics. I think that adding -fsanitize-trap=<list> is the way to go.
I would love to see the ugly mask -fsanitize=undefined-trap gone in one way or another. Instead, I'd vote for silently disabling vptr sanitizer if the user runs the invocation:

`clang++ -fsanitize=undefined -fsanitize-trap=undefined`.

"Well, you're asking us to skip runtime checks and trap on error instead, we'll do that, but implicitly disable sanitizers which can't be implemented w/o runtime checks".

rsmith added a comment.Jun 9 2015, 6:32 PM

"Well, you're asking us to skip runtime checks and trap on error instead, we'll do that, but implicitly disable sanitizers which can't be implemented w/o runtime checks".

Yeah, I'd be OK with that, but we'll need to be pretty clear about it in the documentation. (It's not entirely natural that "trap on error" and "don't use the runtime library" are the same thing, but it's the latter that requires us to turn off the vptr sanitizer when the former is enabled.)

pcc added a comment.Jun 9 2015, 7:29 PM

I don't follow why the dependence on RTTI is linked to whether you emit diagnostics, can you explain?

The CFI mechanism is designed not to depend on RTTI; see http://clang.llvm.org/docs/ControlFlowIntegrityDesign.html for details on how it works. However if diagnostics are enabled we will need RTTI, as that is the mechanism we use to get the name of the class that failed the check.

I would think the diagnostic handler could gracefully degrade to not using the RTTI if it's unavailable.

Perhaps, but without a class name its usefulness is limited.

As for the library dependence, I would think it's preferable to produce *some* kind of diagnostic by default on the way out rather than just silently terminating, unless there are security implications to doing so (are you worried about an attacker overwriting the sanitizer runtime's callback function?).

I believe that the default behavior ought to be that which is recommended for binaries shipped to end users. A diagnostic is no more actionable to a user than a crash, and even the function call to emit the diagnostic has a small file size cost. As for security implications, the runtime library (as with any code linked into the program) forms part of an attack surface which we should seek to minimize.

If you think that we should trap silently by default, then I have no objection to adding a -fsanitize-trap= mechanism, and turning it on by default for CFI.

Sounds good then, I'll work on that approach.

pcc updated this revision to Diff 27800.Jun 16 2015, 5:14 PM
pcc edited edge metadata.
  • Re-implement in terms of -fsanitize-trap
pcc updated this revision to Diff 27801.Jun 16 2015, 5:42 PM
  • Document -fsanitize-trap precedence over -fsanitize-recover
pcc updated this revision to Diff 27803.Jun 16 2015, 5:51 PM

Reverting the documentation change, moving it to D10464

pcc updated this revision to Diff 27978.Jun 18 2015, 5:23 PM

Refresh

pcc retitled this revision from Implement diagnostic mode for -fsanitize=cfi*, -fsanitize=cfi-diag. to Implement non-trapping variant of -fsanitize=cfi* sanitizers..Jun 18 2015, 5:23 PM
pcc updated this revision to Diff 27981.Jun 18 2015, 5:37 PM
  • Fix assertion failure
samsonov accepted this revision.Jun 18 2015, 5:46 PM
samsonov added a reviewer: samsonov.

I'm OK with this change, but let Richard have a look as well.

This revision is now accepted and ready to land.Jun 18 2015, 5:46 PM
rsmith accepted this revision.Jun 18 2015, 6:38 PM
rsmith edited edge metadata.

LGTM too.

This revision was automatically updated to reflect the committed changes.