JIT: Add IBT support #8636

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.

Sign up for GitHub

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

Jump to bottom

Merged

dstogov merged 1 commit into php:master from chen-hu-97:jit

May 31, 2022

Contributor

chen-hu-97 commented May 26, 2022

Indirect Branch Tracking (IBT) is part of Intel's Control-Flow
Enforcement Technology (CET). IBT is hardware based, forward edge
Control-Flow-Integrity mechanism where any indirect CALL/JMP must target
an ENDBR instruction or suffer #CP.

This commit adds IBT support for JIT:

Add endbr32/64 instruction in Dynasm.
Insert endbr32/64 in indirect branch target for jitted code.

gcc support CET since v8.1 and set it to default since gcc 11. With this
commit, endbr is inserted in jitted code if PHP is compiled with "gcc
-fcf-protection=full/branch".

Signed-off-by: Chen, Hu hu1.chen@intel.com

Contributor Author

chen-hu-97 commented May 26, 2022

This patch passes test on a full CET environment/stack at a CET enabled Intel CPU. Without the patch, JIT test will fail because IBT violation.

Component	Upstreamed?
gcc	Y
glibc	Y
Linux Kernel: IBT support for userspace	N

$ make test TESTS="ext/opcache/tests/jit"
=====================================================================
Number of tests :  316               313
Tests skipped   :    3 (  0.9%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Tests passed    :  313 ( 99.1%) (100.0%)
---------------------------------------------------------------------
Time taken      :    8 seconds
=====================================================================

saundefined added the Category: JIT label

nikic requested a review from dstogov

May 26, 2022 15:52

dstogov requested changes

View reviewed changes

ext/opcache/jit/zend_jit_x86.dasc Outdated

@@ @@ -3044,6 +3086,7 @@ static int zend_jit_align_func(dasm_State **Dst) @@
               	track_last_valid_opline = 0;
               	jit_return_label = -1;
               	|.align 16
+              	|	ENDBR

Member

dstogov May 26, 2022

It's better to move this from zend_jit_align_func() to zend_jit_prologue()

Contributor Author

chen-hu-97 May 27, 2022

Done.

ext/opcache/jit/zend_jit_x86.dasc Outdated

@@ @@ -3065,6 +3108,7 @@ static int zend_jit_prologue(dasm_State **Dst) @@
               static int zend_jit_label(dasm_State **Dst, unsigned int label)
               {
               	|=>label:
+              	|	ENDBR

Member

dstogov May 26, 2022

We don't need this for each label, only for some that are followed by zend_jit_prologue()

Contributor Author

chen-hu-97 May 27, 2022

I add endbr in zend_jit_label to avoid "segment fault" (it's actually control protection in kernel) during debug. However, after move endbr from zend_jit_align_func() to zend_jit_prologue() as you suggested, the endbr in zend_jit_label looks useless because no error shown.
In following patchset rev, let's tentatively remove this.

ext/opcache/jit/zend_jit_x86.dasc Outdated

Comment on lines 3871 to 3875

+              		|	ENDBR
               		|	call ->context_threaded_call
               	} else {
               		const zend_op *next_opline = opline + 1;
+              		|	ENDBR

Member

dstogov May 26, 2022

Better to remove this. Now, I'm not sure if they may be necessary or not.
context threading is an experimental mechanism, but it never worked well.

Contributor Author

chen-hu-97 May 27, 2022

Removed.

ext/opcache/jit/zend_jit_x86.dasc Outdated

Comment on lines 2239 to 2221

               	|->one:
+              	|	ENDBR
               	|.dword 0, 0x3ff00000

Member

dstogov May 26, 2022

This one is definitely wrong.

Contributor Author

chen-hu-97 May 27, 2022

Done.

ext/opcache/jit/zend_jit_x86.dasc Outdated

@@ @@ -1690,6 +1700,7 @@ static void zend_jit_stop_reuse_ip(void) @@
               static int zend_jit_interrupt_handler_stub(dasm_State **Dst)
               {
               	|->interrupt_handler:
+              	|	ENDBR

Member

dstogov May 26, 2022

only few of the ENDBR inserted in stubs below are really necessary. Most of them just a helper functions that can't be called indirectly. Only counters need to be updated.

Contributor Author

chen-hu-97 May 27, 2022

Done.

ext/opcache/jit/zend_jit_x86.dasc

Comment on lines +1626 to +1635

+              |.macro ENDBR
+              ||#if defined (__CET__) && (__CET__ & 1) != 0
+              |	.if X64
+              |		endbr64
+              |	.else
+              |		endbr32
+              |	.endif
+              ||#endif
+              |.endmacro

Member

dstogov May 26, 2022

Can we enable/disable generation of endbr ar run-time? e.g. thorough new bit in opcache.jit directive?

Member

dstogov May 26, 2022

I'm not sure if this is really necessary...
Can you share some CET vs non-CET benchmarks results? (at least Zend/bench.php)

Contributor Author

chen-hu-97 May 27, 2022

My concern is that runtime enable/disable may cause inconsistency, i.e., jitted code has endbr and gcced code doesn't. Our current method mimics or aligns with the behavior of gcc who enable/disable generation of endbr at compile time.
GCC:
When compiled with gcc -fcf-protection=full (default) or gcc -fcf-protection=branch, the MACRO __CET__ is defined and endbr is inserted. It will also emit a property named IBT to ELF header.
Execution flow:
Once ld.so see IBT property in ELF header, it issues an arch_prctl() syscall to kernel to enable HW/CPU CET engine.
Benchmarks results:
I will run and share Google PKB benchmark result next week. Below are quick Zend/bench.php results:

CET. Binary has endbr and HW/CPU CET on.

$ ./sapi/cli/php Zend/bench.php
simple             0.026
simplecall         0.011
simpleucall        0.031
simpleudcall       0.026
mandel             0.098
mandel2            0.087
ackermann(7)       0.018
ary(50000)         0.003
ary2(50000)        0.002
ary3(2000)         0.033
fibo(30)           0.056
hash1(50000)       0.006
hash2(500)         0.007
heapsort(20000)    0.022
matrix(20)         0.021
nestedloop(12)     0.022
sieve(30)          0.012
strcat(200000)     0.004
------------------------
Total              0.485

Non-CET. Binary has no endbr and HW/CPU CET off

$ ./sapi/cli/php Zend/bench.php
simple             0.025
simplecall         0.011
simpleucall        0.030
simpleudcall       0.025
mandel             0.098
mandel2            0.090
ackermann(7)       0.020
ary(50000)         0.003
ary2(50000)        0.003
ary3(2000)         0.031
fibo(30)           0.056
hash1(50000)       0.006
hash2(500)         0.007
heapsort(20000)    0.022
matrix(20)         0.021
nestedloop(12)     0.022
sieve(30)          0.012
strcat(200000)     0.004
------------------------
Total              0.486

BTW, when HW/CPU CET engine is OFF, the endbr is regarded as nop. For this case, even the app has endbr inside, the perf impact is negligible.

chen-hu-97 force-pushed the jit branch from 25e9619 to 3517c8d Compare

May 27, 2022 12:21

ramsey added the Extension: opcache label

ramsey added this to the PHP 8.2 milestone

ramsey added the Waiting on Review label

dstogov reviewed

View reviewed changes

ext/opcache/jit/zend_jit_x86.dasc Outdated

		@@ -2334,7 +2344,6 @@ static int zend_jit_hybrid_loop_hot_counter_stub(dasm_State **Dst)
		}

		\|->hybrid_loop_hot_counter:

Member

dstogov May 30, 2022

You probably missed ENDBR here.
Personally, Now I would move ENDBR into zend_jit_hybrid_hot_counter_stub() and zend_jit_hybrid_trace_counter_stub(). I don't see any other problems.
Did you run tests? Everything is passed?
Please fix the last issue, and merge this or let me do this.

Contributor Author

chen-hu-97 May 31, 2022

Now I would move ENDBR into zend_jit_hybrid_hot_counter_stub() and zend_jit_hybrid_trace_counter_stub()

Done and it looks good.

Test: Passed
On a full CET environment/stack at CET enabled Intel CPU:
JIT test like make test TESTS="ext/opcache/tests/jit" passed. Without the patch, the test fails.
This patch doesn't incur addition fail in make test.

I think I don't have permission to merge?


          JIT: Add IBT support

a3e285f

Indirect Branch Tracking (IBT) is part of Intel's Control-Flow
Enforcement Technology (CET). IBT is hardware based, forward edge
Control-Flow-Integrity mechanism where any indirect CALL/JMP must target
an ENDBR instruction or suffer #CP.

This commit adds IBT support for JIT:
1. Add endbr32/64 instruction in Dynasm.
2. Insert endbr32/64 in indirect branch target for jitted code.

gcc support CET since v8.1 and set it to default since gcc 11. With this
commit, endbr is inserted in jitted code if PHP is compiled with "gcc
-fcf-protection=full/branch".

Signed-off-by: Chen, Hu <hu1.chen@intel.com>

chen-hu-97 force-pushed the jit branch from 3517c8d to a3e285f Compare

May 31, 2022 02:54

dstogov merged commit c1fcd45 into php:master

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request


          Revert "JIT: Add IBT support (php#8636)"

81ef13d

This reverts commit c1fcd45.

iluuu1994 mentioned this pull request

Fix asan jit #8711

Closed

iluuu1994 added a commit that referenced this pull request


          Revert "JIT: Add IBT support (#8636)"

3f557eb

This reverts commit c1fcd45.

Closes GH-8711

chen-hu-97 added a commit to chen-hu-97/php-src that referenced this pull request


          JIT: Add IBT support (php#8636)

f882325

Indirect Branch Tracking (IBT) is part of Intel's Control-Flow
Enforcement Technology (CET). IBT is hardware based, forward edge
Control-Flow-Integrity mechanism where any indirect CALL/JMP must target
an ENDBR instruction or suffer #CP.

This commit adds IBT support for JIT:
1. Add endbr32/64 instruction in Dynasm.
2. Insert endbr32/64 in indirect branch target for jitted code.

gcc support CET since v8.1 and set it to default since gcc 11. With this
commit, endbr is inserted in jitted code if PHP is compiled with "gcc
-fcf-protection=full/branch".

Signed-off-by: Chen, Hu <hu1.chen@intel.com>

Contributor Author

chen-hu-97 commented Jun 10, 2022 •

edited

Loading

Hi, @dstogov @iluuu1994, update some status here.
Looks like it's the conflict between JITTED code and "address sanitize". If I don't insert ENDBR before JITTED code (i.e., don't add ENDBR in zend_jit_prologue ), this issue is gone.

I can reproduce this locally:
$ ./buildconf --force && CC="gcc -fcf-protection=branch" ./configure 'CFLAGS=-fsanitize=undefined,address -DZEND_TRACK_ARENA_ALLOC' LDFLAGS=-fsanitize=undefined,address && make -j16
$ make test TESTS="ext/opcache/tests/jit"
Now we can see several failures. Just pick one failure case to debug as below:
$ gdb --args ./sapi/cli/php -d zend_extension=~/chen/php-src/modules/opcache.so -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.jit_buffer_size=128M -d opcache.jit=tracing -d opcache.jit_debug=0x139 ./ext/opcache/tests/jit/bug81226.php

I will learn "address sanitize" and then try to debug.

chen-hu-97 added a commit to chen-hu-97/php-src that referenced this pull request


          JIT: Add IBT support (php#8636)

5ba1959

Indirect Branch Tracking (IBT) is part of Intel's Control-Flow
Enforcement Technology (CET). IBT is hardware based, forward edge
Control-Flow-Integrity mechanism where any indirect CALL/JMP must target
an ENDBR instruction or suffer #CP.

This commit adds IBT support for JIT:
1. Add endbr32/64 instruction in Dynasm.
2. Insert endbr32/64 in indirect branch target for jitted code.

gcc support CET since v8.1 and set it to default since gcc 11. With this
commit, endbr is inserted in jitted code if PHP is compiled with "gcc
-fcf-protection=full/branch".

Signed-off-by: Chen, Hu <hu1.chen@intel.com>

chen-hu-97 mentioned this pull request

JIT: Add IBT support #8774

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: JIT Extension: opcache Waiting on Review