Fixed segfault on ZTS when one thread attempts to execute JIT code while another is compiling code

[dktapps]

this happens when one thread tries to compile a script while another thread is already executing some code in the JIT region. The next time thread1 attempted to execute code in the JIT region, it would crash if thread2 still retained protection because the region didn't have exec permissions.

This was discovered while updating my fork of ext-pthreads to PHP 8.0. It started with an observation that my tests wouldn't fault when using opcache.jit_debug=256, which eventually led me to discover this code.

[nikic]

@dstogov Could you please take a look at this one?

I'm a bit concerned that this will cause issues with some "security" settings that don't allow mapping with write and execute at the same time.

[dktapps]

FWIW, the JIT region is already mprotect()'d with RWX on zend_jit_startup():

php-src/ext/opcache/jit/zend_jit.c

Line 4234 in 496e474

if (mprotect(dasm_buf, dasm_size, PROT_READ | PROT_WRITE | PROT_EXEC) != 0) {

php-src/ext/opcache/jit/zend_jit.c

Line 4246 in 496e474

if (!VirtualProtect(dasm_buf, dasm_size, PAGE_EXECUTE_READWRITE, &old)) {

So this shouldn't cause any new security permission issues that weren't already present.

[nikic]

@dktapps Both of those only happen if you enable certain jit.debug settings. In a non-debugging configuration, I think write and execute are always exclusive.

[dktapps]

My bad, I didn't notice that. The conditions here are flipped but I assumed they were the same.

If this change is a problem, it's possible to limit it to just ZTS to minimize the impact (NTS is not affected anyway).

[dktapps]

I've pushed a commit to limit the impact to ZTS only.

The only other solution I can see for this problem on ZTS is to copy JIT code from the JIT buffer into thread-local memory on ZTS, where it can be protected with R-X without the possibility for faults. Then the JIT buffer can be protected with RW- without worrying about other threads. But that's more costly and probably more complex to implement (I wouldn't know where to start).

[dktapps]

Another possible solution I just thought of would be to only unprotect the memory region from dasm_ptr[0] to the end of the buffer as RW- (instead of unprotecting the entire buffer including precompiled code), and leave the rest of the buffer as R-X. This would require that the buffer size only increases and that old code doesn't get updated. I don't know the JIT internals well enough to know if that's a viable option (at the least, it looks like dasm_ptr would have to be at the end of the buffer, instead of at the start).

[nikic]

@dktapps I think limiting this to ZTS is probably good enough, as most people don't use ZTS builds. I'm not sure partial unprotection is possible (in any case, it would only work at page granularity, which would waste memory).

[dstogov]

@dktapps I think the current implementation is good enough. It's pity, it reduces security for ZTS build, but I don't see another simple solution. Limiting the protected region won't work, because JIT might need to patch existing code.

For a better solution, we may think about usage of Intel memory protection keys (this is not portable, see https://www.usenix.org/system/files/atc19-park-soyeon.pdf), or moving JIT compiler into separate process (this is going to be a pain for Windows/ASLR).

[dktapps]

Thanks guys. Could we get this backported onto 8.0? nvm, it already is.