Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need way to properly name retpoline thunks #25

Closed
rmustacc opened this issue Jul 16, 2019 · 1 comment

Comments

@rmustacc
Copy link

commented Jul 16, 2019

In gcc7 there is support for a number of different functions which are called thunks. These thunks may fire at function entry, return, or be used for indirect calls, among other things. The way that gcc7 names these depends on whether or not it believes that the general system supports what it considers hidden, link-once capabilities. This is summarized in the USE_HIDDEN_LINKONCE macro. Unfortunately, for a confluence or reasons, gcc7 does not currently believe that our environment supports the use of USE_HIDDEN_LINKONCE. Thew ay that gcc7 names the thunks depends on support for this feature. Without this feature, the thunks are named in ways that aren't very usable.

Unfortunately, to properly mitigate Spectre v2 in software, we are opting to use retpolines on systems that don't have enhanced IBRS. To do that, one needs to pass the following two options to gcc7: -mindirect-branch=thunk-extern -mindirect-branch-register. These cause us not to emit retpolines inline, but rather to a centralized point. This is important beacuse we need to patch all of those points and we don't want to have introduce a new reolcation type to krtld (and the surrounding toolchain) at this time for that.

@richlowe put together a patch for this which basically forces us to name indirect extern thunk types such as those used by retpolines regardless of the USE_HIDDEN_LINKONCE capabilities. The way that gcc7 works here is that there are two different types of ways that the various thunks are compiled:

  1. In the context of various functions.
  2. At the end of a compilation unit to emit various thunks noted as being required.

When using the retpoline configuration described above, the only thunks that are present are the ones emitted via 1. However, because the compiler comes back for others in case two, that is why cfun may or may not be valid in the patch. Further, because of these different types and uses, it seems reasonable that we can override the naming issue when dealing with indirect extern thunks, because they need to refer to a single external name and the implementation will never be part of the compilation unit itself, hence the fact that we force the external name makes sense.

To test this, I've put together a couple different things. First, I built illumos and did a wsdiff between the version with these changes and without it, changing nothing else in illumos. The wsdiff only had noise based on changes in CTF. Next, I did a bunch of testing of the actual retpoline implementation. While I haven't finished the verification that no more non-indirect calls remain, I have confirmed that we properly are always building with the right names in this case.

Finally, I went through and ran the gcc test suite. The results I had before were:

                === gcc Summary ===

# of expected passes            117266
# of unexpected failures        71
# of unexpected successes       1
# of expected failures          371
# of unresolved testcases       1
# of unsupported tests          2089

                === g++ Summary ===

# of expected passes            105204
# of unexpected failures        3
# of expected failures          394
# of unsupported tests          4054

After, I had:

                === gcc Summary ===

# of expected passes            117276
# of unexpected failures        61
# of unexpected successes       1
# of expected failures          371
# of unresolved testcases       1
# of unsupported tests          2089


                === g++ Summary ===

# of expected passes            105204
# of unexpected failures        3
# of expected failures          394
# of unsupported tests          4054

Going through the differences between the tests, I was able to observe that the indirect-extern thunk tests now passed. The other thunk tests failed in the same way as before, which was part of the goal -- to make sure that they weren't impacted.

@rmustacc

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

Resolved in e8c3c2b.

@rmustacc rmustacc closed this Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant
You can’t perform that action at this time.