RFR: 8308503: AArch64: SIGILL when running with -XX:UseBranchProtection=pac-ret on hardware without PAC feature
Andrew Haley
aph at openjdk.org
Tue May 23 15:30:08 UTC 2023
On Tue, 23 May 2023 15:06:39 GMT, Andrew Haley <aph at openjdk.org> wrote:
>> When revisiting the behavior of UseBranchProtection [1], we get one SIGILL error when running with -XX:UseBranchProtection=pac-ret on hardware without PAC.
>>
>> Problem:
>>
>> We build and run `java --version` with the following configuration matrix `Config X VMoption X Machine`.
>>
>>
>> Config = {--enable-branch-protection, null}
>> VMoption = {-XX:UseBranchProtection=pac-ret, -XX:UseBranchProtection=standard}
>> Machine = {w/ PAC, w/o PAC}
>>
>>
>> VM crashes with SIGILL error for configure `Config=null, VMoption=pac-ret, Machine=w/o PAC`. The unrecognized instruction is `pacia x30, x29`, i.e. `pacia(lr, rfp)` generated by function `MacroAssembler::protect_return_address()`. [2]
>>
>> Root cause:
>>
>> 1. Instruction `pacia` is not in the NOP space. That's why `Config=null, VMoption=pac-ret` passes on `hardware w/ PAC`, but fails on `hardware w/o PAC`.
>>
>> 2. -XX:UseBranchProtection=pac-ret behaves differently from the document [3], i.e.
>>
>>
>> In order to use Branch Protection features in the VM,
>> --enable-branch-protection must be used
>>
>>
>> `_rop_protection` is not turned off for `Config=null`. That's why `VMoption=pac-ret, Machine=w/o PAC` passes with
>> `Config=--enable-branch-protection` but fails with `Config=null`.
>>
>> Fix:
>>
>> This patch refines the parsing of -XX:UseBranchProtection=pac-ret:
>>
>> 1. We handle "pac-ret" and "standard" in the same way, since only one type of branch protection is implemented for now, i.e. "pac-ret". We may update "standard" in the future if "bti" protection is added.
>>
>> 2. `_rop_protection` is not turned on unless all the three conditions are satisfied [4]. Otherwise, it's kept off and one warning message is emitted.
>>
>>
>> // Enable PAC if this code has been built with branch-protection, the
>> // CPU/OS supports it, and incompatible preview features aren't enabled.
>>
>>
>> [1] https://bugs.openjdk.org/browse/JDK-8287325?focusedCommentId=14581099&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14581099
>> [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L5976
>> [3] https://github.com/openjdk/jdk/blob/master/doc/building.md#branch-protection
>> [4] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/vm_version_aarch64.cpp#L457
>
> src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 466:
>
>> 464: } else if (Arguments::enable_preview()) {
>> 465: // Not currently compatible with continuation freeze/thaw.
>> 466: warning("ROP-protection is incompatible with virtual threads preview feature. Disabling ROP-protection.");
>
> Suggestion:
>
> _rop_protection = false;
> warning("ROP-protection is incompatible with virtual threads preview feature. Disabling ROP-protection.");
I presume you didn't intend to change the logic here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14095#discussion_r1202509833
More information about the hotspot-dev
mailing list