Fixed bug #8338 (Intel CET is disabled unintentionally since PHP-8.1.0) #8339

chen-hu-97 · 2022-04-11T08:05:08Z

Intel Control Flow Enforcement Technology (CET) is enabled by default
since gcc v8.1. With CET, gcc emits following processor-specific program
property types in .note.gnu.property section:

Indirect Branch Tracking (IBT)
Shadow Stack (SHSTK)

Such properties are there before PHP-8.1.0.
$ sudo readelf -n sapi/cli/php
Displaying notes found in: .note.gnu.property
Owner Data size Description
GNU 0x00000020 NT_GNU_PROPERTY_TYPE_0
Properties: x86 feature: IBT, SHSTK
x86 ISA needed: x86-64-baseline

However, the properties are missing since PHP-8.1.0.
$ sudo readelf -n sapi/cli/php
Displaying notes found in: .note.gnu.property
Owner Data size Description
GNU 0x00000010 NT_GNU_PROPERTY_TYPE_0
Properties: x86 ISA needed: x86-64-baseline

This is caused by commit c276c16 "Implement Fibers" which introduces
some assembly files such as jump_x86_64_sysv_elf_gas.S and
make_x86_64_sysv_elf_gas.S. The object files of such assembly miss
.note.gnu.property section and thus the final output files miss it too.

Fix this via adding linker option.

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

cmb69 · 2022-04-11T15:36:40Z

cc @trowski

trowski · 2022-04-12T15:29:06Z

I know nothing about Intel CET, but at a glance this seems to make sense.

cmb69 · 2022-04-26T11:49:08Z

Zend/zend_fibers.c

@@ -141,7 +141,7 @@ typedef struct {

 /* These functions are defined in assembler files provided by boost.context (located in "Zend/asm"). */
 extern void *make_fcontext(void *sp, size_t size, void (*fn)(boost_context_data));
-extern boost_context_data jump_fcontext(void *to, zend_fiber_transfer *transfer);
+extern boost_context_data jump_fcontext(void *to, zend_fiber_transfer *transfer) __attribute__ ((__indirect_return__));


__attribute__ ((__indirect_return__)) is not necessarily supported (see the failing CI builds), and as such must not be applied uncoditionally.

attribute ((indirect_return)) is relatively new gcc attreibute. So I need figure out other fix.
How about insert one line inline assembly after jump_fcontext(). Maybe that's too ugly?

Maybe it is okay to only enable shstk if __attribute__((indirect_return)) (or equivalent) is supported by the compiler. Those interested in best security practices would need to use a newish compiler version, but that may be okay.

I wouldn't know if something like __attribute__((indirect_return)) is supported by MSVC. CET support is generally available there.

Based on my current understanding (maybe it's not correct), there are no such attribute for shadow stack.
Basically, the compiler such as gcc take care of shadow stack. No special treatment is needed for application.

But Fiber or applications use makecontext() are exception. They make context (i.e., they maintain stack) by themselves instead of gcc/libc. So they also need creating a shadow stack which requires kernel support. Such kernel patch is under review.

cmb69 · 2022-04-26T11:50:39Z

Zend/asm/jump_x86_64_sysv_elf_gas.S

@@ -30,6 +30,7 @@
 .type jump_fcontext,@function
 .align 16
 jump_fcontext:
+    endbr64


If I'm not mistaken, these assembler files are re-used from the boost libraries, and as such it would probably be a good idea to provide these modifications upstream.

Yes, this file is imported from boostorg/context, so please do provide any changes here upstream as well.

Sure, I will try to provide CET support to boostorg/context as well and then echo here.

chen-hu-97 · 2022-04-26T13:29:57Z

I know nothing about Intel CET, but at a glance this seems to make sense.

Hi @trowski,
Intel CET is a HW security feature in some Intel and AMD CPUs, for example, since Intel TigerLake and Sapphire Rapids. It includes:

Indirect Branch Tracking (IBT)
Any indirect CALL/JMP must target an ENDBR instruction or suffer Control Protection (CP)
Shadow Stack (SHSTK)
When func return, it pops out the return address from stack and shadow stack. If the addresses are different, contol protection is raised.

Below supports are needed to enable CET:

gcc/libc/linker already support this since gcc v8.1
Kernel.
Kernel support for kernel IBT has been merged in 5.18 RC2
Kernel support for user space IBT is still on the way of upstream. (Php is in this category)
Kernel support for SHSTK is under review.

@hjl-tools has a nice explanation for CET in LPC. https://lpc.events/event/2/contributions/147/attachments/72/83/CET-LPC-2018.pdf

chen-hu-97 · 2022-04-29T02:48:17Z

Thanks community for the detailed review. Now the reviews focus on Fiber part support for "indirect branch tracking".
For JIT part, I add new instructions to Dynasm https://luajit.org/git/luajit.git. Maybe it would probably be a good idea to provide these modifications upstream as well?

devnexen · 2022-04-29T04:20:03Z

Would it be better to do separate PR at this point tough ?

mvorisek · 2022-05-04T07:32:41Z

can this target PHP 8.1 and is the Intel CET security feature supported also with MSVC?

cmb69 · 2022-05-04T08:59:12Z

[…] is the Intel CET security feature supported also with MSVC?

See https://docs.microsoft.com/en-us/cpp/build/reference/cetcompat?view=msvc-170.

mvorisek · 2022-05-04T09:15:29Z

[…] is the Intel CET security feature supported also with MSVC?

See https://docs.microsoft.com/en-us/cpp/build/reference/cetcompat?view=msvc-170.

added to #8392 to see if tests pass

can the compiler default flags be specified somewhere so pure ./configure.bat & nmake will have them set by default?

cmb69 · 2022-05-04T09:28:35Z

/CETCOMPAT needs to be added to buildconf.js/confutils.js (and is a linker flag, by the way).

cmb69 · 2022-05-04T10:03:06Z

The following patch should do for VS 2019 and up:

 win32/build/confutils.js | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/win32/build/confutils.js b/win32/build/confutils.js
index 9bf1bf2814..1571d1964f 100644
--- a/win32/build/confutils.js
+++ b/win32/build/confutils.js
@@ -3401,10 +3401,13 @@ function toolset_setup_common_ldlags()
 	ADD_FLAG("PHP_LDFLAGS", "/nodefaultlib:libcmt");
 
 	if (VS_TOOLSET) {
-		if (VCVERS >= 1900) {
-			if (PHP_SECURITY_FLAGS == "yes") {
+		if (PHP_SECURITY_FLAGS == "yes") {
+			if (VCVERS >= 1900) {
 				ADD_FLAG('LDFLAGS', "/GUARD:CF");
 			}
+			if (VCVERS >= 1920) {
+				ADD_FLAG('LDFLAGS', "/CETCOMPAT");
+			}
 		}
 		if (PHP_VS_LINK_COMPAT != "no") {
 			// Allow compatible IL versions, do not require an exact match.

mvorisek · 2022-05-04T12:10:33Z

this PR should target PHP 8.1 (or maybe 8.0, but the desc claims the support is broken since PHP 8.1)

chen-hu-97 · 2022-05-05T12:03:13Z

this PR should target PHP 8.1 (or maybe 8.0, but the desc claims the support is broken since PHP 8.1)

I thinks it targets PHP8.1.

If the user want to compile the PHP to enable CET, he should specify gcc "-fcf-protection" option explicitly. Below is an example:
$ ./buildconf --force && CC="gcc -fcf-protection" ./configure && make -j64

However, PHP 8.1 introduces some hand-coded assemblers (from Fiber) which ignores/breaks this gcc option. With this PR, if "-fcf-protection" is detected, linker options will be added to make sure the final PHP bin is CET enabled.

chen-hu-97 · 2022-05-17T07:23:40Z

Per @devnexen's comments, I separate the patches. Now this PR focus on Fiber part. It's based on merged PR in boost: boostorg/context#199.

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

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

$ make test TESTS="Zend/tests/fibers"
=====================================================================
Number of tests : 60 60
Tests skipped : 0 ( 0.0%) --------
Tests warned : 0 ( 0.0%) ( 0.0%)
Tests failed : 0 ( 0.0%) ( 0.0%)
Tests passed : 60 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken : 1 seconds
=====================================================================

Zend/zend_fibers.c

trowski

Other than the minor nit about attribute placement, this LGTM.

ramsey · 2022-05-23T15:46:05Z

If this is a bug in PHP 8.1, please change the base branch to PHP-8.1. Thanks!

chen-hu-97 · 2022-05-24T10:33:51Z

If this is a bug in PHP 8.1, please change the base branch to PHP-8.1. Thanks!

Hi @ramsey, I saw this issue in fiber since PHP 8.1. It's also in current master. For such case, shall we change base branch to 8.1?

ramsey · 2022-05-24T14:01:38Z

Current master is 8.2.0-dev, so if it exists in both PHP-8.1 and master, then you need to target PHP-8.1 and we will merge it up into master.

If the bug also exists in PHP-8.0, then you should target PHP-8.0 instead.

cmb69 · 2022-05-24T14:21:45Z

Just to be sure, adding __attribute__((__indirect_return__)) wouldn't change the function's ABI, or would it?

chen-hu-97 · 2022-05-25T03:05:40Z

Just to be sure, adding __attribute__((__indirect_return__)) wouldn't change the function's ABI, or would it?

"No, it won't change ABI."
I got feedback from the author of the indirect_return in gcc.

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 fiber: 1. Add endbr32/64 in assembly 2. Inform compiler jump_fcontext may return via indirect branch Furthermore: gcc support CET since v8.1 and set it to default since gcc 11. That is, the ELF header of sapi/cli/php has a property named IBT. However, such property is lost since PHP8.1 because the assembly introduced by Fiber. This commit also fixes this. Signed-off-by: Chen, Hu <hu1.chen@intel.com> Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>

chen-hu-97 · 2022-05-25T04:41:27Z

Current master is 8.2.0-dev, so if it exists in both PHP-8.1 and master, then you need to target PHP-8.1 and we will merge it up into master.

If the bug also exists in PHP-8.0, then you should target PHP-8.0 instead.

Hi @ramsey,
I change the target branch to PHP-8.1. But the from branch is master and thus looks like there are some conflicts. Maybe I need create a new PR whose from branch is also PHP-8.1?

I am new on the flow of github. Sorry for the inconvenience.

cmb69 · 2022-05-25T09:03:12Z

I change the target branch to PHP-8.1.

The proper way to do this for php-src is to first rebase the branch onto the new target branch (PHP-8.1), then force-push, and only then change the target branch via UI.

cmb69

Thank you!

matyhtf mentioned this pull request Apr 16, 2022

enables explictly shadow stack and indirect branch tracking on intel swoole/swoole-src#4686

Closed

cmb69 reviewed Apr 26, 2022

View reviewed changes

mvorisek mentioned this pull request May 4, 2022

Enable Intel CET on Windows by default #8491

Closed

chen-hu-97 force-pushed the master branch from 2a022f1 to 28a7302 Compare May 17, 2022 06:48

chen-hu-97 requested review from trowski and cmb69 May 19, 2022 01:16

cmb69 reviewed May 23, 2022

View reviewed changes

Zend/zend_fibers.c Outdated Show resolved Hide resolved

trowski approved these changes May 23, 2022

View reviewed changes

ramsey added Waiting on Author Category: Fibers Category: Engine labels May 23, 2022

chen-hu-97 changed the base branch from master to PHP-8.1 May 25, 2022 04:12

chen-hu-97 force-pushed the master branch from 0eeafa4 to 980b3f5 Compare May 25, 2022 15:02

cmb69 approved these changes May 25, 2022

View reviewed changes

ramsey linked an issue May 25, 2022 that may be closed by this pull request

Intel CET is disabled unintentionally since PHP-8.1.0 #8338

Closed

ramsey removed the Waiting on Author label May 25, 2022

ramsey closed this in 040a37d May 25, 2022

cmb69 mentioned this pull request Nov 4, 2022

Fiber: add shadow stack support #9283

Closed

Fixed bug #8338 (Intel CET is disabled unintentionally since PHP-8.1.0) #8339

Fixed bug #8338 (Intel CET is disabled unintentionally since PHP-8.1.0) #8339

Uh oh!

Conversation

chen-hu-97 commented Apr 11, 2022

Uh oh!

cmb69 commented Apr 11, 2022

Uh oh!

trowski commented Apr 12, 2022

Uh oh!

cmb69 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

chen-hu-97 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

cmb69 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

chen-hu-97 Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmb69 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

trowski Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

chen-hu-97 Apr 29, 2022

Choose a reason for hiding this comment

Uh oh!

chen-hu-97 commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chen-hu-97 commented Apr 29, 2022

Uh oh!

devnexen commented Apr 29, 2022

Uh oh!

mvorisek commented May 4, 2022

Uh oh!

cmb69 commented May 4, 2022

Uh oh!

mvorisek commented May 4, 2022

Uh oh!

cmb69 commented May 4, 2022

Uh oh!

cmb69 commented May 4, 2022

Uh oh!

mvorisek commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chen-hu-97 commented May 5, 2022

Uh oh!

chen-hu-97 commented May 17, 2022

Uh oh!

Uh oh!

trowski left a comment

Choose a reason for hiding this comment

Uh oh!

ramsey commented May 23, 2022

Uh oh!

chen-hu-97 commented May 24, 2022

Uh oh!

ramsey commented May 24, 2022

Uh oh!

cmb69 commented May 24, 2022

Uh oh!

chen-hu-97 commented May 25, 2022

Uh oh!

chen-hu-97 commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented May 25, 2022

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chen-hu-97 Apr 29, 2022 •

edited

Loading

chen-hu-97 commented Apr 26, 2022 •

edited

Loading

mvorisek commented May 4, 2022 •

edited

Loading

chen-hu-97 commented May 25, 2022 •

edited

Loading