[PATCH] D54555: [clang][slh] add attribute for speculative load hardening

[Zola Bridges via Phabricator via llvm-commits]

zbrid added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3092
+  let Spellings = [Clang<"speculative_load_hardening">];
+  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Documentation = [SpeculativeLoadHardeningDocs];
----------------
aaron.ballman wrote:
> On reflection, I think this could make sense being applied to Obj-C methods as well, correct?
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. 


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3611-3631
+This attribute indicates that
+`Speculative Load Hardening <https://llvm.org/docs/SpeculativeLoadHardening.html>`_
+should be enabled for the function body. 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. These vulnerabilities are classified as "Spectre
+variant #1" vulnerabilities typically. Notably, this does not attempt to
----------------
kristof.beyls wrote:
> zbrid wrote:
> > kristof.beyls wrote:
> > > 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?
> > 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.
> 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.
> 
> 
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. :)


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3611
+  let Content = [{
+This attribute indicates that
+`Speculative Load Hardening <https://llvm.org/docs/SpeculativeLoadHardening.html>`_
----------------
aaron.ballman wrote:
> 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).
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54555