Re: RFR: 8330171: Lazy W^X switch implementation

2024-04-23 Thread Sergey Nazarkin
On Fri, 19 Apr 2024 07:44:17 GMT, Richard Reingruber  wrote:

>> An alternative for preemptively switching the W^X thread mode on macOS with 
>> an AArch64 CPU. This implementation triggers the switch in response to the 
>> SIGBUS signal if the *si_addr* belongs to the CodeCache area. With this 
>> approach, it is now feasible to eliminate all WX guards and avoid 
>> potentially costly operations. However, no significant improvement or 
>> degradation in performance has been observed.  Additionally, considering the 
>> issue with AsyncGetCallTrace, the patched JVM has been successfully operated 
>> with [asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and 
>> [async-profiler](https://github.com/async-profiler/async-profiler). 
>> 
>> Additional testing:
>> - [x] MacOS AArch64 server fastdebug *gtets*
>> - [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4*
>> - [ ] Benchmarking
>> 
>> @apangin and @parttimenerd could you please check the patch on your 
>> scenarios??
>
> What about granting `WXWrite` only if the current thread is in 
> `_thread_in_vm`?
> That would be more restrictive and roughly equivalent how it currently works. 
> Likely there are some places then that should be granted `WXWrite` eagerly 
> because they need `WXWrite` without `_thread_in_vm`. E.g. the JIT compiler 
> threads should have `WXWrite` and never `WXExec` (I assume) which should be 
> checked in the signal handler.

The patch doesn't protect against native agents, as this is obviously 
impossible. The current code doesn't do that either.  For the bytecode, it 
doesn't prevent the attacker from abusing unsafe api to modify code cache. 
However unsafe functions are already considered "safe" and we proactively 
enable WXWrite as well as move thread to `_thread_in_vm` state (@reinrich). 
JITed code can't write to the cache either with or without the patch.  

I totally get the sense of loss of security. But is this really the case?

-

PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2071988941


Re: RFR: 8330171: Lazy W^X switch implementation

2024-04-15 Thread Sergey Nazarkin
On Mon, 15 Apr 2024 08:33:25 GMT, Bernhard Urban-Forster  
wrote:

> do you have numbers on how many transitions are done with your PR vs. the 
> current state when running the same program?

With just simple **java -version** it is ~180 vs ~9500 (new vs old),  for 
**java -help** ~1120 vs ~86300. For the applications the ration is about the 
same.

-

PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2057280998


Re: RFR: 8330171: Lazy W^X switch implementation

2024-04-12 Thread Sergey Nazarkin
On Fri, 12 Apr 2024 15:26:03 GMT, Andrew Haley  wrote:

>> Hello Sergey.
>> W^X mode was initially forced by Apple to prevent writing to executable 
>> memory, as a security feature.
>> This change just eliminates this security feature at all, doesn't it ?
>> Basically: "want to write to Executable memory ? ok, here you go"
>
>> Hello Sergey. W^X mode was initially forced by Apple to prevent writing to 
>> executable memory, as a security feature. This change just eliminates this 
>> security feature at all, doesn't it ? Basically: "want to write to 
>> Executable memory ? ok, here you go"
> 
> Yes @VladimirKempik, you are right. No, we should not do this.
> 
> Instead, when we enter the VM we could track the current state of W^X and 
> whenever we enter a block that needs to write into code space we would set W 
> if needed. When we leave the VM or when we call back into Java we would set 
> X, if needed. The cost of doing this would be small, but we'd have to find 
> all the blocks that need to write into code space. This might be more effort 
> than we want to make, though.
> 
> So where would be need to make the transitions to W? At a guess, whenever we 
> start assembling something, and in all of the methods in 
> nativeInst_aarch64.?pp, and in class Patcher. And to X, in the call stub and 
> a few other places.
> 
> That would minimize the transitions exactly to the set of places we actually 
> need.

Thanks @theRealAph, @VladimirKempik
> Instead, when we enter the VM we could track the current state of W^X and 
> whenever we enter a block that needs to write into code space we would set W 
> if needed. When we leave the VM or when we call back into Java we would set 
> X, if needed. The cost of doing this would be small, but we'd have to find 
> all the blocks that need to write into code space. This might be more effort 
> than we want to make, though.

It is the way in which it is implemented in the current code.  Unfortunately, 
it is hardly maintainable solution that suffers from issues like [1-5].   

As I understand it, your concern is that the implementation doesn't prevent 
rogue from writing to the code cache with some some unsafe api? 

1. https://bugs.openjdk.org/browse/JDK-8302736
2. https://bugs.openjdk.org/browse/JDK-8327990
3. https://bugs.openjdk.org/browse/JDK-8327036
4. https://bugs.openjdk.org/browse/JDK-8304725
5. https://bugs.openjdk.org/browse/JDK-8307549

-

PR Comment: https://git.openjdk.org/jdk/pull/18762#issuecomment-2052164890


RFR: 8330171: Lazy W^X swtich implementation

2024-04-12 Thread Sergey Nazarkin
An alternative for preemptively switching the W^X thread mode on macOS with an 
AArch64 CPU. This implementation triggers the switch in response to the SIGBUS 
signal if the *si_addr* belongs to the CodeCache area. With this approach, it 
is now feasible to eliminate all WX guards and avoid potentially costly 
operations. However, no significant improvement or degradation in performance 
has been observed.  Additionally, considering the issue with AsyncGetCallTrace, 
the patched JVM has been successfully operated with 
[asgct_bottom](https://github.com/parttimenerd/asgct_bottom) and 
[async-profiler](https://github.com/async-profiler/async-profiler). 

Additional testing:
- [x] MacOS AArch64 server fastdebug *gtets*
- [ ] MacOS AArch64 server fastdebug *jtreg:hotspot:tier4*
- [ ] Benchmarking

@apangin and @parttimenerd could you please check the patch on your scenarios??

-

Commit messages:
 - Fix non-macos builds
 - Revert "8304725: AsyncGetCallTrace can cause SIGBUS on M1"
 - Remove ThreadWXEnable guard
 - Lazy W^X state switch

Changes: https://git.openjdk.org/jdk/pull/18762/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18762=00
  Issue: https://bugs.openjdk.org/browse/JDK-8330171
  Stats: 325 lines in 46 files changed: 17 ins; 305 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/18762.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18762/head:pull/18762

PR: https://git.openjdk.org/jdk/pull/18762


Integrated: 8312246: NPE when HSDB visits bad oop

2023-07-20 Thread Sergey Nazarkin
On Tue, 18 Jul 2023 16:47:00 GMT, Sergey Nazarkin  wrote:

> Hi!
> 
> This is a minor HSDB fix. It prints "bad oop" instead of throwing an NPE 
> message to the console when a corrupted object is found.

This pull request has now been integrated.

Changeset: a7427678
Author:Sergey Nazarkin 
Committer: Yuri Nesterenko 
URL:   
https://git.openjdk.org/jdk/commit/a7427678e160bf54c57d5bec80650b053dfc9e9a
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod

8312246: NPE when HSDB visits bad oop

Reviewed-by: cjplummer, sspitsyn

-

PR: https://git.openjdk.org/jdk/pull/14922


Re: RFR: 8312246: NPE when HSDB visits bad oop

2023-07-20 Thread Sergey Nazarkin
On Tue, 18 Jul 2023 16:47:00 GMT, Sergey Nazarkin  wrote:

> Hi!
> 
> This is a minor HSDB fix. It prints "bad oop" instead of throwing an NPE 
> message to the console when a corrupted object is found.

Chris, Serguei,  thank you for the review.

-

PR Comment: https://git.openjdk.org/jdk/pull/14922#issuecomment-1643566646


RFR: 8312246: NPE when HSDB visits bad oop

2023-07-18 Thread Sergey Nazarkin
Hi!

This is a minor HSDB fix. It prints "bad oop" instead of throwing an NPE 
message to the console when a corrupted object is found.

-

Commit messages:
 - 8312246: NPE when HSDB visits bad oop

Changes: https://git.openjdk.org/jdk/pull/14922/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14922=00
  Issue: https://bugs.openjdk.org/browse/JDK-8312246
  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14922.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14922/head:pull/14922

PR: https://git.openjdk.org/jdk/pull/14922