8371918: aarch64: Incorrect pointer dereference in TemplateInterpreterGenerator::generate_native_entry

[bsdkurt]

…rGenerator::generate_native_entry

I believe there's a incorrect pointer deference in TemplateInterpreterGenerator::generate_native_entry() in this part of the code:

// get native function entry point in r10
  {
    Label L;
    __ ldr(r10, Address(rmethod, Method::native_function_offset()));
    ExternalAddress unsatisfied(SharedRuntime::native_method_throw_unsatisfied_link_error_entry());
    __ lea(rscratch2, unsatisfied);
    __ ldr(rscratch2, rscratch2);
    __ cmp(r10, rscratch2);
    __ br(Assembler::NE, L);
    __ call_VM(noreg,
               CAST_FROM_FN_PTR(address,
                                InterpreterRuntime::prepare_native_call),
               rmethod);
    __ get_method(rmethod);
    __ ldr(r10, Address(rmethod, Method::native_function_offset()));
    __ bind(L);
  }

If I understand this correctly, the entry point for unsatisfied link error is loaded into rscratch2. The next instruction, ldr(rscratch2, rscratch2), dereferences that pointer and reads from the text segment the initial instructions at the entry point into rscratch2. It then compares the native method entry point in r10 with the initial instructions loaded into rscratch2 which will never match. I believe the intent here was to compare the native method entry point with the unsatisfied link error entry point and the ldr(rscratch2, rscratch2) instruction should be removed.

This was found on OpenBSD/aarch64. OpenBSD has a security feature where the text segments are marked execute only and do not allow reads independent of execution. the ldr(rscratch2, rscratch2) instruction causes a segfault because it is reading the text segment. While this bug was found on OpenBSD I believe it applies to all OS on aaarch64.

This change removes the errant aarch64 hotspot assembly instruction that was reading from libjvm.so .text segment.

Updated comment with markdown for code.

---

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-8371918: aarch64: Incorrect pointer dereference in TemplateInterpreterGenerator::generate_native_entry (Bug - P4)

Reviewers

• Andrew Haley (@theRealAph - Reviewer)
• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28327

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@bsdkurt This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8371918: aarch64: Incorrect pointer dereference in TemplateInterpreterGenerator::generate_native_entry

Reviewed-by: aph, shade

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 41 new commits pushed to the master branch:

• 8af5943: 8370334: javadoc NPE with "import module" statement
• 26460b6: 8353835: Implement JEP 500: Prepare to Make Final Mean Final
• 8cdfec8: 8371852: Shenandoah: Unused ShenandoahFreeSet::_allocated_since_gc_start field
• ... and 38 more: https://git.openjdk.org/jdk/compare/10f262a6ad9a6e89cd79409c5e1a3f7efda76928...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@theRealAph, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@bsdkurt 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.

[bsdkurt]

Here is a gdb session on OpenBSD/aarch64 without this patch showing the SEGFAULT where the ldr(rscratch2, rscratch2) instruction is attempting to load the initial instructions (paciasp) for throw_unsatisfied_link_error into x9:

Core was generated by `javac'.
Program terminated with signal SIGABRT, Aborted.
#0  thrkill () at /tmp/-:3

warning: 3	/tmp/-: No such file or directory
[Current thread is 1 (process 121574)]
(gdb) bt
#0  thrkill () at /tmp/-:3
#1  0x000000190c19873c in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51
#2  0x00000019249f1470 [PAC] in os::abort (dump_core=true, siginfo=<optimized out>, context=<optimized out>) at /home/truk/jdk/jdk/src/hotspot/os/posix/os_posix.cpp:2226
#3  0x0000001924f43660 [PAC] in VMError::report_and_die (id=id@entry=11, message=<optimized out>, message@entry=0x1911d160d0 "", detail_fmt=<optimized out>, detail_args=..., 
    thread=<optimized out>, pc=0x19c269d9b8 ")\001@\371_\001\t\353A\004", siginfo=0x1911d16648, context=0x1911d16528, filename=0x0, lineno=0, size=0)
    at /home/truk/jdk/jdk/src/hotspot/share/utilities/vmError.cpp:1992
#4  0x0000001924f42df0 [PAC] in VMError::report_and_die (thread=0x1931a44800, sig=0, sig@entry=11, pc=0x0, siginfo=0x65, context=0x31, detail_fmt=0x0)
    at /home/truk/jdk/jdk/src/hotspot/share/utilities/vmError.cpp:1631
#5  0x0000001924f43d50 [PAC] in VMError::report_and_die (thread=<optimized out>, sig=11, pc=<optimized out>, siginfo=<optimized out>, context=<optimized out>)
    at /home/truk/jdk/jdk/src/hotspot/share/utilities/vmError.cpp:1651
#6  0x0000001924d0e098 [PAC] in JVM_handle_bsd_signal (sig=11, info=0x1911d16648, ucVoid=0x1911d16528, abort_if_unrecognized=1)
    at /home/truk/jdk/jdk/src/hotspot/os/posix/signals_posix.cpp:652
#7  <signal handler called>
#8  0x00000019c269d9b8 in ?? ()
#9  0x00000019c269ae70 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) frame 8
#8  0x00000019c269d9b8 in ?? ()
(gdb) x/i $pc
=> 0x19c269d9b8:	ldr	x9, [x9]
(gdb) x/i $x9
   0x1924b3fc08 <throw_unsatisfied_link_error(JNIEnv*, ...)>:	paciasp
(gdb) x/x $x9
0x1924b3fc08 <throw_unsatisfied_link_error(JNIEnv*, ...)>:	0xd503233f
(gdb) x/i $x10
   0x192aaad88c <Java_java_lang_System_registerNatives>:	bti	c

[mlbridge]

Webrevs

• 00: Full (e5c2e609)

[dean-long]

It looks like RISCV is broken in the same way.

[dean-long]

According to InterpreterRuntime::prepare_native_call(), if there is a signal handler, which is checked first, then there should be a native function. So I wonder if we can remove the check for the native function from all CPU ports.

[theRealAph]

Ah yes, I see.

The mistake was mine: I thought __ cmpptr(rax, unsatisfied.addr(), rscratch1) meant
__ cmpptr(rax, unsatisfied, rscratch1). In other words, I missed the significance of .addr().

[RealFYang]

It looks like RISCV is broken in the same way.

Fired new JBS for RISC-V: https://bugs.openjdk.org/browse/JDK-8371966

[bsdkurt]

@theRealAph Thank you for the review.

/integrate

[openjdk]

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

[shipilev]

Looks reasonable.

[shipilev]

I did also wonder how it is not breaking now with uninitialized native entries. But I see we are doing this init as part of signature handler resolution:

jdk/src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp

Lines 1323 to 1334 in 54893dc

	// get signature handler
	{
	Label L;
	__ ldr(t, Address(rmethod, Method::signature_handler_offset()));
	__ cbnz(t, L);
	__ call_VM(noreg,
	CAST_FROM_FN_PTR(address,
	InterpreterRuntime::prepare_native_call),
	rmethod);
	__ ldr(t, Address(rmethod, Method::signature_handler_offset()));
	__ bind(L);
	}

-- before we hit this block. So, as @dean-long says, maybe we do not need this native method entry check at all. But this fix is fine to unbreak BSD/AArch64 alone.

[shipilev]

/sponsor

[openjdk]

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

• 0b3df48: 8372048: Performance improvement on Linux remote desktop
• d2926df: 8371649: ZGC: AArch64: redundant OrderAccess::fence in ZBarrierSetAssembler::patch_barrier_relocation
• 54893dc: 8371985: Parallel: Move should_attempt_scavenge to ParallelScavengeHeap
• ... and 67 more: https://git.openjdk.org/jdk/compare/10f262a6ad9a6e89cd79409c5e1a3f7efda76928...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @bsdkurt Pushed as commit ae4d9c2.

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

[bsdkurt]

@shipilev Thank you for the review and sponsor. I was wondering why this issue had not been discovered previously. It does appear that the check against unsatisfied link may be unnecessary.