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(-)
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
--
2.34.1On 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 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 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 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(?)
© 2016 - 2026 Red Hat, Inc.