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..
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 25336 Build 25335: arc lint + arc unit
Event Timeline
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 | ||
---|---|---|
3097 | Should this be done with Clang<"speculative_load_hardening">? Unsure... | |
clang/test/CodeGen/attr-speculative-load-hardening.cpp | ||
2–3 | 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. |
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.
clang/test/CodeGen/attr-speculative-load-hardening.cpp | ||
---|---|---|
2–3 | 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. |
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
3636–3656 | I think the documentation/specification of this attribute should aim to:
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 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. | |
clang/test/CodeGen/attr-speculative-load-hardening.cpp | ||
2–3 | This makes sense to me. I agree we'll very likely need an attribute to disable SLH per function. |
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?
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.
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!
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
3636–3656 | 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 | ||
2–3 | Sounds good. Let's discuss the specifics with respect to inlining in the follow up patch. |
clang/include/clang/Basic/AttrDocs.td | ||
---|---|---|
3636–3656 | I've made an attempt at a wording that I think is better: """ Speculative Load Hardening is a best-effort mitigation against When inlining, the attribute is sticky. Inlining a function that I think the above:
I also removed a few sentence from the original in my proposal above, with the following reasons:
What do you think about the above proposed text? |
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3097 | On reflection, I think this could make sense being applied to Obj-C methods as well, correct? | |
clang/include/clang/Basic/AttrDocs.td | ||
3636 | 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). | |
3639 | 80-col |
[clang][slh] update docs with kristof's proposal; mention the attr is
function declarations
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3097 | 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 | ||
3636 | 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. | |
3636–3656 | 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. :) |
clang/include/clang/Basic/Attr.td | ||
---|---|---|
3097 | 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 |
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?
- [clang][slh] add Clang attr no_speculative_load_hardening
Remove changes that were meant to be in a different revision
@kristof.beyls - Good catch. I updated the lang ref documentation as well. Thanks!
@aaron.ballman - Thanks!
Should this be done with Clang<"speculative_load_hardening">? Unsure...