8387792: Enable PAC-RET for VM code on Windows/ARM64

[raneashay]

This patch adds MSVC's "/guard:signret" flag to the C/C++ compilation
flags so that the VM code includes signing and authentication
instructions to ensure that the return address is not tampered by any
callee. Specifically, MSVC chooses signing using the B key, so every
non-leaf function starts with the pacibsp instruction (for signining
the return address) and ends with the autibsp instruction (for
authenticating the return address). Both pacibsp and autibsp
instructions are in the NOP space, so older AArch64 processors that do
not support these instructions shouldn't be impacted by these
instructions.

As a matter of slight detail, this patch adds the "/guard:signret" flag
only when the OpenJDK build is passed the "--enable-branch-protection"
flag, which is off by default. Consequently, this change will not
impact ordinary builds of OpenJDK.

I've validated this patch by running the test/jdk:tier{1,2,3},
test/hotspot/jtreg:tier{1,2,3}, test/langtools:tier{1,2,3}, and
test/lib-test:tier1 tests with branch protection enabled. This patch
does not introduce any new failures.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8387792: Enable PAC-RET for VM code on Windows/ARM64 (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31795/head:pull/31795
$ git checkout pull/31795

Update a local copy of the PR:
$ git checkout pull/31795
$ git pull https://git.openjdk.org/jdk.git pull/31795/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 31795

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31795.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back arane! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@raneashay The following label will be automatically applied to this pull request:

• build

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

[mlbridge]

Webrevs

• 02: Full - Incremental (93170e60)
• 01: Full - Incremental (b1f09f10)
• 00: Full (f94f2c35)

[dholmes-ora]

Seems reasonable. I have a couple of queries, but the approval needs to come from @erikj79 .

[dholmes-ora]

Is it possible for the toolchain type to be "microsoft" when the target OS is not "windows"??

Related: the docs say this is/was Linux only, but the preceding logic does not check the OS for gcc/clang. ??

[dholmes-ora]

I wonder if it would be cleaner to separate the BP compiler flag from the BP assembler flag? (E.g. we would define BRANCH_PROTECTION_CFLAG and BRANCH_PROTECTION_ASFLAG separately and so avoid repeating platform/toolchain checks.)

[raneashay]

Thanks David for taking a look! I will incorporate the suggestions soon.

Just as a heads up, I discovered that this patch breaks the os::win32::platform_print_native_stack() function, since the pointer signature now needs to be dropped when walking the stack. It seems like the fix should be small (I just need to invoke the XPACLRI instruction along with some plumbing).

Sorry about any confusion that this causes, but I wanted to let you know there's an additional (small) change required to this patch. I'll post an update after I run through all tests again.

[openjdk]

@raneashay hotspot-runtime has been added to this pull request based on files touched in new commit(s).

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-runtime. This can be overridden with the /reviewers command.

[raneashay]

Fixed the stack walking issue using the XPACLRI instruction in the pauth_drop_signature_win_arm64() function. This patch updates pauth_windows_aarch64.inline.hpp in preparation for a subsequent patch that will enable PAC for JITed code.

Also merged the master branch to pull an unrelated test fix.

[dholmes-ora]

Thanks for the updates. Can't comment on the Aarch64 specifics but I will put this through our build system just as a sanity check.

[dholmes-ora]

Sanity build check was fine.

[shqking]

Regarding the test you've done, IIUC, you built one JDK with --enable-branch-protection and ran the jtreg tests with VM option -XX:UseBranchProtection=standard.

Note that as listed in https://bugs.openjdk.org/browse/JDK-8388072, I found ~500 jtreg failures on Linux/AArch64(i.e. Neoverse-V1 cpu). I was wondering did you find any jtreg failure on Win/AArch64 side? Thanks

[raneashay]

Hi @shqking, thanks much for running the tests with "-XX:UseBranchProtection". I was under the (now mistaken) assumption that because this patch doesn't change the runtime CPU detection code for the CPU_PACA feature, the "-XX:UseBranchProtection" flag would have no effect, so I was running tests without passing "JTREG=VM_OPTIONS=-XX:UseBranchProtection=standard".

I was able to reproduce the assertion failure. The problem is that the LHS of the assertion (top.pc()) is after removing the pointer signature (see frame::frame() in frame_aarch64.inline.hpp) whereas the RHS uses the raw pointer. I fixed this locally by calling ContinuationHelper::return_address_at() on the RHS pointer. That call invokes pauth_strip_verifiable() on AArch64 but it's a NOP on other platforms.

Since you have a JBS issue for this, would you like to add it to your fix? If not, I'm happy to roll it into this patch. In the meantime, I'll go run all of my tests again with "-XX:UseBranchProtection=standard".

[shqking]

@raneashay Thanks for your reply.

Yes, adding ContinuationHelper::return_address_at() should resolve the assertion failure. Based on my local testing, there are several call sites where this helper is required, and I currently have a WIP fix addressing them.

Since you have a JBS issue for this, would you like to add it to your fix? If not, I'm happy to roll it into this patch. In the meantime, I'll go run all of my tests again with "-XX:UseBranchProtection=standard".

Either approach works for me. That said, I suggest keeping this PR narrowly scoped—focused on the build system and C runtime changes rather than Java code—and handling the assertion fixes separately under my JBS issue. WDYT?