-
Notifications
You must be signed in to change notification settings - Fork 54
Renamed file and updated naming scheme to also include names used in academia. #12
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
I would need to re-read the paper to discuss what BTB vs PHT mean, but I want to specifically mention why I believe that the current demo should be classified with variant 1 and not variant 2 of Spectre. Variant 2 of Spectre has an important distinct property from variant 1: it involves injecting a target into the predictor that no correct execution of a branch could possibly reach. This is extremely different from simply training the predictor to go to one valid target, and then leveraging this trained target to trigger a misprediction. The original naming from Intel also specifically highlights this: Branch Target Injection (BTI). This is an extremely important distinction for a number of reasons:
This is why I think it is really important to classify this as variant 1. Hope that explanation helps. |
After checking the code, I have to agree with @cc0x1f here. This is an instance of Spectre-BTB (aka Spectre variant 2). In all Spectre variants, the branch predictor is manipulated (which you call injecting). For both Spectre-PHT and Spectre-BTB, there are 4 different variants of injecting a different prediction (see [2], Figure 3). Regarding your reasons:
To conclude, this PoC is, as @cc0x1f said, Spectre-BTB not Spectre-PHT. [1] https://spectreattack.com/spectre.pdf |
Spectre-BTB is much more broad than "Spectre variant 2" as it is used pervasively in the rest of the industry. "variant 2" is a subset of "Spectre-BTB" which is all I've been suggesting. I'm not saying anything about "Spectre-PHT" here... And jumping a head a bit:
I'm not arguing about this classification as I've tried to say several times. I think in this regard, we are talking past each other a bit. My firm position from working with all of the HW vendors and other parties as part of the cross-industry mitigation effort is that "variant 2" refers very specifically to the core issue of CVE-2017-5715 which Intel describes as "Branch Target Injection". The injection combined with the BTB are the two critical components for something to be "variant 2". Using the academic classification you have proposed, this is Spectre-BTB-CA-OP. I do not think that "variant 2" should be used to refer to any other issues. I am very happy to provide a mapping from the classification you use in the research paper, but I is essential that "variant 2" only refer to the injection into the BTB. The reason why this is so crucial is that there are mitigation techniques that are specifically documented as "mitigating variant 2" which only mitigate the injection into the BTB. They do not mitigate the issue in this demo! We cannot call this "variant 2" without creating serious and harmful confusion among those deploying mitigations. And we cannot go change that documentation to use a different classification scheme. The specific mitigation strategy I refer to here is the use of IBRS and/or STIBP in conjunction with operating system changes to prevent cross-address space BTB injection. We have strong reasons to believe this is the most critical aspect to mitigate, and we have strong mitigations widely available and well documented as handling "variant 2". I hope this makes clear why I am insisting on this narrow definition of the term "variant 2". I want to again note that I'm not disagreeing that this demo is classified in a related space exactly as you indicate:
I 100% agree with this. I simply disagree that we can call this "variant 2". That will create active and harmful confusion as it is not mitigated by STIBP or similar hardware techniques. I also wanted to respond to some of the other comments here:
The demo isn't about what unrelated vulnerabilities might exist, it is about which specific one is demonstrated. The existence of an indirect branch doesn't make this a meaningful demo of any other variation on Spectre-BTB, it is just the one you identified: Spectre-BTB-SA-IP. If we want to demo the others, we should craft narrow and specific demos of them.
Yes, exactly. But see above: the mitigations are documented to cover "variant 2", and due their narrow nature, it is essential we don't put other things into that set.
This is not accurate. First, the mitigations I'm referring to are not a single flag, but a set of changes to HW and system software. It is the conjunction of system software changes and availability of IBPB/IBRS/STIBP (in different combinations based on HW generation) for Intel parts for example. Second, while some have elected to not fully deploy these mitigations for user-space applications due to the severe performance impact on older HW parts, newer HW parts from Intel and other vendors do not have that problem. There, even user-space applications have "variant 2" (as I described it above) fully mitigated when combined with the system software changes.
Agreed. This is specifically why I do not want it classified in any way as "variant 2" which is widely believed to be thoroughly and easily mitigated. |
I think the main argument was that this is not Spectre v1, which is clearly described as "bounds check bypass". So, calling anything that is not Spectre-PHT "Spectre v1" is misleading. Better, because the v1,v2,... naming scheme is unclear to many, we should altogether avoid it. |
The original discussions with Intel and everyone else around this made it abundantly clear it wasn't just about bounds checks. No one is arguing "bounds check bypass" is good terminology. I don't think "variant 1" vs. "Spectre-PHT" is more or less clear. I don't find "PHT" especially useful (there are too many forms of branch predictor for me to like naming after each one of them) and so I somewhat prefer the perhaps overly generic "variant 1".
Only on some microarchitectures which use a pattern history table? There are other branch predictors that are still susceptible to the same core idea...
Some find "variant 1" misleading. Some find the "PHT" taxonomy misleading....
I don't think we will find a perfect naming scheme. But I think we might simply have a nice table that tracks all of the variations here and explains how they relate to each other. |
Hey sorry about the huge delay getting back to you, it was a really busy time of year. (Cppcon, performance review, ...) I wanted to sit on this reply to give others a chance to respond, but they didn't and I feel bad that I've left you hanging. I really appreciate this issue being raised, since I also was super confused about the exact boundaries being drawn around Spectre v1 vs v2, and had a couple iterations of this kind of discussion in private without gaining as much understanding of the issue as this thread has brought me. So, thank you!
This seems like a major part of the disagreement worth elaborating on. Calling it "bounds check bypass" is IMO definitely overspecifying it, and that term/concept is worth abandoning forever. The specific code pattern of a direct branch based on a comparison of index against array length, as opposed to any other direct branch, does not change how the vulnerability works or how it can be removed. The same vulnerability/mitigations will work whenever a memory address can be made to point to a secret by mistraining the direct branch predictor. (Much of the discussion here revolves around indirect branches, but at least direct branches are all the same microarchitecturally, right?) SLH works on more than just bounds checks, for example! (We get into messier territory if we want to talk about predicted branches that lead to a secret, but not via a speculative buffer overflow. IDK, what do you call that?) As an example, many months ago I modified our spectre v1 example in a local copy to instead never use (or, well, rely on the use of) an out-of-range index, not even speculatively. Instead I wanted to execute a speculative bypass of the condition below, the "short string optimization": class MyShortStringOptimization {
size_t size_;
union Data {char* p; char s[sizeof(p)];};
Data data_;
public:
size_t size() const { return size_; }
const char& operator[](size_t n) const {
if (size_ >= sizeof(data_.s)) {
// is this so big we need to store in the heap instead?
return data_.p[n];
}
return data_.s[n];
}
}; It worked perfectly, and was prevented perfectly by SLH! In this example, we can train the branch predictor to think that the string is short, and then pass it a string (and index) that is larger than
This is a good point. I hope we do that as little as possible, though. Even though indirect and direct branches are different on the CPU, they are not different in the C++ programming language (or any other high level programming language). Any if statement can become an indirect branch, and any virtual function call can become a direct branch -- not just the direct/indirect branches that we expect, respectively. As a result, we likely don't want to have separate mitigations for the two cases, even if we do need separate test cases. (Which are necessarily flawed for the same reason, in some way we might need to fix.) For what it's worth, I think that the original naming scheme had a not-unreasonable basis, and mapped cleanly to our intent. The new naming scheme doesn't completely capture the right conceptual boundaries, and also is a lot of opaque letters. Also, it will be hecking merge conflicts. So I am -1 on renaming the file unless we have a really solid name everyone can agree is perfect. The README change I am more sympathetic to, but I think we need a dedicated index rather than putting it in the README. There's too many overlapping names and this would get big, fast. |
I'm aware that SLH works on more than just direct branches (depending on the specific case). But a systematization of attacks should not be tailored to a specific defense, right? Otherwise we could tailor a systematization also to retpoline and it might not look more useful then either. I agree that it doesn't make sense to group it by the bounds check bypass, but that's also not what we propose.
And I agree, branches are not always directly exposed to the programming language. But the systematization of attacks should not be based on programming languages as that would mean that we need different systematization for every programming language!? |
Renamed spectre_v1_indirect_jumps.cc to spectre_v2.cc as this is actually an implementation of Spectre-V2/Spectre-BTB, not Spectre-V1/Spectre-PHT. This can be verified by adding an lfence before line 137. If it was indeed a Spectre-PHT attack, speculation would end here but instead the data is still leaked. By adding the lfence before line 51 we can actually prevent leakage. Therefore, we mistrain the Branch Target Buffer, making it a v2/BTB attack.
Also updated the names used in the demos/README.md file to also include the names used by academia. This change is mostly to keep names across industry and academia consistent.