Message-ID: <9ecbbf06fdc992720a2da1f14b7863e9493cdcc9.camel@intel.com>
Date:   Mon, 21 Aug 2023 11:04:18 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Van Bulck, Jo" <jo.vanbulck@...kuleuven.be>
CC:     "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 2/5] selftests/sgx: Fix function pointer relocation in
 test enclave.

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".

[...]

Powered by blists - more mailing lists