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

Implement W^X-compatible JIT. Hopefully makes JIT work on iOS again #8937

Merged
merged 9 commits into from
Aug 28, 2016

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Aug 28, 2016

Will hopefully fix issue #8122, with a lot less of a speed hit than @vit9696's patch (thanks for that!)

For now, block linking is disabled in this mode. Would like confirmation that this works on iOS before doing the work to enable it.

To play around with this mode on other platforms, simply change PlatformIsWXExclusive in MemoryUtil.cpp to always return true. Speed hit appears to be minimal, disregarding the loss of block linking.

Please let me know if this works or not in the comments. I've enabled the mode and tested on Android and Windows, and those are fine.

@seadil
Copy link

seadil commented Aug 28, 2016

@hrydgard You forgot this line in main.mm, otherwise Xcode throws an error message:
#import <sys/syscall.h>

PanicAlert("Bad memory protect : W^X is in effect, can't both write and exec");
}
// Note - both VirtualProtect and mprotect will affect the full pages containing the requested range,
// so no need to round ptr and size down/up.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's confusing to say this and then round anyway (which we're assuming is necessary on iOS?)

I wonder if this helps anything on the Galaxy S7 (if the other change is turned off.)

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I thought it was, but turned out it wasn't, or at least it worked after I added the rounding...

@vit9696
Copy link
Contributor

vit9696 commented Aug 28, 2016

@hrydgard That looks pretty good, though I cannot test right now.
Some parts felt a bit strange to me but I am not familiar with the codebase and may be mistaken.

  • Why would a realign be needed here or here? If the memory is allocated in pages I'd expect the pointer to be aligned already? Or it is continuously changed by other instructions?
  • This comment is not really accurate: it's necessary for any unjailbroken iOS, and jailbroken starting with jailbroken iOS 9.0 on arm64. I do agree with the intent, however, and am just complaining for nothing :)
  • Page size function is invalid for native iOS 9.0 on arm64 (where the page is 16384). I could have missed iOS specific one somewhere, but from what I can tell there is none. Consider using PAGE_SIZE.

@hrydgard
Copy link
Owner Author

@vit9696 The AlignCode16 are not really relevant for this pull request, but there is a recommendation to start functions at 16-byte boundaries on some AMD CPUs especially so why not.

OK, I'll fix the comment and change to PAGE_SIZE.

return 4096;
// For reference, here's how to check:
// if (sys_info.dwPageSize == 0)
// GetSystemInfo(&sys_info);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, we're doing that already:

#define MEM_PAGE_MASK (uintptr_t)(sys_info.dwPageSize - 1)

-[Unknown]

@unknownbrackets unknownbrackets merged commit 933949d into master Aug 28, 2016
@vit9696
Copy link
Contributor

vit9696 commented Aug 28, 2016

ow, @unknownbrackets are you sure this builds on iOS? Currently cloning and will check but I fear that not all the headers are included still.

@hrydgard
Copy link
Owner Author

hrydgard commented Aug 28, 2016

@vit9696 let's just fix it if there are problems. I'll be around for a few hours so just let me know

@seadil
Copy link

seadil commented Aug 28, 2016

Common/MemoryUtil.cpp:337:9: Use of undeclared identifier 'PAGE_SIZE'

@vit9696
Copy link
Contributor

vit9696 commented Aug 28, 2016

I guess this is the right fix :)

@hrydgard
Copy link
Owner Author

Pushed a fix

@vit9696
Copy link
Contributor

vit9696 commented Aug 28, 2016

Thanks =)

@hrydgard hrydgard deleted the memprot-w-xor-x branch August 28, 2016 17:22
@Anuskuss
Copy link
Contributor

Anuskuss commented Sep 1, 2016

@hrydgard I can't really read code that well so bear with me:
We use PlatformIsWXExclusive() to determinante if we cannot use the normal JIT. So the logic should be

if (( iOS < 9 && isJailbroken() ) || ( iOS == 9 && isJailbroken() && arch == armv7 )) { canUseJIT = true }

How do we test if the device is jailbroken? There are several methods for that, did you find a reliable one?
And will the new JIT engine work with older devices with older firmwares (like the iPhone 3GS @unknownbrackets has)?

Also should we include a JIT [W^X] (slower) as an interpreter option for debugging reasons, or would this be too confusing?

Also look at this PDF from the Black Hat 2016. Will the new security changes affect our approach in iOS 10?

Changes to JSCore (Safari)

“Hardened WebKit JIT Mapping” protects the Safari Browser on iOS. Most other devices, including Amazon Kindle, compile JavaScript code in beforehand. But iOS assures quick JavaScript execution by compiling JavaScript just before execution, (JIT). But there is a downside to JIT implementation- it demands far much less strict code-signing, thereby making the system vulnerable.
Apple addressed the issue in the latest security measures on iOS. Apple patched the JIT security concerns by dedicating specific areas of application memory to particular tasks. Unlike before, JavaScript is now not allowed to run code in raw data storage in iOS 9 and previous versions; raw data storage is highly vulnerable to attacks. On the iOS 10, the compiled JavaScript code runs on execution-particular memory. Once the code is running on this memory, it cannot be changed, thus preventing on-the-fly code alterations attacks.

Thanks for listening.

@hrydgard
Copy link
Owner Author

hrydgard commented Sep 1, 2016

I don't think the W^X compatibility slowed it down much, so for safety and simplicity, we now always use W^X compatible JIT on iOS, even if we could do it the old style.

Therefore, the check is now simply:

canIosUseJit = true;

as W^X should work on all iOS devices.

We'll see about iOS 10.

About the BlackHat thing, we give ourselves debugger permissions so we don't need to concern ourselves with that. App Store apps can't do that but we're not on the app store.

Including a W^X JIT option on other platforms doesn't make much sense IMO - while it's useful for developers for debugging, you can just change a single line of code to get into that mode anyway.

@Anuskuss
Copy link
Contributor

Anuskuss commented Sep 1, 2016

@hrydgard Thanks for answering my questions!

Do you have actual numbers on how the W^X JIT compares to the old one? I would really like to help and test it for you (iPhone 6 - 8.4) but I don't have access to a computer.

I know this may be a lot to ask but if you could provide me with a compiled PPSSPP or even write a quick way that let's me choose both cores, and push it to the Cydia buildbot, I could quickly compare some games and tell you if the old JIT was even worth it (on iOS).

But on the other hand, I'm pretty sure you're going to find a better tester than myself 😁

@hrydgard
Copy link
Owner Author

hrydgard commented Sep 1, 2016

No numbers, but there should only be a difference when it's compiling new code, which mostly happens when you enter new levels or do new things. We have to do some extra memory operations to un-write-protect and then write protect the generated code in this case. I'd expect possibly very small stutters when you enter a new level and then no difference to the old JIT.

@unknownbrackets
Copy link
Collaborator

Just to say, the 3GS couldn't even run most easy/simple games at full speed with frameskip iirc. Also, last I checked my iOS version was too low to run PPSSPP anymore, but not a huge loss.

The performance impact is likely to vary by operating system. hrydgard could test on Windows, let's say, or Android, but it might not reflect the performance impact on iOS.

So someone with an iOS device that doesn't need W^X would likely need to perform the tests, but this would likely require writing code to get a good sense of the compilation performance impact (since runtime won't be affected, and loading times are harder to measure.)

-[Unknown]

Vogtinator added a commit to nspire-emus/firebird that referenced this pull request Dec 2, 2016
- Only required for 64bit OSs, it's expensive
- hrydgard/ppsspp#8937
Vogtinator added a commit to nspire-emus/firebird that referenced this pull request Dec 2, 2016
- Only required for 64bit OSs, it's expensive
- hrydgard/ppsspp#8937
Vogtinator added a commit to nspire-emus/firebird that referenced this pull request Dec 2, 2016
- Only required for 64bit OSs, it's expensive
- hrydgard/ppsspp#8937
Vogtinator added a commit to nspire-emus/firebird that referenced this pull request Dec 3, 2016
- Only required for 64bit OSs, it's expensive
- hrydgard/ppsspp#8937
Vogtinator added a commit to nspire-emus/firebird that referenced this pull request Dec 3, 2016
- Required on non-JB iOS and jailbroken 64bit iOS
- hrydgard/ppsspp#8937
adriweb pushed a commit to nspire-emus/firebird that referenced this pull request Dec 4, 2016
- Required on non-JB iOS and jailbroken 64bit iOS
- hrydgard/ppsspp#8937
@adriweb
Copy link

adriweb commented Dec 4, 2016

( Sorry for the GitHub noise regarding commit references... We've finally managed to implement W^X JIT on non-JB iOS as well on our project :) )

@hrydgard
Copy link
Owner Author

hrydgard commented Dec 4, 2016

That's no problem, that kind of commit reference noise doesn't send me notifications :) Nice job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants