QEMUv7: PKCS#11 TA crash with Clang 18.1.7

[jforissier]

Platform is QEMUv7. OP-TEE and TAs are built with Clang 18.1.7. The PKCS#11 TA crashes upon first call to __ta_entry() with a stack check error. No issue observed with GCC or Clang 12.0.0 (downloaded with make clang-toolchains), or with QEMUv8.

Details and how to reproduce:

$ mkdir /tmp/optee
$ cd /tmp/optee
$ repo init -u https://github.com/OP-TEE/manifest.git
$ repo sync -j10
$ cd build
$ make -j3 toolchains
$ make clang-toolchains-build   # be patient, takes time!
$ export PATH=$(pwd)/../toolchains/clang-18.1.7/bin:$PATH
$ make -j$(nproc) COMPILER=clang check CHECK_TEST=xtest XTEST_ARGS=pkcs11_1001

The make check command ends with:

[...]
Starting QEMU... done, guest is booted.
Running: xtest pkcs11_1001...
# pkcs11_1001 FAIL
make: *** [Makefile:205: check] Error 1

Crash info from the log file:

$ tail -30 ../out/bin/serial0.log | ../optee_os/scripts/symbolize.py -d ../optee_os/out/arm/ta/pkcs11
D/LD:  ldelf:177 ELF (fd02c9da-306c-48c7-a49c-bbd827ae86ee) at 0x161000
E/TA:  stack smashing detected
E/TC:? 0
E/TC:? 0 TA panicked with code 0xffff300f (TEE_ERROR_OVERFLOW)
E/LD:  Status of TA fd02c9da-306c-48c7-a49c-bbd827ae86ee
E/LD:   arch: arm
E/LD:  region  0: va 0x00103000 pa 0x0e301000 size 0x002000 flags rw-s (ldelf)
E/LD:  region  1: va 0x00105000 pa 0x0e303000 size 0x00f000 flags r-xs (ldelf)
E/LD:  region  2: va 0x00114000 pa 0x0e312000 size 0x001000 flags rw-s (ldelf)
E/LD:  region  3: va 0x00115000 pa 0x0e313000 size 0x004000 flags rw-s (ldelf)
E/LD:  region  4: va 0x00119000 pa 0x0e317000 size 0x001000 flags r--s
E/LD:  region  5: va 0x0011a000 pa 0x0e388000 size 0x002000 flags rw-s (stack)
E/LD:  region  6: va 0x00161000 pa 0x0e318000 size 0x05b000 flags r-xs [0] .text .rodata .ARM.extab .ta_head .ARM.exidx .dynsym .dynstr .hash .gnu.hash
E/LD:  region  7: va 0x001bc000 pa 0x0e373000 size 0x015000 flags rw-s [0] .dynamic .got .rel.dyn .data .bss
E/LD:   [0] fd02c9da-306c-48c7-a49c-bbd827ae86ee @ 0x00161000 (/tmp/optee/optee_os/out/arm/ta/pkcs11/fd02c9da-306c-48c7-a49c-bbd827ae86ee.elf)
E/LD:  Call stack:
E/LD:   0x0018917c _utee_panic at /tmp/optee/optee_os/lib/libutee/arch/arm/utee_syscalls_a32.S:53
E/LD:   0x00176ca9 __stack_chk_fail at /tmp/optee/optee_os/lib/libutils/isoc/stack_check.c:27
E/LD:   0x00176bd3 __ta_entry_c at /tmp/optee/optee_os/out/arm/export-ta_arm32/src/user_ta_header.c:?
E/LD:   0x00176bf4 __ta_entry at /tmp/optee/optee_os/out/arm/export-ta_arm32/src/ta_entry_a32.S:22
D/TC:? 0 user_ta_enter:196 tee_user_ta_enter: TA panicked with code 0xffff300f (TEE_ERROR_OVERFLOW)
D/TC:? 0 release_ta_ctx:670 Releasing panicked TA ctx
D/TC:? 0 tee_ta_close_session:461 csess 0x1653cea0 id 1
D/TC:? 0 tee_ta_close_session:479 Destroy session
D/TC:? 0 destroy_context:318 Destroy TA ctx (0x1653ce60)
E/TC:? 0 tee_ta_open_session:745 Failed for TA fd02c9da-306c-48c7-a49c-bbd827ae86ee. Return error 0xffff3024
ERR [119] LT:ckteec_invoke_init:304: TEEC open session failed ffff3024 from 3

/tmp/optee/out-br/build/optee_test_ext-1.0/host/xtest/pkcs11_1000.c:244: rv has an unexpected value: 0x30 = CKR_DEVICE_ERROR, expected 0x0 = CKR_OK

I believe the code that sets __stack_chck_guard is incorrect. That variable is supposed to be reserved by the compiler implementation and therefore it is undefined behavior to touch it in C code. The following patch fixes the issue but I am not sure it is the way to go (we would still want to initialize the stack check value to some true random sequence I suppose):

diff --git a/ta/user_ta_header.c b/ta/user_ta_header.c
index d602f9226..d1d79ebe6 100644
--- a/ta/user_ta_header.c
+++ b/ta/user_ta_header.c
@@ -44,7 +44,7 @@ void __noreturn _C_FUNCTION(__ta_entry)(unsigned long func,
 					struct utee_params *up,
 					unsigned long cmd_id);
 
-void __noreturn _C_FUNCTION(__ta_entry)(unsigned long func,
+void __noreturn __attribute__((no_stack_protector)) _C_FUNCTION(__ta_entry)(unsigned long func,
 					unsigned long session_id,
 					struct utee_params *up,
 					unsigned long cmd_id)

@maximus64 what do you think?

[maximus64]

@jforissier Interesting that we're encountering this issue when the function is defined with the noreturn attribute. It appears that Clang 18 has decided to also check the stack canary for noreturn functions.

In Linux user space, the dynamic linker (ld.so) typically initializes __stack_chk_guard with a random value during the program's startup phase, before any user code runs. So one approach is to generate a random stack canary value and populate this symbol in OPTEE's ldelf, but this seems complicated.

Your proposed solution looks okay. In fact, in the Linux kernel, they initialize the kernel stack canary in a similar way (see start_kernel() in Linux source code); start_kernel() also sets __no_stack_protector.

Do we support the attribute((constructor)) or static initializers yet? If so, then maybe this would be a good point to set up the __stack_chk_guard value before entering the TA entry point. It would make sense to do this in ldelf.

[jforissier]

@jforissier Interesting that we're encountering this issue when the function is defined with the noreturn attribute. It appears that Clang 18 has decided to also check the stack canary for noreturn functions.

In Linux user space, the dynamic linker (ld.so) typically initializes __stack_chk_guard with a random value during the program's startup phase, before any user code runs. So one approach is to generate a random stack canary value and populate this symbol in OPTEE's ldelf, but this seems complicated.

I agree.

Your proposed solution looks okay. In fact, in the Linux kernel, they initialize the kernel stack canary in a similar way (see start_kernel() in Linux source code); start_kernel() also sets __no_stack_protector.

Sounds good. Thanks for the link.

Do we support the attribute((constructor)) or static initializers yet? If so, then maybe this would be a good point to set up the __stack_chk_guard value before entering the TA entry point. It would make sense to do this in ldelf.

We do support __attribute__((constructor)) (see https://github.com/OP-TEE/optee_os/blob/4.3.0/lib/libutee/user_ta_entry.c#L106-L136) but unfortunately the constructors are called too late, by __utee_entry() in __ta_entry() (or more exactly __ta_entry_c() function in the case of arm32). And since there may be several C constructors per ELF binary, it does not sound like a good option anyway because there would be no way to make sure the stack protector setup is the first C function called. In fact, I'd say that setting the stack protector in C is only possible if the function is __no_stack_protector itself, which brings us back to the above suggestion.

Thanks for your advice. I will create a PR with a proposed fix.

[jforissier]

#7049