RFR: 8287325: AArch64: fix virtual threads with -XX:UseBranchProtection=pac-ret [v2]

[Hao Sun]

On Thu, 10 Aug 2023 23:53:34 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> Hao Sun has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Rename return_pc_at and patch_pc_at
>>   
>>   Rename return_pc_at to return_address_at.
>>   Rename patch_pc_at to patch_return_address_at.
>
>> 
>> Finally, we choose to use value zero as the modifier. Trivially, it's compatible with virtual threads. However, compared to FP modifier, this solution would reduce the strength of PAC-RET protection to some extent. E.g., you get the same authentication code for each call to the function, whereas using FP gives you different codes as long as the stack depth is different.
> 
> So, how important is this weakening? Could we keep the FP, but save a relative FP into the stack in compiled code? That wouldn't be hard, and might be a better fix. What do you think?

Thanks for your comments. @theRealAph 
> So, how important is this weakening?  

To be honest, I'm not sure how to describe the importance degree. And as I see it, we should apply SP modifier if we can.

> Could we keep the FP, but save a relative FP into the stack in compiled code? That wouldn't be hard, and might be a better fix. What do you think?

I think it should work if we simply store the relative SP in the stack.
And we should also store it for the "interpreted frame" as well, hence we may need update that metadata as well.

Anyway, I just uploaded the new implementation, i.e. using **relative SP** as the PAC modifier, where we don't need store the relative SP on the stack.
@theRealAph  and @dean-long  Could you help take another look at this patch? Thanks

In the new commits, the four TODOs in our prototype [1] are addressed.

TODO-1: Fuzz.java
Frames may be freezed by the slow path and thawed by the fast path.
Hence we need resign the return address for topmost frame, which is
previously updated on heap frame by freeze_slow().
See the update in function Thaw<ConfigT>::thaw_fast().

TODO-2: Enable more PAC authentications in shared code
In the shared code, we try to do as many PAC signing and
authentication as possible. But it's meaningless for the frames on the
heap.

TODO-3: stub tests at file stubRoutines.cpp
This issue was fixed in [JDK-8310355](https://bugs.openjdk.java.net/browse/JDK-8310355) already.

TODO-4: clobber rscratch1 or rscratch2 issue
In our demo patch [2], we tried to find all the exception cases of
rscratch2 usages, that is, we cannot clobber rscratch2 in enter() or
leave(). In my local test, tier1~3 passed on Linux/AArch64.
Even so, I'm not sure if we indeed find **all** the cases. Hence, in
this new revision, we still follow the previous prototype, i.e. saving
and resotring the scratch register on the stack for the sake of safety.

Test:
1. Cross compilations on arm32/s390/ppc/riscv passed.
2. zero build and x86 build passed.
3. tier1~3 passed on Linux/AArch64 w/ and w/o PAC-RET.

[1] https://github.com/openjdk/jdk/pull/13322#issuecomment-1566347900
[2] https://github.com/shqking/jdk/commit/c3ca8640

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13322#issuecomment-1676658096