Page MenuHomePhabricator

[x86/SLH] Add a real Clang flag and LLVM IR attribute for Speculative Load Hardening.
ClosedPublic

Authored by chandlerc on Aug 23 2018, 5:16 AM.

Details

Summary

Wires up the existing pass to work with a proper IR attribute rather
than just a hidden/internal flag. The internal flag continues to work
for now, but I'll likely remove it soon.

Most of the churn here is adding the IR attribute. I talked about this
Kristof Beyls and he seemed at least initially OK with this direction.
The idea of using a full attribute here is that we *do* expect at least
some forms of this for other architectures. There isn't anything
*inherently* x86-specific about this technique, just that we only have
an implementation for x86 at the moment.

While we could potentially expose this as a Clang-level attribute as
well, that seems like a good question to defer for the moment as it
isn't 100% clear whether that or some other programmer interface (or
both?) would be best. We'll defer the programmer interface side of this
for now, but at least get to the point where the feature can be enabled
without relying on implementation details.

This also allows us to do something that was really hard before: we can
enable *just* the indirect call retpolines when using SLH. For x86, we
don't have any other way to mitigate indirect calls. Other architectures
may take a different approach of course, and none of this is surfaced to
user-level flags.

I can split this patch up into an LLVM-side and a Clang-side, but it
seemed honestly harder to review there, as this way it is clear how
these pieces are intended to be used. That said, if reviewers would like
them split, I'm happy to oblige.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Aug 23 2018, 5:16 AM
rnk accepted this revision.Aug 23 2018, 1:22 PM

It makes sense to me to do this all in one go and defer the clang __attribute__ until later.

llvm/include/llvm/IR/Attributes.td
249 ↗(On Diff #162149)

I can't tell if this is just vim fixing the lack of a proper trailing newline, but revert any whitespace change if possible.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1169 ↗(On Diff #162149)

These appear to repeat LLVMBitcodes.h, unless I am mistaken. Weird. Anyway, fixing that is out of scope.

This revision is now accepted and ready to land.Aug 23 2018, 1:22 PM
rsmith added inline comments.Aug 23 2018, 1:36 PM
clang/include/clang/Driver/Options.td
2004–2007 ↗(On Diff #162149)

We usually only provide -cc1 flags to switch into the non-default state.

clang/include/clang/Frontend/CodeGenOptions.def
214 ↗(On Diff #162149)

Spaces rather than underscores here.

clang/lib/Driver/ToolChains/Arch/X86.cpp
169–170 ↗(On Diff #162149)

-mspeculative-load-hardening -mretpoline-external-thunk does not enable +retpoline-indirect-branches (but -mretpoline-external-thunk by itself does). Is that intentional?

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1169 ↗(On Diff #162149)

The values "above the fold" in Phabricator aren't all the same as the enum value plus one. *shrug*

chandlerc updated this revision to Diff 162319.Aug 23 2018, 7:47 PM
chandlerc marked 3 inline comments as done.

Address review comments.

Thanks, should all be addressed now.

clang/lib/Driver/ToolChains/Arch/X86.cpp
169–170 ↗(On Diff #162149)

Yes, as that's is specifically useful (and that is one thing that motivated my above comment that we should remove the ability to use -mretpoline-external-thunk by itself eventually).

You might only need SLH's minimal retpolines, but you might need to provide your own thunks for them in the Kernel for example.

llvm/include/llvm/IR/Attributes.td
249 ↗(On Diff #162149)

Soryr, my annoying editor. Will fix before submit.

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
1169 ↗(On Diff #162149)

Yeah, this isn't factorable easily. Anyways....

chandlerc updated this revision to Diff 162322.Aug 23 2018, 8:06 PM

Add a test file that I somehow missed earlier (sorry about that).

rsmith accepted this revision.Aug 23 2018, 9:26 PM
rsmith added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
1150 ↗(On Diff #162322)

You can just use hasArg(OPT_mspeculative_load_hardening) here.

kristof.beyls added inline comments.Aug 23 2018, 11:15 PM
llvm/docs/LangRef.rst
1659–1661 ↗(On Diff #162322)

It feels wrong to me to have source code that is annotated to get hardened, but that actually will not get hardened (whether it is due to it being inlined somewhere or due to any other automatic behind-the-back-of-the-developer transformation). I fear this may lower trust in the protection this attribute provides.
I assume there is a use case where the developer wants to indicate "no hardening in this function nor in any functions inlined here". If that needs to be supported, my feel is that we may need to support that in another way.
I guess that there must be some cases where just duplicating the function to be inlined in the source code into a hardened and a non-hardened version could be too hard to do for some programs.
So, in short, I don't know what the best solution here is. I just want to raise my concern that I don't think it's a good idea that functions that are marked to be hardened end up not getting hardened under some circumstances.

Move to a more conservative model suggested by Kristof.

chandlerc marked an inline comment as done.Aug 23 2018, 11:57 PM
chandlerc added inline comments.
llvm/docs/LangRef.rst
1659–1661 ↗(On Diff #162322)

I actually started out with this, so I'm very sympathetic to the perspective.

Given that you prefer it, let's start with this very conservative stance. We can always add specific mechanisms to be less conservative later, but I agree that it seems much better to start with a safe position.

kristof.beyls accepted this revision.Aug 27 2018, 5:18 AM

I'm not an expert on many of the areas touched by this patch, but it looks fine from me from a high-level point-of-view, modulo a few nits I have on a few comments.

clang/lib/Driver/ToolChains/Arch/X86.cpp
151 ↗(On Diff #162325)

s/deprecated/deprecating/
s/warning/warn/

llvm/include/llvm/IR/Attributes.td
181–185 ↗(On Diff #162325)

After updating the LangRef.rst text, I think this comment also needs to be updated. As is, it still documents the old inlining behaviour?
I'm also not sure in how far the comment makes most sense here. This is already documented in LangRef.rst, and AFAIK, the inlining compatibility mode is not something that is defined here?

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
78–82 ↗(On Diff #162325)

I'm not sure, but do you really still need an option to force enable SLH when you have function attributes now to enable it?
When you generate LLVM-IR using clang, you now have a clang option to enable that function attribute on all functions, so do you still have scenarios where you need an LLVM backend option to override the function attribute?

llvm/lib/Target/X86/X86TargetMachine.cpp
474 ↗(On Diff #162325)

I guess this is true for some other passes too, and they don't add such a comment here. Maybe best to remove this comment if my guess is right?

chandlerc marked 5 inline comments as done.Sep 4 2018, 5:25 AM

All outstanding comments addressed, and landing this. Thanks all for the reviews and let me know if I missed anything.

llvm/include/llvm/IR/Attributes.td
181–185 ↗(On Diff #162325)

Updated the comment.

You can override the compatibility here -- see the CompatRule productions just before the MergeRule ones.

I'd like this to be documented near the code in addition to in the langref personally. It reminds the reader to look below at the rule sections.

llvm/lib/Target/X86/X86SpeculativeLoadHardening.cpp
78–82 ↗(On Diff #162325)

Long term, no, we don't.

I just have some users that are already passing this flag. Out of convenience, I wanted to leave this working until they have picked up the new version of Clang and LLVM and migrated. I don't expect it to last long (a week or two?).

This revision was automatically updated to reflect the committed changes.
chandlerc marked an inline comment as done.