Page MenuHomePhabricator

[clang][slh] add attribute for speculative load hardening
ClosedPublic

Authored by zbrid on Nov 14 2018, 4:46 PM.

Details

Summary

LLVM IR already has an attribute for speculative_laod_hardening. Before
this commit, when a user passed the -mspeculative-load-hardening flag to
Clang, every function would have this attribute added to it. This Clang
attribute will allow users to opt into SLH on a function by function basis..

Diff Detail

Repository
rC Clang

Event Timeline

zbrid created this revision.Nov 14 2018, 4:46 PM

Really cool, and nice job with the first round of things. Very nice testing!

Adding Kristof as he and I talked about this general direction and I want him to see the initial patch.

Some questions in-line.

clang/include/clang/Basic/Attr.td
3092 ↗(On Diff #174130)

Should this be done with Clang<"speculative_load_hardening">? Unsure...

clang/test/CodeGen/attr-speculative-load-hardening.cpp
1–2 ↗(On Diff #174130)

A question I have is how this interacts with the commandline flag?

Or are you thinking to handle that separately in a follow-up patch?

I'm pretty happy with either approach, mostly curious.

zbrid updated this revision to Diff 174298.Nov 15 2018, 4:31 PM

This changes the spelling of the attribute to be the Clang spelling. The test is updated to reflect this change. A FIXME is added to note that this attribute doesn't current work well with the command line flags for SLH.

zbrid marked an inline comment as done.Nov 15 2018, 4:40 PM
zbrid added inline comments.
clang/test/CodeGen/attr-speculative-load-hardening.cpp
1–2 ↗(On Diff #174130)

I'll address and test the interaction between this attribute and the command line flag in a follow up patch. The current plan is to 1) add an attribute to not harden a function 2) warn the user if they marked a function to not be hardened that may be hardened due to the behavior where a calling function gets hardened if a called function is hardened and inlined into the calling function 3) have the command line flag set the default and the function attribute to override the default.

kristof.beyls added inline comments.Nov 16 2018, 6:34 AM
clang/include/clang/Basic/AttrDocs.td
3611–3631 ↗(On Diff #174298)

I think the documentation/specification of this attribute should aim to:

  1. make sure that users of this attribute can reason about how they're code will be protected & whether the protection is useful for them.
  2. give enough freedom to make this implementable for all architectures.

Overall, it feels to me that the documentation as is, is a bit too concrete - or at least more concrete than ideal, for both aims 1 & 2.

For example, for aim 1, I'm very happy this includes the inlining semantics. But maybe it'd be even more useful if this was stated more abstractly/independently of inlining? For example outlining is another transformation that changes which function code lives in - so at first sight one might wonder if we'd need to define rules for outlining too. I haven't tried to think about that yet. Thinking out loud, maybe the description should be more something like "When you specify the speculative_load_hardening function attribute - it's guaranteed that SLH protection is applied to the code in the function, even if transformations like inlining happen. Similarly when you specify the no_speculative_load_hardening it is guaranteed that SLH protection is not applied." (Of course the exact semantics of the no_speculative_load_hardening attribute are TBD in a later patch - so we could delay that discussion till then).

Another concern for aim 1 with the wording as is, is that I think it is both to abstract in some places, and too concrete in others. I think that "This is a best-effort attempt to
mitigate all known speculative execution information leak vulnerabilities
that are based on the fundamental principles of modern processors'
speculative execution." is too vague. Especially since the attribute is called "speculative_load_hardening", I think the specification should be in terms of "hardening" "speculatively executed loads". Maybe a reasonable balance between abstract and concrete specification could be along the lines of "This is a best-effort attempt to mitigate known speculative execution information leak vulnerabilities by hardening loads that are executed speculatively."
I think it's useful to mention Spectre v1 vs Spectre v2 - but don't think they should be used to define what kind of incorrect speculation this hardens against. As is written now, it seems strictly necessary to me to basically say "aims to protect against SpectreV1, but not against SpectreV2", as the other words in the documentation aren't precise enough to be able to derive that.
It would be nice to have words in the documentation that allows to derive "aims to protect against SpectreV1, but not against SpectreV2", so the semantics of the attribute is specified a bit more abstractly. Of course, then it'd be good to still in the documentation say something like "... this means for example that this aims to protect against SpectreV1 but not against SpectreV2".

I realize I'm asking for a lot here and I also cannot immediately come up with strictly better wording. And I don't want to block forward progress. So, I'd also be happy with incrementally improving the documentation. But I do think it's important to agree on the general direction the semantic definition of this attribute should move to.
What do you think about this?

clang/test/CodeGen/attr-speculative-load-hardening.cpp
1–2 ↗(On Diff #174130)

This makes sense to me. I agree we'll very likely need an attribute to disable SLH per function.
I haven't tried to fully think through the consequences of hardening a function that was specifically marked to not be hardened as a consequence of inlining. I'm guessing not inlining in that case might be an alternative solution.
Anyway - I'm happy for incremental development here, so maybe that is a discussion for the follow-on patch.

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Not entirely sure, but I don't think so.
The way this is implemented both for x86 (if I understood the code correctly) and how this most likely will be implemented for AArch64 (see https://reviews.llvm.org/D49069 for what I expect will become the basis to implement SLH for AArch64) is that an illegal value is put in the stack pointer to signal misspeculation having happened. So, in that respect, having that illegal value in the stack pointer is an ABI rule to communicate mis-speculation across function boundaries.
However, given that this is ABI only used on miss-speculated paths, and miss-speculated paths get corrected eventually, programs will keep on working correctly if functions calling each other don't follow the same ABI in this respect. Obviously, you'll lose some protection against cross-function call miss-speculation.

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Not entirely sure, but I don't think so.
The way this is implemented both for x86 (if I understood the code correctly) and how this most likely will be implemented for AArch64 (see https://reviews.llvm.org/D49069 for what I expect will become the basis to implement SLH for AArch64) is that an illegal value is put in the stack pointer to signal misspeculation having happened. So, in that respect, having that illegal value in the stack pointer is an ABI rule to communicate mis-speculation across function boundaries.
However, given that this is ABI only used on miss-speculated paths, and miss-speculated paths get corrected eventually, programs will keep on working correctly if functions calling each other don't follow the same ABI in this respect. Obviously, you'll lose some protection against cross-function call miss-speculation.

Would there be value in making this a type attribute that appertains to the function type so that you don't lose this protection when calling through function pointers (as you could then add the attribute to the function pointer type as well)? If the stack pointer value is changed as part of the function prologue, this may not be needed, but if the stack pointer value is changed on the calling side, perhaps this would add value?

Does this hardening impact the ABI in any way? e.g., do we have to do anything special to handle calls through function pointers where the bound function pointer is marked with this attribute?

Not entirely sure, but I don't think so.

It influences the ABI, but not in a way that breaks compatibility.

The way this is implemented both for x86 (if I understood the code correctly) and how this most likely will be implemented for AArch64 (see https://reviews.llvm.org/D49069 for what I expect will become the basis to implement SLH for AArch64) is that an illegal value is put in the stack pointer to signal misspeculation having happened. So, in that respect, having that illegal value in the stack pointer is an ABI rule to communicate mis-speculation across function boundaries.
However, given that this is ABI only used on miss-speculated paths, and miss-speculated paths get corrected eventually, programs will keep on working correctly if functions calling each other don't follow the same ABI in this respect. Obviously, you'll lose some protection against cross-function call miss-speculation.

A good way to think about this is that SLH introduces a backwards compatible extension to the ABI. Two SLH'ed functions need to agree on their ABI, but they can be freely mixed with non-SLH functions.

Would there be value in making this a type attribute that appertains to the function type so that you don't lose this protection when calling through function pointers (as you could then add the attribute to the function pointer type as well)? If the stack pointer value is changed as part of the function prologue, this may not be needed, but if the stack pointer value is changed on the calling side, perhaps this would add value?

IMO, no.

This is fundamentally a property of the definition of a function, not of a call of a function. Whether the function being called has SLH or not doesn't change how you call it in any way.

aaron.ballman accepted this revision.Nov 16 2018, 4:25 PM

Would there be value in making this a type attribute that appertains to the function type so that you don't lose this protection when calling through function pointers (as you could then add the attribute to the function pointer type as well)? If the stack pointer value is changed as part of the function prologue, this may not be needed, but if the stack pointer value is changed on the calling side, perhaps this would add value?

IMO, no.

This is fundamentally a property of the definition of a function, not of a call of a function. Whether the function being called has SLH or not doesn't change how you call it in any way.

Great! Thank you for the explanations.

I think the interaction with the command line flag and the negative version of this attribute can be done incrementally. This LGTM!

This revision is now accepted and ready to land.Nov 16 2018, 4:25 PM
zbrid updated this revision to Diff 174487.Nov 16 2018, 5:13 PM

[clang][slh] update attribute docs

zbrid marked an inline comment as done.Nov 16 2018, 5:48 PM
zbrid added inline comments.
clang/include/clang/Basic/AttrDocs.td
3611–3631 ↗(On Diff #174298)

To start, I updated the sentence that you mentioned was a bit too vague with your suggested wording. I think the extra specificity is nice.

I do think it makes sense to update this documentation to include enough information about how the attribute works to reason about why this would work for v1 and not v2. However at this point, I think I'd have to be more familiar with Spectre variants and the fundamental differences between them to appropriately revise how the documentation currently lets users know which variant the attribute does harden against based on solely the semantics of the attribute rather than an explicit call out.

I wonder if users of this would similarly not understand the exact differences and so perhaps the emphasis that this is for v1 and not v2 is useful in the sense that it lets users match their idea of what's being fixed to the things they have in mind?

As for not giving people the impression that this is solely for Spectre v1. This may be a silly question [I'm new to this area and don't have full context on all the CPU vulnerabilities], but does this protect against other side channel timing attacks beyond Spectre v1? If so, is the issue that we should call these out as well? If not, is there a benefit you're thinking of from not emphasizing this is for v1 vs v2 or other issues?

As for this particular patch, do you think the current wording is sufficient and we can continue this discussion in future patches? I'm happy to keep this line of discussion on future patches and make updates as my understanding increases and and more behaviors get defined.

clang/test/CodeGen/attr-speculative-load-hardening.cpp
1–2 ↗(On Diff #174130)

Sounds good. Let's discuss the specifics with respect to inlining in the follow up patch.

zbrid marked an inline comment as done.Nov 16 2018, 5:49 PM
kristof.beyls added inline comments.Nov 20 2018, 2:25 AM
clang/include/clang/Basic/AttrDocs.td
3611–3631 ↗(On Diff #174298)

I've made an attempt at a wording that I think is better:

"""
This attribute indicates that
Speculative Load Hardening <https://llvm.org/docs/SpeculativeLoadHardening.html>_
should be enabled for the function body.

Speculative Load Hardening is a best-effort mitigation against
information leak attacks that make use of control flow
miss-speculation - specifically miss-speculation of whether a branch
is taken or not. Typically vulnerabilities enabling such attacks are
classified as "Spectre variant #1". Notably, this does not attempt to
mitigate against miss-speculation of branch target, classified as
"Spectre variant #2" vulnerabilities.

When inlining, the attribute is sticky. Inlining a function that
carries this attribute will cause the caller to gain the
attribute. This is intended to provide a maximally conservative model
where the code in a function annotated with this attribute will always
(even after inlining) end up hardened.
"""

I think the above:

  • Does describes more directly what the mitigation does; and at the same time indicates that this results in protecting against SpectreV1, but not V2.
  • I agree that users may not easily understand what the mitigation aims to protect against if we don't explain "it mitigates against SpectreV1, but not SpectreV2".
  • Maybe the main reason why I prefer the semantics not to be defined in terms of "SpectreV1" is that research is still very active in this area, and different researchers come up with slightly different terminology for different variants. For example, the recently published "A Systematic Evaluation of Transient Execution Attacks on Defenses" calls Spectre-V1 variants Spectre-PHT. I think we should aim to describe what the mitigation does. As a result, we should be able to relatively easily update documentation when different classifications or names get introduced to refer to specific vulnerabilities. For example, if the naming scheme introduced in the above paper gets traction we could easily augment the documentation to say "this aims to mitigate Spectre-PHT, but not Spectre-BTB".

I also removed a few sentence from the original in my proposal above, with the following reasons:

  • I expect that most cores targeted by LLVM perform at least some form of speculative execution, so I thought the sentence ending with "is expected to be a no-op." does not add much value.
  • The sentence starting with "Instead, this is a target-independent request...": I think it's assumed by default that attributes are target-independent, so don't see too much value in calling that out explicitly. I also though the rest of the sentence doesn't add too much over the other sentences already there.
  • In general, shorter documentation should be better than longer documentation, if it gets the same information across.

What do you think about the above proposed text?
I apologize for putting so much focus on the documentation. I think the documentation indeed will potentially need further iteration and refinement as SLH develops further. However, I do think that starting of with the best possible basis for documentation is a worthwhile exercise that will make future iterations easier, with a better overall result.

aaron.ballman added inline comments.Nov 20 2018, 6:22 AM
clang/include/clang/Basic/Attr.td
3092 ↗(On Diff #174487)

On reflection, I think this could make sense being applied to Obj-C methods as well, correct?

clang/include/clang/Basic/AttrDocs.td
3611 ↗(On Diff #174487)

I think the docs should also mention that the attribute can only be applied to a function declaration (and Obj-C methods if you decide to update the SubjectList above).

3614 ↗(On Diff #174487)

80-col

zbrid updated this revision to Diff 174820.Nov 20 2018, 12:12 PM
zbrid marked an inline comment as done.

[clang][slh] update docs with kristof's proposal; mention the attr is
function declarations

clang/include/clang/Basic/Attr.td
3092 ↗(On Diff #174487)

I don't know much about Objective C, but maybe we can work out whether this makes sense to use with Objective C together. There is a Subject called ObjCMethod which I could add as a subject. Assuming this will add speculative_load_hardening to the LLVM IR for Objective C in the same way this happens for C/C++, then do you know if there'd be any issue with that LLVM IR attribute being compatible with any Objective C specific middle end or back end passes/code? Off the top of my head, I think it would be fine, but I'm new to LLVM hacking, so I may have missed something.

clang/include/clang/Basic/AttrDocs.td
3611 ↗(On Diff #174487)

I'll make it explicit it's only for function declarations with a bit of rewording. Note that this will also appear in the section for function attributes on the page for Clang attributes. I'll add info about Objective C once we figure out if it makes sense.

3611–3631 ↗(On Diff #174298)

I think the wording you proposed looks good. I updated the documentation with it while also calling out this attribute is for function declaration as Aaron suggested. Also your points are well taken. I agree it's important to make the docs as good as possible. :)

aaron.ballman added inline comments.Nov 20 2018, 1:07 PM
clang/include/clang/Basic/Attr.td
3092 ↗(On Diff #174487)

I think you can get away with adding ObjCMethod to the subject list (updating the docs), and adding a new test with a .m extension that adds the attribute to an Objective-C method declaration and checks that the IR sees the attribute has been lowered. e.g.,

@interface SimpleClass
- (void)someMethod __attribute__((speculative_load_hardening));
@end
zbrid updated this revision to Diff 174971.EditedNov 21 2018, 1:02 PM

add objective c methods as a subject and related test

zbrid marked 3 inline comments as done.Nov 21 2018, 1:05 PM
kristof.beyls accepted this revision.Nov 22 2018, 12:00 AM

LGTM now. I'm not an expert on the clang attribute mechanics, so am relying on Aaron's judgement/review for that part.
Thanks Zola!

I noticed that the documentation for the LLVM-IR speculative_load_hardening attribute in LangRef (section https://llvm.org/docs/LangRef.html#function-attributes) will now no longer be the same as the documentation for the clang attribute here after reviewing. Maybe it'd be best to update that documentation to be the same as the one for the clang attribute here?

zbrid updated this revision to Diff 175296.Nov 26 2018, 10:57 AM
  • [clang][slh] add Clang attr no_speculative_load_hardening
zbrid updated this revision to Diff 175297.Nov 26 2018, 10:58 AM
  • [clang][slh] add Clang attr no_speculative_load_hardening

    Remove changes that were meant to be in a different revision
zbrid updated this revision to Diff 175306.Nov 26 2018, 11:35 AM

Update langref documentation about the related LLVM attribute

@kristof.beyls - Good catch. I updated the lang ref documentation as well. Thanks!

@aaron.ballman - Thanks!

This revision was automatically updated to reflect the committed changes.