| Message ID | 20230724165832.15797-3-jo.vanbulck@cs.kuleuven.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/sgx: Fix compilation errors. | expand |
On Mon Jul 24, 2023 at 4:58 PM UTC, Jo Van Bulck wrote: > Relocate encl_op_array entries at runtime relative to the enclave base to > ensure correct function pointer when compiling the test enclave with -Os. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/test_encl.c | 6 ++++-- > tools/testing/selftests/sgx/test_encl.lds | 1 + > tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d6397295e3..4e31a6c3d673 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op) > > } > > +uint64_t get_enclave_base(void); > + > void encl_body(void *rdi, void *rsi) > { > - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { > + static void (*encl_op_array[ENCL_OP_MAX])(void *) = { > do_encl_op_put_to_buf, > do_encl_op_get_from_buf, > do_encl_op_put_to_addr, > @@ -135,5 +137,5 @@ void encl_body(void *rdi, void *rsi) > struct encl_op_header *op = (struct encl_op_header *)rdi; > > if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + (*(get_enclave_base() + encl_op_array[op->type]))(op); > } > diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds > index a1ec64f7d91f..ca659db2a534 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -10,6 +10,7 @@ PHDRS > SECTIONS > { > . = 0; > + __enclave_base = .; > .tcs : { > *(.tcs*) > } : tcs > diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S > index 03ae0f57e29d..6126dbd7ad1c 100644 > --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S > +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S > @@ -86,6 +86,11 @@ encl_entry_core: > mov $4, %rax > enclu > > + .global get_enclave_base > +get_enclave_base: > + lea __enclave_base(%rip), %rax > + ret > + > .section ".data", "aw" > > encl_ssa_tcs1: > -- > 2.34.1 Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote: > Relocate encl_op_array entries at runtime relative to the enclave base to > ensure correct function pointer when compiling the test enclave with -Os. 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. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/test_encl.c | 6 ++++-- > tools/testing/selftests/sgx/test_encl.lds | 1 + > tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index c0d6397295e3..4e31a6c3d673 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op) > > } > > +uint64_t get_enclave_base(void); > + > void encl_body(void *rdi, void *rsi) > { > - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { > + static void (*encl_op_array[ENCL_OP_MAX])(void *) = { > do_encl_op_put_to_buf, > do_encl_op_get_from_buf, > do_encl_op_put_to_addr, > @@ -135,5 +137,5 @@ void encl_body(void *rdi, void *rsi) > struct encl_op_header *op = (struct encl_op_header *)rdi; > > if (op->type < ENCL_OP_MAX) > - (*encl_op_array[op->type])(op); > + (*(get_enclave_base() + encl_op_array[op->type]))(op); > } > diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds > index a1ec64f7d91f..ca659db2a534 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -10,6 +10,7 @@ PHDRS > SECTIONS > { > . = 0; > + __enclave_base = .; > .tcs : { > *(.tcs*) > } : tcs > diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S > index 03ae0f57e29d..6126dbd7ad1c 100644 > --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S > +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S > @@ -86,6 +86,11 @@ encl_entry_core: > mov $4, %rax > enclu > > + .global get_enclave_base > +get_enclave_base: > + lea __enclave_base(%rip), %rax > + ret > + > .section ".data", "aw" > > encl_ssa_tcs1:
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
On Mon, 2023-08-07 at 09:13 +0200, Jo Van Bulck wrote: > 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). Thanks for analysing! > > 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 */ How is the "prepared copy" prepared, exactly? Could you paste the relevant code here too? IMHO w/o it it's hard to tell whether the code could be wrong or not after relocating. > > 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]. > I am not sure whether all those 'rela.dyn' matters due to the reason you mentioned below ... > 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. > ... here. Those relocation table are not used by enclave builder anyway. Only ".tsc" ".text" and ".data" + some heap are built as enclave. > 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). I don't quite follow here. Per my understanding TCS pages can be any page within the enclave. I don't quite understand what does "relocated" mean here. > > 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 think we are kinda mixing two things together: 1) the "relocation" supported by the "non-enclave" normal case, where the compiler/linker generates the PIC code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2) the "relocation" for the enclave case, where the compiler/linker still generates the PIC code, but the "enclave loader" loads the enclave into a random virtual address of the process. Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as powerful as the "real loader" in the normal case. In fact, reading the code, IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and plus some heap) and load them into the enclave. Now the important thing is: those sections are "contiguous" in the enclave. That means the kernel needs to build the enclave ELF file with those sections "contiguously" in the same order too as a single piece, and this single piece can work at any random address that the "enclave loader" loads the enclave. Any address fixing up due to different location of ".data"/".tsc" section at loading time cannot be generated. This should be the thing that we need to focus on. That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the enclave ELF file. > > 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. I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??). However I am yet not convinced the "relocate function-pointer" thing. If you can paste the relevant code it would be helpful. Or am I missing something big?
On 18.08.23 05:54, Huang, Kai wrote: > How is the "prepared copy" prepared, exactly? Could you paste the relevant code > here too? IMHO w/o it it's hard to tell whether the code could be wrong or not > after relocating. The "prepared copy" resides in .data and simply contains the absolute addresses of the functions (i.e., as they appear in objdump with .text starting at 0x2000). Compiled with gcc -static -fPIC -Os, this looks as follows (the "prepared copy" starts at 0x5000, immediately following encl_buffer at the start of .data): readelf -x .data test_encl.elf 0x00005000 64200000 00000000 7a200000 00000000 d ......z ...... 0x00005010 90200000 00000000 a3200000 00000000 . ....... ...... 0x00005020 b6200000 00000000 24200000 00000000 . ......$ ...... 0x00005030 00200000 00000000 bb200000 00000000 . ....... ...... objdump -D test_encl.elf | grep do_encl_ 0000000000002000 <do_encl_emodpe>: 0000000000002024 <do_encl_eaccept>: 0000000000002064 <do_encl_op_put_to_buf>: 000000000000207a <do_encl_op_get_from_buf>: 0000000000002090 <do_encl_op_put_to_addr>: 00000000000020a3 <do_encl_op_get_from_addr>: 00000000000020b6 <do_encl_op_nop>: 00000000000020bb <do_encl_init_tcs_page>: For reference, the full encl_body code: 0000000000002175 <encl_body>: 2175: f3 0f 1e fa endbr64 2179: 49 89 f8 mov %rdi,%r8 217c: 48 8d 35 7d 2e 00 00 lea 0x2e7d(%rip),%rsi # 5000 <encl_buffer+0x2000> 2183: 48 8d 7c 24 b8 lea -0x48(%rsp),%rdi 2188: b9 10 00 00 00 mov $0x10,%ecx 218d: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) 218f: 49 8b 00 mov (%r8),%rax 2192: 48 83 f8 07 cmp $0x7,%rax 2196: 77 0a ja 21a2 <encl_body+0x2d> 2198: 48 8b 44 c4 b8 mov -0x48(%rsp,%rax,8),%rax 219d: 4c 89 c7 mov %r8,%rdi 21a0: ff e0 jmp *%rax 21a2: c3 ret Thus, the "prepared copy" with _absolute_ function pointers is loaded from .data and copied onto the stack. The code then jumps to the _absolute_ function address loaded from the local copy, i.e., _without_ first properly relocating the loaded address. > I am not sure whether all those 'rela.dyn' matters due to the reason you > mentioned below ... > >> 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. >> > > ... here. > > Those relocation table are not used by enclave builder anyway. Only ".tsc" > ".text" and ".data" + some heap are built as enclave. Yes, the relocation table is not used by the (untrusted) enclave builder, neither by a (trusted) initialization stub inside the enclave. Hence, this commit tries to address this by _manually_ relocating the (only) needed relocations. >> 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). > > I don't quite follow here. Per my understanding TCS pages can be any page > within the enclave. I don't quite understand what does "relocated" mean here. Yes, the TCS can be any page in the enclave, as the architectural definitions are explicitly position-independent: OSSA and OENTRY are specified as a relative _offset_ to the enclave base runtime load address. The thing is, these fields are initialized in test_encl_bootstrap.S as symbols to filled in by the linker: .section ".tcs", "aw" .balign 4096 .fill 1, 8, 0 # STATE (set by CPU) .fill 1, 8, 0 # FLAGS .quad encl_ssa_tcs1 # OSSA .fill 1, 4, 0 # CSSA (set by CPU) .fill 1, 4, 1 # NSSA .quad encl_entry # OENTRY /* snipped */ Thus, when compiling/linking with "-static-pie", the linker (obviously not aware of SGX TCS semantics) will treat these symbols as "normal" and recognize that they need to be relocated as they are absolute (non-RIP relative) references and places them in ".rela.dyn": 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 Thus, my earlier comment says that we can safely ignore these apparent/false TCS "relocations". > I think we are kinda mixing two things together: 1) the "relocation" supported > by the "non-enclave" normal case, where the compiler/linker generates the PIC > code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2) > the "relocation" for the enclave case, where the compiler/linker still generates > the PIC code, but the "enclave loader" loads the enclave into a random virtual > address of the process. > > Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as > powerful as the "real loader" in the normal case. In fact, reading the code, > IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and > plus some heap) and load them into the enclave. Agreed, the "enclave loader" should simply copy the sections into EPC memory without being a "real loader". Particularly, it should *not* do any relocations as that would change the code and, hence, the MRENCLAVE signature. > > Now the important thing is: those sections are "contiguous" in the enclave. > That means the kernel needs to build the enclave ELF file with those sections > "contiguously" in the same order too as a single piece, and this single piece > can work at any random address that the "enclave loader" loads the enclave. Any > address fixing up due to different location of ".data"/".tsc" section at loading > time cannot be generated. > > This should be the thing that we need to focus on. > Agreed. That's why any _necessary_ relocations need to happen *inside* the enclave, by the application or a small initialization stub, such that the enclave MRENCLAVE identity is independent of its load address. > That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the > enclave ELF file. Agreed, this would be "ideal". However, I don't see a way to generate the function-pointer table without needing a runtime relocation. For all other code, we don't have to care about this and we can simply rely on -static-pie and -fPIE to emit RIP-relative code without needing relocations. Afais, when storing an address in a variable, a relocation is needed. I agree though that we do *not* need a full .rela.dyn parser here and can simply manually relocate the only relevant case here, ie encl_op_array. > I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??). Not sure on the exact difference between -fPIE and -fPIC. I changed to -fPIE because gcc mentions in the documentation for "-static-pie" that: For predictable results, you must also specify the same set of options used for compilation (-fpie, -fPIE, or model suboptions) when you specify this linker option. > However I am yet not convinced the "relocate function-pointer" thing. If you > can paste the relevant code it would be helpful. See above. Let me know if you'd like more context or the full binary. > Or am I missing something big? Nothing big, but, in my understanding, even PIE code may still need (limited) relocations, eg when storing (function-pointer) addresses in a C variable. This is normally taken care of by glibc on startup, but we need to do this manually here as implemented in this commit. Best, Jo
On Fri, 2023-08-18 at 19:30 -0700, Jo Van Bulck wrote: > On 18.08.23 05:54, Huang, Kai wrote: > > How is the "prepared copy" prepared, exactly? Could you paste the relevant code > > here too? IMHO w/o it it's hard to tell whether the code could be wrong or not > > after relocating. > > The "prepared copy" resides in .data and simply contains the absolute > addresses of the functions (i.e., as they appear in objdump with .text > starting at 0x2000). > > Compiled with gcc -static -fPIC -Os, this looks as follows (the > "prepared copy" starts at 0x5000, immediately following encl_buffer at > the start of .data): > > readelf -x .data test_encl.elf > 0x00005000 64200000 00000000 7a200000 00000000 d ......z ...... > 0x00005010 90200000 00000000 a3200000 00000000 . ....... ...... > 0x00005020 b6200000 00000000 24200000 00000000 . ......$ ...... > 0x00005030 00200000 00000000 bb200000 00000000 . ....... ...... > > objdump -D test_encl.elf | grep do_encl_ > 0000000000002000 <do_encl_emodpe>: > 0000000000002024 <do_encl_eaccept>: > 0000000000002064 <do_encl_op_put_to_buf>: > 000000000000207a <do_encl_op_get_from_buf>: > 0000000000002090 <do_encl_op_put_to_addr>: > 00000000000020a3 <do_encl_op_get_from_addr>: > 00000000000020b6 <do_encl_op_nop>: > 00000000000020bb <do_encl_init_tcs_page>: > > For reference, the full encl_body code: > > 0000000000002175 <encl_body>: > 2175: f3 0f 1e fa endbr64 > 2179: 49 89 f8 mov %rdi,%r8 > 217c: 48 8d 35 7d 2e 00 00 lea 0x2e7d(%rip),%rsi > # 5000 <encl_buffer+0x2000> > 2183: 48 8d 7c 24 b8 lea -0x48(%rsp),%rdi > 2188: b9 10 00 00 00 mov $0x10,%ecx > 218d: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) > 218f: 49 8b 00 mov (%r8),%rax > 2192: 48 83 f8 07 cmp $0x7,%rax > 2196: 77 0a ja 21a2 <encl_body+0x2d> > 2198: 48 8b 44 c4 b8 mov -0x48(%rsp,%rax,8),%rax > 219d: 4c 89 c7 mov %r8,%rdi > 21a0: ff e0 jmp *%rax > 21a2: c3 ret > > Thus, the "prepared copy" with _absolute_ function pointers is loaded > from .data and copied onto the stack. The code then jumps to the > _absolute_ function address loaded from the local copy, i.e., _without_ > first properly relocating the loaded address. Thanks. You mentioned this was generated by "-static -fPIC -Os" but I think using "-static-pie -fPIE -Os" would probably generate the same. > > > I am not sure whether all those 'rela.dyn' matters due to the reason you > > mentioned below ... > > > > > 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. > > > > > > > ... here. > > > > Those relocation table are not used by enclave builder anyway. Only ".tsc" > > ".text" and ".data" + some heap are built as enclave. > > Yes, the relocation table is not used by the (untrusted) enclave > builder, neither by a (trusted) initialization stub inside the enclave. > Hence, this commit tries to address this by _manually_ relocating the > (only) needed relocations. > > > > 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). > > > > I don't quite follow here. Per my understanding TCS pages can be any page > > within the enclave. I don't quite understand what does "relocated" mean here. > > Yes, the TCS can be any page in the enclave, as the architectural > definitions are explicitly position-independent: OSSA and OENTRY are > specified as a relative _offset_ to the enclave base runtime load address. > > The thing is, these fields are initialized in test_encl_bootstrap.S as > symbols to filled in by the linker: > > .section ".tcs", "aw" > .balign 4096 > > .fill 1, 8, 0 # STATE (set by CPU) > .fill 1, 8, 0 # FLAGS > .quad encl_ssa_tcs1 # OSSA > .fill 1, 4, 0 # CSSA (set by CPU) > .fill 1, 4, 1 # NSSA > .quad encl_entry # OENTRY > /* snipped */ > > Thus, when compiling/linking with "-static-pie", the linker (obviously > not aware of SGX TCS semantics) will treat these symbols as "normal" and > recognize that they need to be relocated as they are absolute (non-RIP > relative) references and places them in ".rela.dyn": Right. Even for "-static-pie" it's still possible to generate "rela.dyn" for those have to use absolute addressing. > > 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 > > Thus, my earlier comment says that we can safely ignore these > apparent/false TCS "relocations". Yeah. I guess that's why the test_encl.elf must be built starting from address 0. > > > I think we are kinda mixing two things together: 1) the "relocation" supported > > by the "non-enclave" normal case, where the compiler/linker generates the PIC > > code, and the loader does the "runtime" fixup for those in the "rela.dyn"; 2) > > the "relocation" for the enclave case, where the compiler/linker still generates > > the PIC code, but the "enclave loader" loads the enclave into a random virtual > > address of the process. > > > > Obviously the "enclave loader" (implemented in this selftests/sgx/...) isn't as > > powerful as the "real loader" in the normal case. In fact, reading the code, > > IIUC, it simply gathers ".tsc"/".text"/".data" sections from the ELF file (and > > plus some heap) and load them into the enclave. > > Agreed, the "enclave loader" should simply copy the sections into EPC > memory without being a "real loader". Particularly, it should *not* do > any relocations as that would change the code and, hence, the MRENCLAVE > signature. Yeah. > > > > > Now the important thing is: those sections are "contiguous" in the enclave. > > That means the kernel needs to build the enclave ELF file with those sections > > "contiguously" in the same order too as a single piece, and this single piece > > can work at any random address that the "enclave loader" loads the enclave. Any > > address fixing up due to different location of ".data"/".tsc" section at loading > > time cannot be generated. > > > > This should be the thing that we need to focus on. > > > > Agreed. That's why any _necessary_ relocations need to happen *inside* > the enclave, by the application or a small initialization stub, such > that the enclave MRENCLAVE identity is independent of its load address. > > > That being said, I think ideally there shouldn't be _ANY_ "rela.dyn" in the > > enclave ELF file. > > Agreed, this would be "ideal". However, I don't see a way to generate > the function-pointer table without needing a runtime relocation. For all > other code, we don't have to care about this and we can simply rely on > -static-pie and -fPIE to emit RIP-relative code without needing > relocations. Afais, when storing an address in a variable, a relocation > is needed. Yeah. Thanks. > > I agree though that we do *not* need a full .rela.dyn parser here and > can simply manually relocate the only relevant case here, ie encl_op_array. > > > I agree we should use "-static-pie" + "-fPIE" (or "-fPIC" is also OK??). > > Not sure on the exact difference between -fPIE and -fPIC. I changed to > -fPIE because gcc mentions in the documentation for "-static-pie" that: > > For predictable results, you must also specify the same set of options > used for compilation (-fpie, -fPIE, or model suboptions) when you > specify this linker option. Yeah I guess we should just use "-fPIE". [...]
On 21.08.23 13:04, Huang, Kai wrote: > Thanks. You mentioned this was generated by "-static -fPIC -Os" but I think > using "-static-pie -fPIE -Os" would probably generate the same. Thanks, yes, I can confirm that the same code is generated with -static-pie -FPIE. > Right. Even for "-static-pie" it's still possible to generate "rela.dyn" for > those have to use absolute addressing. Indeed. >> Thus, my earlier comment says that we can safely ignore these >> apparent/false TCS "relocations". > > Yeah. I guess that's why the test_encl.elf must be built starting from address > 0. True (for the ELF in the linker script, but we still need the runtime relocations for the function-pointer table when loading the enclave at a non-zero base address). > Yeah I guess we should just use "-fPIE". Agreed. Best, Jo
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index c0d6397295e3..4e31a6c3d673 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -119,9 +119,11 @@ static void do_encl_op_nop(void *_op) } +uint64_t get_enclave_base(void); + void encl_body(void *rdi, void *rsi) { - const void (*encl_op_array[ENCL_OP_MAX])(void *) = { + static void (*encl_op_array[ENCL_OP_MAX])(void *) = { do_encl_op_put_to_buf, do_encl_op_get_from_buf, do_encl_op_put_to_addr, @@ -135,5 +137,5 @@ void encl_body(void *rdi, void *rsi) struct encl_op_header *op = (struct encl_op_header *)rdi; if (op->type < ENCL_OP_MAX) - (*encl_op_array[op->type])(op); + (*(get_enclave_base() + encl_op_array[op->type]))(op); } diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds index a1ec64f7d91f..ca659db2a534 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -10,6 +10,7 @@ PHDRS SECTIONS { . = 0; + __enclave_base = .; .tcs : { *(.tcs*) } : tcs diff --git a/tools/testing/selftests/sgx/test_encl_bootstrap.S b/tools/testing/selftests/sgx/test_encl_bootstrap.S index 03ae0f57e29d..6126dbd7ad1c 100644 --- a/tools/testing/selftests/sgx/test_encl_bootstrap.S +++ b/tools/testing/selftests/sgx/test_encl_bootstrap.S @@ -86,6 +86,11 @@ encl_entry_core: mov $4, %rax enclu + .global get_enclave_base +get_enclave_base: + lea __enclave_base(%rip), %rax + ret + .section ".data", "aw" encl_ssa_tcs1:
Relocate encl_op_array entries at runtime relative to the enclave base to ensure correct function pointer when compiling the test enclave with -Os. Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> --- tools/testing/selftests/sgx/test_encl.c | 6 ++++-- tools/testing/selftests/sgx/test_encl.lds | 1 + tools/testing/selftests/sgx/test_encl_bootstrap.S | 5 +++++ 3 files changed, 10 insertions(+), 2 deletions(-)