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

Fri Nov 16 17:48:57 PST 2018

zbrid marked an inline comment as done.
zbrid 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
----------------
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.

================
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
+//
----------------
kristof.beyls wrote:
> 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.
Sounds good. Let's discuss the specifics with respect to inlining in the follow up patch.

Repository:
  rL LLVM

https://reviews.llvm.org/D54555