Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in test enclave.

[Jo Van Bulck]

On 03.08.23 05:58, Huang, Kai wrote:
> Putting aside whether we should consider building the selftests using "-Os", it
> would be helpful to explain how can the "-Os" break the existing code, so that
> we can review why this fix is reasonable.  Perhaps it's obvious to others but
> it's not obvious to me what can go wrong here.

I dug deeper into this and the real problem here is that the enclave ELF 
is linked with -static but also needs to be relocatable, which, in my 
understanding, is not what -static is meant for (i.e., the man pages 
says -static overrides -pie). Particularly, with -static I noticed that 
global variables are hard-coded assuming the ELF is loaded at base 
address zero.

When reading more on this, it seems like the proper thing to do for 
static and relocatable binaries is to compile with -static-pie, which is 
added since gcc 8 [1] (and similarly supported by clang).

As a case in point, to hopefully make this clearer, consider the 
following C function:

extern uint8_t __enclave_base;
void *get_base(void) {
	return &__enclave_base;
}

Compiling with -static and -fPIC hard codes the __enclave_base symbol to 
zero (the start of the ELF enclave image):

00000000000023f4 <get_base>:
    23f4:	f3 0f 1e fa          	endbr64
    23f8:	55                   	push   %rbp
    23f9:	48 89 e5             	mov    %rsp,%rbp
    23fc:	48 c7 c0 00 00 00 00 	mov    $0x0,%rax
    2403:	5d                   	pop    %rbp
    2404:	c3                   	ret

Compiling with -static-pie and -fPIE properly emits a RIP-relative address:

00000000000023f4 <get_base>:
    23f4:	f3 0f 1e fa          	endbr64
    23f8:	55                   	push   %rbp
    23f9:	48 89 e5             	mov    %rsp,%rbp
    23fc:	48 8d 05 fd db ff ff 	lea    -0x2403(%rip),%rax        # 0 
<__enclave_base>
    2403:	5d                   	pop    %rbp
    2404:	c3                   	ret

Now, the fact that it currently *happens* to work is a mere coincidence 
of how the local encl_op_array initialization is compiled without 
optimizations with -static -fPIC:

00000000000023f4 <encl_body>:
    /* snipped */
    2408:       48 8d 05 ec fe ff ff    lea    -0x114(%rip),%rax 
# 22fb <do_encl_op_put_to_buf>
    240f:       48 89 45 b0             mov    %rax,-0x50(%rbp)
    2413:       48 8d 05 18 ff ff ff    lea    -0xe8(%rip),%rax 
# 2332 <do_encl_op_get_from_buf>
    241a:       48 89 45 b8             mov    %rax,-0x48(%rbp)
    241e:       48 8d 05 44 ff ff ff    lea    -0xbc(%rip),%rax 
# 2369 <do_encl_op_put_to_addr>
    /* snipped */

When compiling with optimizations with -static -fPIC -Os, encl_op_array 
is instead initialized with a prepared copy from .data:

00000000000021b5 <encl_body>:
    /* snipped */
    21bc:       48 8d 35 3d 2e 00 00    lea    0x2e3d(%rip),%rsi 
# 5000 <encl_buffer+0x2000>
    21c3:       48 8d 7c 24 b8          lea    -0x48(%rsp),%rdi
    21c8:       b9 10 00 00 00          mov    $0x10,%ecx
    21cd:       f3 a5                   rep movsl %ds:(%rsi),%es:(%rdi)
    /* snipped */

Thus, in this optimized code, encl_op_array will have function pointers 
that are *not* relocated. The compilation assumes the -static binary has 
base address zero and is not relocatable:

$ readelf -r test_encl.elf

There are no relocations in this file.

When compiling with -static-pie -PIE -Os, the same code is emitted *but* 
the binary is relocatable:

$ readelf -r test_encl.elf

Relocation section '.rela.dyn' at offset 0x4000 contains 12 entries:
  Offset          Info           Type           Sym. Value    Sym. Name 
+ Addend
# tcs1.ossa
000000000010  000000000008 R_X86_64_RELATIVE                    7000
# tcs1.oentry
000000000020  000000000008 R_X86_64_RELATIVE                    21e3
# tcs2.ossa
000000001010  000000000008 R_X86_64_RELATIVE                    8000
# tcs2.oentry
000000001020  000000000008 R_X86_64_RELATIVE                    21e3
# encl_op_array
000000006000  000000000008 R_X86_64_RELATIVE                    2098
000000006008  000000000008 R_X86_64_RELATIVE                    20ae
000000006010  000000000008 R_X86_64_RELATIVE                    20c4
000000006018  000000000008 R_X86_64_RELATIVE                    20d7
000000006020  000000000008 R_X86_64_RELATIVE                    20ea
000000006028  000000000008 R_X86_64_RELATIVE                    203e
000000006030  000000000008 R_X86_64_RELATIVE                    2000
000000006038  000000000008 R_X86_64_RELATIVE                    20ef

Apparently, for static-pie binaries, glibc includes a 
_dl_relocate_static_pie routine that will perform the relocations as 
part of the startup [2,3]. Since the enclave loading process is 
different and glibc is not included, we cannot rely on these relocations 
to be performed and we need to do them manually. Note: the first 4 
symbols in the relocation table above are from the TCS initialization in 
test_encl_bootstrap.S and should *not* be relocated (as these are 
relative to enclave base as per SGX spec).

Bottom line: the way I see it, the enclave should either ensure no 
relocations are needed, or perform the relocations manually where 
needed, or include a _dl_relocate_static_pie equivalent that parses the 
.rela.dyn ELF section and patches all relocations (minus the TCS 
symbols). Since the latter (most general) approach is likely going to 
make the selftest enclave unnecessarily complex by including ELF parsing 
etc, I propose to simply relocate the function-pointer table manually 
(which is indeed the only place that needs relocations).

I will include code to properly compile the selftest enclave with 
-static-pie as per above and relocate the function-pointer table in the 
next patch revision.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81498
[2] https://stackoverflow.com/a/62587156
[3] 
https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/dl-reloc-static-pie.c;h=382482fa739f10934aeb916650c65a4db231c481;hb=HEAD