| Message ID | 20230724165832.15797-5-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: > Do not declare the enclave data buffer static to ensure it is not optimized ~~~~~~ as static /enclave data buffer/encl_buffer/ > away by the compiler, even when not used entirely by the test enclave code. "Declare encl_buffer as global, in order to ensure that it is not optimized away by the compiler." So, when exactly is it optimized away by the compiler? This is missing. BR, Jarkko
On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote: > Do not declare the enclave data buffer static to ensure it is not optimized > away by the compiler, even when not used entirely by the test enclave code. The "encl_buffer" array is initialized to 1 explicitly, which means it should be in .data section. As Jarkko commented, can you add more information about in what cases it can be optimized away? > Use -fPIE to make the compiler access the non-static buffer with > RIP-relative addressing. > I am not quite following how does "RIP-relative addressing" fix any problem? Is there any hard relationship between "RIP-relative addressing" and the problem that you are having? > Place the enclave data buffer in a separate > section that is explicitly placed at the start of the .data segment in the > linker script, as expected by the external tests manipulating page > permissions. So this change is not to fix the problem that "the compiler may optimize away the encl_buffer array", correct? > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/Makefile | 2 +- > tools/testing/selftests/sgx/test_encl.c | 5 +++-- > tools/testing/selftests/sgx/test_encl.lds | 1 + > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile > index 50aab6b57da3..c5483445ba28 100644 > --- a/tools/testing/selftests/sgx/Makefile > +++ b/tools/testing/selftests/sgx/Makefile > @@ -13,7 +13,7 @@ endif > > INCLUDES := -I$(top_srcdir)/tools/include > HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack > -ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \ > +ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIE \ > -fno-stack-protector -mrdrnd $(INCLUDES) > > TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index aba301abefb8..5c274e517d13 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -7,9 +7,10 @@ > /* > * Data buffer spanning two pages that will be placed first in .data > * segment. Even if not used internally the second page is needed by > - * external test manipulating page permissions. > + * external test manipulating page permissions. Do not declare this > + * buffer as static, so the compiler cannot optimize it out. > */ > -static uint8_t encl_buffer[8192] = { 1 }; > +uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192]; > > enum sgx_enclu_function { > EACCEPT = 0x5, > diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds > index ca659db2a534..79b1e41d8d24 100644 > --- a/tools/testing/selftests/sgx/test_encl.lds > +++ b/tools/testing/selftests/sgx/test_encl.lds > @@ -24,6 +24,7 @@ SECTIONS > } : text > > .data : { > + *(.data.encl_buffer) > *(.data*) > } : data >
On 28.07.23 21:19, Jarkko Sakkinen wrote:
> So, when exactly is it optimized away by the compiler? This is missing.
The problem is that declaring encl_buf as static, implies that it will
only be used in this file and the compiler is allowed to optimize away
any entries that are never used within this compilation unit (e.g., when
optimizing out the memcpy calls).
In reality, the tests outside test_encl.elf rely on both the size and
exact placement of encl_buf at the start of .data.
For example, clang -Os generates the following (legal) code when
encl_bug is declared as static:
0000000000001020 <do_encl_op_put_to_buf>:
mov 0x8(%rdi),%al
mov %al,0x1fd7(%rip) # 3000 <encl_buffer.0>
mov 0x9(%rdi),%al
mov %al,0x8fce(%rip) # a000 <encl_buffer.1.0>
mov 0xa(%rdi),%al
mov %al,0x8fd5(%rip) # a010 <encl_buffer.1.1>
mov 0xb(%rdi),%al
mov %al,0x8fce(%rip) # a012 <encl_buffer.1.2>
mov 0xc(%rdi),%al
mov %al,0x8fd3(%rip) # a020 <encl_buffer.1.3>
mov 0xd(%rdi),%al
mov %al,0x8fce(%rip) # a024 <encl_buffer.1.4>
mov 0xe(%rdi),%al
mov %al,0x8fd1(%rip) # a030 <encl_buffer.1.5>
mov 0xf(%rdi),%al
mov %al,0x8fca(%rip) # a032 <encl_buffer.1.6>
ret
Disassembly of section .data:
0000000000003000 <encl_buffer.0>:
3000: 01 00
...
0000000000004000 <encl_ssa_tcs1>:
Thus, this proposed patch fixes both the size and location:
1. removing the static keyword from the encl_bug declaration ensures
that the _entire_ buffer is preserved with expected size, as the
compiler cannot anymore assume encl_buf is only used in this file.
2. adding _attribute__((section(".data.encl_buffer"))) ensures that we
can control the expected location at the start of the .data section. I
think this is optional, as encl_buf always seems to be placed at the
start of .data in all my tests. But afaik this is not guaranteed as per
the C standard and such constraints on exact placement should better be
explicitly controlled in the linker script(?)
On 03.08.23 06:22, Huang, Kai wrote: > The "encl_buffer" array is initialized to 1 explicitly, which means it should be > in .data section. As Jarkko commented, can you add more information about in > what cases it can be optimized away? Yes it will indeed be in .data, but it may be reduced in size. See my reply to Jarkko's question for more information and a concrete example. > I am not quite following how does "RIP-relative addressing" fix any problem? Is > there any hard relationship between "RIP-relative addressing" and the problem > that you are having? Good point. I think this is now cleared up with the -static-pie discussion in reply to you other point on patch 2/5. Concretely, the reason -fPIE is needed is that otherwise global variables (i.e., not declared as static) would not be addressed RIP-relative, but as hard-coded addresses assuming the -static binary is loaded at a fixed address. > So this change is not to fix the problem that "the compiler may optimize away > the encl_buffer array", correct? Correct, this fix ensures the expected location. As per my reply to Jarkko's question: > 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we can control the expected location at the start of the .data section. I think this is optional, as encl_buf always seems to be placed at the start of .data in all my tests. But afaik this is not guaranteed as per the C standard and such constraints on exact placement should better be explicitly controlled in the linker script(?) Perhaps I better split this into 2 separate patches? The location-control may also not be needed in practice, but afaik that would be undefined C behavior (cf above) and better be avoided? Best, Jo > >> >> Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> >> --- >> tools/testing/selftests/sgx/Makefile | 2 +- >> tools/testing/selftests/sgx/test_encl.c | 5 +++-- >> tools/testing/selftests/sgx/test_encl.lds | 1 + >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile >> index 50aab6b57da3..c5483445ba28 100644 >> --- a/tools/testing/selftests/sgx/Makefile >> +++ b/tools/testing/selftests/sgx/Makefile >> @@ -13,7 +13,7 @@ endif >> >> INCLUDES := -I$(top_srcdir)/tools/include >> HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack >> -ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \ >> +ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIE \ >> -fno-stack-protector -mrdrnd $(INCLUDES) >> >> TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx >> diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c >> index aba301abefb8..5c274e517d13 100644 >> --- a/tools/testing/selftests/sgx/test_encl.c >> +++ b/tools/testing/selftests/sgx/test_encl.c >> @@ -7,9 +7,10 @@ >> /* >> * Data buffer spanning two pages that will be placed first in .data >> * segment. Even if not used internally the second page is needed by >> - * external test manipulating page permissions. >> + * external test manipulating page permissions. Do not declare this >> + * buffer as static, so the compiler cannot optimize it out. >> */ >> -static uint8_t encl_buffer[8192] = { 1 }; >> +uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192]; >> >> enum sgx_enclu_function { >> EACCEPT = 0x5, >> diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds >> index ca659db2a534..79b1e41d8d24 100644 >> --- a/tools/testing/selftests/sgx/test_encl.lds >> +++ b/tools/testing/selftests/sgx/test_encl.lds >> @@ -24,6 +24,7 @@ SECTIONS >> } : text >> >> .data : { >> + *(.data.encl_buffer) >> *(.data*) >> } : data >> >
On Mon, 2023-08-07 at 11:41 +0200, Jo Van Bulck wrote: > On 28.07.23 21:19, Jarkko Sakkinen wrote: > > So, when exactly is it optimized away by the compiler? This is missing. > > The problem is that declaring encl_buf as static, implies that it will > only be used in this file and the compiler is allowed to optimize away > any entries that are never used within this compilation unit (e.g., when > optimizing out the memcpy calls). > > In reality, the tests outside test_encl.elf rely on both the size and > exact placement of encl_buf at the start of .data. > > For example, clang -Os generates the following (legal) code when > encl_bug is declared as static: > > 0000000000001020 <do_encl_op_put_to_buf>: > mov 0x8(%rdi),%al > mov %al,0x1fd7(%rip) # 3000 <encl_buffer.0> > mov 0x9(%rdi),%al > mov %al,0x8fce(%rip) # a000 <encl_buffer.1.0> > mov 0xa(%rdi),%al > mov %al,0x8fd5(%rip) # a010 <encl_buffer.1.1> > mov 0xb(%rdi),%al > mov %al,0x8fce(%rip) # a012 <encl_buffer.1.2> > mov 0xc(%rdi),%al > mov %al,0x8fd3(%rip) # a020 <encl_buffer.1.3> > mov 0xd(%rdi),%al > mov %al,0x8fce(%rip) # a024 <encl_buffer.1.4> > mov 0xe(%rdi),%al > mov %al,0x8fd1(%rip) # a030 <encl_buffer.1.5> > mov 0xf(%rdi),%al > mov %al,0x8fca(%rip) # a032 <encl_buffer.1.6> > ret > > Disassembly of section .data: > > 0000000000003000 <encl_buffer.0>: > 3000: 01 00 > ... > 0000000000004000 <encl_ssa_tcs1>: > > Thus, this proposed patch fixes both the size and location: > > 1. removing the static keyword from the encl_bug declaration ensures > that the _entire_ buffer is preserved with expected size, as the > compiler cannot anymore assume encl_buf is only used in this file. Could we use "used" attribute? https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html used This attribute, attached to a variable with static storage, means that the variable must be emitted even if it appears that the variable is not referenced. When applied to a static data member of a C++ class template, the attribute also means that the member is instantiated if the class itself is instantiated. > > 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we > can control the expected location at the start of the .data section. I > think this is optional, as encl_buf always seems to be placed at the > start of .data in all my tests. But afaik this is not guaranteed as per > the C standard and such constraints on exact placement should better be > explicitly controlled in the linker script(?) This looks sane.
On 18.08.23 06:07, Huang, Kai wrote: > Could we use "used" attribute? > > https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html > > used > > This attribute, attached to a variable with static storage, means that > the variable must be emitted even if it appears that the variable is > not referenced. > > When applied to a static data member of a C++ class template, the > attribute also means that the member is instantiated if the class > itself is instantiated. Thank you for pointing this out! I was not aware of this attribute, but it is indeed exactly what we need in this case and works as expected. >> >> 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we >> can control the expected location at the start of the .data section. I >> think this is optional, as encl_buf always seems to be placed at the >> start of .data in all my tests. But afaik this is not guaranteed as per >> the C standard and such constraints on exact placement should better be >> explicitly controlled in the linker script(?) > > This looks sane. Thanks, applying this in a separate commit as discussed. Best, Jo
diff --git a/tools/testing/selftests/sgx/Makefile b/tools/testing/selftests/sgx/Makefile index 50aab6b57da3..c5483445ba28 100644 --- a/tools/testing/selftests/sgx/Makefile +++ b/tools/testing/selftests/sgx/Makefile @@ -13,7 +13,7 @@ endif INCLUDES := -I$(top_srcdir)/tools/include HOST_CFLAGS := -Wall -Werror -g $(INCLUDES) -fPIC -z noexecstack -ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIC \ +ENCL_CFLAGS := -Wall -Werror -static -nostdlib -nostartfiles -fPIE \ -fno-stack-protector -mrdrnd $(INCLUDES) TEST_CUSTOM_PROGS := $(OUTPUT)/test_sgx diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index aba301abefb8..5c274e517d13 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -7,9 +7,10 @@ /* * Data buffer spanning two pages that will be placed first in .data * segment. Even if not used internally the second page is needed by - * external test manipulating page permissions. + * external test manipulating page permissions. Do not declare this + * buffer as static, so the compiler cannot optimize it out. */ -static uint8_t encl_buffer[8192] = { 1 }; +uint8_t __attribute__((section(".data.encl_buffer"))) encl_buffer[8192]; enum sgx_enclu_function { EACCEPT = 0x5, diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds index ca659db2a534..79b1e41d8d24 100644 --- a/tools/testing/selftests/sgx/test_encl.lds +++ b/tools/testing/selftests/sgx/test_encl.lds @@ -24,6 +24,7 @@ SECTIONS } : text .data : { + *(.data.encl_buffer) *(.data*) } : data
Do not declare the enclave data buffer static to ensure it is not optimized away by the compiler, even when not used entirely by the test enclave code. Use -fPIE to make the compiler access the non-static buffer with RIP-relative addressing. Place the enclave data buffer in a separate section that is explicitly placed at the start of the .data segment in the linker script, as expected by the external tests manipulating page permissions. Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> --- tools/testing/selftests/sgx/Makefile | 2 +- tools/testing/selftests/sgx/test_encl.c | 5 +++-- tools/testing/selftests/sgx/test_encl.lds | 1 + 3 files changed, 5 insertions(+), 3 deletions(-)