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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
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
----------------
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?


================
Comment at: clang/test/CodeGen/attr-speculative-load-hardening.cpp:1-2
+// RUN: %clang_cc1 -std=c++11 -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK1
+// RUN: %clang_cc1 -std=c++11 -disable-llvm-passes -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK2
+//
----------------
zbrid wrote:
> chandlerc wrote:
> > 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.
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54555