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

8277204: Implement PAC-RET branch protection on Linux/AArch64 #6334

Closed
wants to merge 34 commits into from

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Nov 10, 2021

PAC is an optional feature in AArch64 8.3 and is compulsory in v9. One
of its uses is to protect against ROP based attacks. This is done by
signing the Link Register whenever it is stored on the stack, and
authenticating the value when it is loaded back from the stack. If an
attacker were to try to change control flow by editing the stack then
the authentication check of the Link Register will fail, causing a
segfault when the function returns.

On a system with PAC enabled, it is expected that all applications will
be compiled with ROP protection. Fedora 33 and upwards already provide
this. By compiling for ARMv8.0, GCC and LLVM will only use the set of
PAC instructions that exist in the NOP space - on hardware without PAC,
these instructions act as NOPs, allowing backward compatibility for
negligible performance cost (2 NOPs per non-leaf function).

Hardware is currently limited to the Apple M1 MacBooks. All testing has
been done within a Fedora Docker image. A run of SpecJVM showed no
difference to that of noise - which was surprising.

The most important part of this patch is simply compiling using branch
protection provided by GCC/LLVM. This protects all C++ code from being
used in ROP attacks, removing all static ROP gadgets from use.

The remainder of the patch adds ROP protection to runtime generated
code, in both stubs and compiled Java code. Attacks here are much harder
as ROP gadgets must be found dynamically at runtime. If/when AOT
compilation is added to JDK, then all stubs and compiled Java will be
susceptible ROP gadgets being found by static analysis and therefore
potentially as vulnerable as C++ code.

There are a number of places where the VM changes control flow by
rewriting the stack or otherwise. I’ve done some analysis as to how
these could also be used for attacks (which I didn’t want to post here).
These areas can be protected ensuring the pointers to various stubs and
entry points are stored in memory as signed pointers. These changes are
simple to make (they can be reduced to a type change in common code and
a few addition sign/auth calls in the backend), but there a lot of them
and the total code change is fairly large. I’m happy to provide a few
work in progress patches.

In order to match the security benefits of the Apple Arm64e ABI across
the whole of JDK, then all the changes mentioned above would be
required.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

  • JDK-8277204: Implement PAC-RET branch protection on Linux/AArch64
  • JDK-8277543: Implement PAC-RET branch protection on Linux/AArch64 (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6334/head:pull/6334
$ git checkout pull/6334

Update a local copy of the PR:
$ git checkout pull/6334
$ git pull https://git.openjdk.java.net/jdk pull/6334/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6334

View PR using the GUI difftool:
$ git pr show -t 6334

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6334.diff

a74nh added 4 commits Nov 10, 2021
PAC is an optional feature in AArch64 8.3 and is compulsory in v9. One
of its uses is to protect against ROP based attacks. This is done by
signing the Link Register whenever it is stored on the stack, and
authenticating the value when it is loaded back from the stack. If an
attacker were to try to change control flow by editing the stack then
the authentication check of the Link Register will fail, causing a
segfault when the function returns.

On a system with PAC enabled, it is expected that all applications will
be compiled with ROP protection. Fedora 33 and upwards already provide
this. By compiling for ARMv8.0, GCC and LLVM will only use the set of
PAC instructions that exist in the NOP space - on hardware without PAC,
these instructions act as NOPs, allowing backward compatibility for
negligible performance cost (2 NOPs per non-leaf function).

Hardware is currently limited to the Apple M1 MacBooks. All testing has
been done within a Fedora Docker image. A run of SpecJVM showed no
difference to that of noise - which was surprising.

The most important part of this patch is simply compiling using branch
protection provided by GCC/LLVM. This protects all C++ code from being
used in ROP attacks, removing all static ROP gadgets from use.

The remainder of the patch adds ROP protection to runtime generated
code, in both stubs and compiled Java code. Attacks here are much harder
as ROP gadgets must be found dynamically at runtime. If/when AOT
compilation is added to JDK, then all stubs and compiled Java will be
susceptible ROP gadgets being found by static analysis and therefore
potentially as vulnerable as C++ code.

There are a number of places where the VM changes control flow by
rewriting the stack or otherwise. I’ve done some analysis as to how
these could also be used for attacks (which I didn’t want to post here).
These areas can be protected ensuring the pointers to various stubs and
entry points are stored in memory as signed pointers. These changes are
simple to make (they can be reduced to a type change in common code and
a few addition sign/auth calls in the backend), but there a lot of them
and the total code change is fairly large. I’m happy to provide a few
work in progress patches.

In order to match the security benefits of the Apple Arm64e ABI across
the whole of JDK, then all the changes mentioned above would be
required.
@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2021

👋 Welcome back a74nh! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 10, 2021
@openjdk
Copy link

openjdk bot commented Nov 10, 2021

@a74nh The following labels will be automatically applied to this pull request:

  • build
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 10, 2021
@theRealAph
Copy link
Contributor

theRealAph commented Nov 10, 2021

Gosh. This is going to take some time to review, and will need at least two reviewers.

Copy link
Member

@erikj79 erikj79 left a comment

Build change looks good, but I can't comment on the code changes.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 10, 2021

Gosh. This is going to take some time to review, and will need at least two reviewers.

Sure. And thanks in advance.

make/autoconf/flags-cflags.m4 Outdated Show resolved Hide resolved
@adinn
Copy link
Contributor

adinn commented Nov 10, 2021

I am also reviewing this.

Copy link
Member

@magicus magicus left a comment

Build changes look much better now, thanks!

Build part approved; the actual code changes needs approval from others.

@fweimer-rh
Copy link

fweimer-rh commented Nov 11, 2021

Is the code still mapped read-write all the time?

@adinn
Copy link
Contributor

adinn commented Nov 11, 2021

Is the code still mapped read-write all the time?

That depends on what code you mean. The JVM code compiled from C++ sources is mapped RO(X) in the text section like any compiled C/C++ code. Protection of that code is covered by the changes to the build system.

The runtime generated runtime stubs and Java method code into which this patch may insert the required PAC instructions are written into a code cache in a section which is mapped RW(X) all the time. It would be hard to map even a subset of this code cache RO because generated code includes call and data sites that need to be patched during execution.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 11, 2021

The runtime generated runtime stubs and Java method code into which this patch may insert the required PAC instructions are written into a code cache in a section which is mapped RW(X) all the time. It would be hard to map even a subset of this code cache RO because generated code includes call and data sites that need to be patched during execution.

Am I right is saying that for Macos, all generated code is remapped RO before execution?

An additional concern I have is that if the globals data was attacked then the UseROPProtection flag could be flipped, and all code after that point would be generated without ROP protection. Marking all the globals data as RO would fix that. Alternatively remove UseROPProtection and then in the macroassembler always generate PAC code, using just the subset of instructions that are NOPs on non-PAC hardware. Or alternatively only generate PAC code based on a #define set at build time. Each option has its own downsides.

@adinn
Copy link
Contributor

adinn commented Nov 11, 2021

Am I right is saying that for Macos, all generated code is remapped RO before execution?

Ah, no, it seems the code cache is not RWX all the time as far as Java threads are concerned. The Macos/AArch64 code is strategically calling pthread_jit_write_protect_np at Java <-> JVM transition points.

That ensures that executable regions are executable but not writable (RX) from a Java thread when running JITted Java code and are writable but not executable (RW) when it calls into JVM code.

An additional concern I have is that if the globals data was attacked then the UseROPProtection flag could be flipped, and all code after that point would be generated without ROP protection. Marking all the globals data as RO would fix that. Alternatively remove UseROPProtection and then in the macroassembler always generate PAC code, using just the subset of instructions that are NOPs on non-PAC hardware. Or alternatively only generate PAC code based on a #define set at build time. Each option has its own downsides.

Globals data can legitimately be written during JVM startup (perhaps in some cases also during execution?). So, they cannot simply be marked as RO.

I am not sure this concern is really warranted. If an attacker is already able to overwrite UseROPProtection then a concern over the resulting omission of JITted ROP protection seems like attending to the loud banging of the stable door while Shergar has already been diced into stew meat.

@theRealAph
Copy link
Contributor

theRealAph commented Nov 11, 2021

Am I right is saying that for Macos, all generated code is remapped RO before execution?

Ah, no, it seems the code cache is not RWX all the time as far as Java threads are concerned. The Macos/AArch64 code is strategically calling pthread_jit_write_protect_np at Java <-> JVM transition points.

And this requires magic kernel support. I did mention it to a kernel engineer who wasn't very impressed, but I think it's pretty cool.

@fweimer-rh
Copy link

fweimer-rh commented Nov 11, 2021

Am I right is saying that for Macos, all generated code is remapped RO before execution?

Ah, no, it seems the code cache is not RWX all the time as far as Java threads are concerned. The Macos/AArch64 code is strategically calling pthread_jit_write_protect_np at Java <-> JVM transition points.

And this requires magic kernel support. I did mention it to a kernel engineer who wasn't very impressed, but I think it's pretty cool.

It's possible to emulate this to some extent with memory protection keys on POWER and (recent) x86. See pkey_alloc.

@openjdk
Copy link

openjdk bot commented Nov 11, 2021

@a74nh this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout aarch64_linux_pac
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 11, 2021
Copy link
Contributor

@theRealAph theRealAph left a comment

This is looking pretty nice now. With the check for -XX:-UseFramePointer argument consistency we're done.

@a74nh
Copy link
Contributor Author

a74nh commented Feb 11, 2022

This is looking pretty nice now. With the check for -XX:-UseFramePointer argument consistency we're done.

Excellent! I'm away all next week, so will add the check when I get back.

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Feb 21, 2022
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Feb 22, 2022
@a74nh
Copy link
Contributor Author

a74nh commented Feb 23, 2022

I did another full jteg run, and everything looks fine.
Think that's all the review comments resolved now too.

@a74nh
Copy link
Contributor Author

a74nh commented Feb 24, 2022

Any more comments? Otherwise I'll integrate later

@a74nh
Copy link
Contributor Author

a74nh commented Feb 24, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 24, 2022
@openjdk
Copy link

openjdk bot commented Feb 24, 2022

@a74nh
Your change (at version c4e0ee3) is now ready to be sponsored by a Committer.

@adinn
Copy link
Contributor

adinn commented Feb 24, 2022

Yup this looks good to me. I will sponsor.

@adinn
Copy link
Contributor

adinn commented Feb 24, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Feb 24, 2022

Going to push as commit 6fab8a2.
Since your change was applied there have been 33 commits pushed to the master branch:

  • abc0ce1: 8282316: Operation before String case conversion
  • 0796620: 8281944: JavaDoc throws java.lang.IllegalStateException: ERRONEOUS
  • 231e48f: 8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422
  • f4486a1: 8262400: runtime/exceptionMsgs/AbstractMethodError/AbstractMethodErrorTest.java fails in test_ame5_compiled_vtable_stub with wrapper
  • 3cfffa4: 8282188: Unused static field MathContext.DEFAULT_DIGITS
  • 379fd85: 8277369: Strange behavior of JMenuBar with RIGHT_TO_LEFT orientation, arrow keys behaves opposite traversing through keyboard
  • cd3e59e: 8282299: Remove unused PartialArrayScanTask default constructor
  • a661003: 8281614: serviceability/sa/ClhsdbFindPC.java fails with java.lang.RuntimeException: 'In code in NMethod for jdk/test/lib/apps/LingeredApp.steadyState' missing from stdout/stderr
  • 43dc9ef: 8281988: Create a regression test for JDK-4618767
  • 253cf78: 8282076: Merge some debug agent changes from the loom repo
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/022d80707c346f4b82ac1eb53e77c634769631e9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 24, 2022
@openjdk openjdk bot closed this Feb 24, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 24, 2022
@openjdk
Copy link

openjdk bot commented Feb 24, 2022

@adinn @a74nh Pushed as commit 6fab8a2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@dholmes-ora
Copy link
Member

dholmes-ora commented Feb 25, 2022

@a74nh this seems to have broken the Zero build:

src/hotspot/share/gc/shared/barrierSetNMethod.cpp:58:33: error: 'pauth_strip_pointer' was not declared in this scope
 58 |   AARCH64_ONLY(return_address = pauth_strip_pointer(return_address));

I'm guessing a missing include file.

@mlbridge
Copy link

mlbridge bot commented Feb 25, 2022

Mailing list message from Andrew Haley on build-dev:

On 2/25/22 13:12, David Holmes wrote:

the Zero build

Zero on AArch64? I wonder if we need a better way of handling this than

AARCH64_ONLY(NOT_ZERO(

Maybe AARCH64_PORT_ONLY would be a Good Thing.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

@a74nh
Copy link
Contributor Author

a74nh commented Feb 25, 2022

Yes, we spotted this today too. https://bugs.openjdk.java.net/browse/JDK-8282392

My initial thought was that I needed to add a pauth header file with stub functions to linux_zero/. Which does feel a little awkward.

AARCH64_PORT_ONLY does sound like a better option.

Thankfully there is no need for full pac support in zero too.... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
8 participants