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@xxxxxxxxxxxxxx>
---
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
