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

8287325: AArch64: fix virtual threads with -XX:UseBranchProtection=pac-ret #9067

Closed
wants to merge 1 commit into from

Conversation

nick-arm
Copy link
Member

@nick-arm nick-arm commented Jun 7, 2022

The continuation free/thaw mechanism relies on being able to move thread stacks around in memory. However when PAC is enabled on supported AArch64 CPUs, the saved LR on the stack contains a "pointer authentication code" signed with the stack pointer at the time the frame was created. When a stack frame is relocated we need to re-sign the LR with the new stack pointer to ensure it authenticates successfully when the method returns.

Introduced ContinuationHelper::return_pc_at() to avoid directly reading the saved PC from the stack in shared code. On AArch64 with PAC it enabled it strips the PAC from the address after reading it, on all other platforms it just loads the PC from the stack as before.


Progress

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

Issue

  • JDK-8287325: AArch64: fix virtual threads with -XX:UseBranchProtection=pac-ret

Contributors

  • Alan Hayward <ahayward@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9067

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

Using diff file

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

…c-ret

The continuation free/thaw mechanism relies on being able to move thread
stacks around in memory.  However when PAC is enabled on supported
AArch64 CPUs, the saved LR on the stack contains a "pointer
authentication code" signed with the stack pointer at the time the frame
was created.  When a stack frame is relocated we need to re-sign the LR
with the new stack pointer to ensure it authenticates successfully when
the method returns.

Also introduced ContinuationHelper::return_pc_at() to avoid directly
reading the saved PC from the stack in shared code.  On AArch64 with PAC
it enabled it strips the PAC from the address after reading it, on all
other platforms it just loads the PC from the stack as before.
@bridgekeeper
Copy link

bridgekeeper bot commented Jun 7, 2022

👋 Welcome back ngasson! 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.

@nick-arm
Copy link
Member Author

nick-arm commented Jun 7, 2022

/contributor add ahayward

@openjdk openjdk bot added the rfr label Jun 7, 2022
@openjdk
Copy link

openjdk bot commented Jun 7, 2022

@nick-arm
Contributor Alan Hayward <ahayward@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented Jun 7, 2022

@nick-arm The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Jun 7, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 7, 2022

Webrevs

@pron
Copy link
Member

pron commented Jun 7, 2022

Could you explain what it is that this change attempts to do? Does every frame's returns address need to be patched on both freeze and thaw? If so, the slow path always has to be taken as we don't walk frames on the fast path. That slows down virtual threads by ~5-20x.

@pron
Copy link
Member

pron commented Jun 7, 2022

If this feature requires re-signing every frame, then PAC-RET should be reconsidered for Java frames (though not native frames). In that case, the right fix for JDK 19 would be to disable this mode when preview is enabled. BTW, +PreserveFramePointer also causes a huge slowdown for virtual threads as it requires the slow path to be taken on every thaw.

@theRealAph
Copy link
Contributor

theRealAph commented Jun 7, 2022

If this feature requires re-signing every frame, then PAC-RET should be reconsidered for Java frames (though not native frames). In that case, the right fix for JDK 19 would be to disable this mode when preview is enabled. BTW, +PreserveFramePointer also causes a huge slowdown for virtual threads as it requires the slow path to be taken on every thaw.

I agree. For the preview, I think we can do without -XX:UseBranchProtection=pac-ret. We should take some time to think about the best way to approach pac-ret with virtual threads.

@pron
Copy link
Member

pron commented Jun 8, 2022

I've read some about the ARM PAC feature, and it seems that pointers are signed after being combined with any arbitrary value stored in some register. The current implementation uses a frame pointer value in the fp register as the other register, which is not only problematic for virtual threads, but with some future planned repurposing of that register. Perhaps some other secret value, that is amenable to copying stacks, could be used to achieve the same effect (although not in JDK 19, obviously).

@nick-arm
Copy link
Member Author

nick-arm commented Jun 8, 2022

Yes the current implementation uses the frame pointer for signing/authentication so -XX:UseBranchProtection=pac-ret turns on PreserveFramePointer.

For now I can make another patch that disables PAC-RET when Arguments::enable_preview() is true.

@dean-long
Copy link
Member

dean-long commented Jun 8, 2022

I don't see why we can't use the stack pointer instead of the frame pointer and get rid of the PreserveFramePointer requirement.

@pron
Copy link
Member

pron commented Jun 8, 2022

@dean-long Which sp, though? The frame's ultimate sp? I I think interpreted frames don't know their sp when they're first constructed (and not sure about when they return). Or it could be the actual location of the return address, but I don't know what that means for the protection mechanism. On the other hand, the preserved fp is always the return address's location minus 1 anyway.

Either way, it's not the right value for Loom. We'll need to consider 1. how important is PAC for Java frames, and 2. if it is important, what second value to use that is invariant under stack copying.

@dean-long
Copy link
Member

dean-long commented Jun 8, 2022

@pron I don't think there is any reason why interpreted frames couldn't continue to use FP while compiled frames use SP. It's not clear how useful PAC is for Java code at dynamic locations (how would the exploit find the ROP gadgets?). I don't think there is a useful alternative to FP/SP that is invariant under stack copying.

nick-arm added a commit to nick-arm/jdk that referenced this issue Jun 9, 2022
PAC-RET is not currently compatible with the continuation freeze/thaw
mechanism used by virtual threads, and additionally
-XX:UseBranchProtection=pac-ret enables PreserveFramePointer which
prevents the use of the continuation thaw fast-path (about 15% slower
for the "Skynet" JMH benchmark).  So for now just disable PAC when
--enable-preview was also passed.

See some earlier discussion here:
openjdk#9067
@nick-arm
Copy link
Member Author

nick-arm commented Jun 9, 2022

For now I can make another patch that disables PAC-RET when Arguments::enable_preview() is true.

See #9102

@pron
Copy link
Member

pron commented Jun 9, 2022

@dean-long Thing is, I don't know the requirement on the second value. This suggests that even a constant value of zero can be used, and perhaps we could do better (?) by using some thread/continuation id, assuming this is useful at all for Java frames. In particular, if the attacker knows the location of the return address, then they also know the value of (a preserved) fp. Perhaps @nick-arm can enlighten us.

@nick-arm
Copy link
Member Author

nick-arm commented Jun 9, 2022

Zero works as well, as does any other value that's the same at function entry/exit. But the disadvantage of zero is that you get the same authentication code for each call to the function, whereas using SP gives you different codes as long as the stack depth is different.

@nick-arm
Copy link
Member Author

nick-arm commented Jun 9, 2022

I'll close this one for now.

@nick-arm nick-arm closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot rfr
4 participants