On 22.08.23 03:11, Huang, Kai wrote:>> +/*
+ * Symbol placed at the start of the enclave image by the linker script.
+ * Declare this extern symbol with visibility "hidden" to ensure the
+ * compiler does not access it through the GOT.
+ */
+extern const uint8_t __attribute__((visibility("hidden"))) __encl_base;
+static const uint64_t encl_base = (uint64_t)&__encl_base;
I had hard time to understand this. The __encl_base is a symbol which is a
fixed value set by the compiler/linker. encl_base has the real storage in the
.data section, but the value is also build-time fixed. IIUC we need some code
to explicitly override it, but I don't see where it's done. Perhaps I missed
something?
Thank you for catching this. Such initialization would indeed have to be
explicitly overridden at runtime and I somehow overlooked this (it seems
I left the line to actually run the tests commented out after
compilation in my test script for all versions; this is now fixed).
Apologies for the confusion, my bad! I've reverted this back to an
explicit (uit64_t)&__encl_base cast in the next patch iteration to avoid
such confusion.
+
+typedef void (*encl_op_t)(void *);
+const encl_op_t encl_op_array[ENCL_OP_MAX] = {
+ do_encl_op_put_to_buf,
+ do_encl_op_get_from_buf,
+ do_encl_op_put_to_addr,
+ do_encl_op_get_from_addr,
+ do_encl_op_nop,
+ do_encl_eaccept,
+ do_encl_emodpe,
+ do_encl_init_tcs_page,
+};
Any reason it cannot be 'static'?
I tested static indeed also works and will include this in the next
iteration.
diff --git a/tools/testing/selftests/sgx/test_encl.lds b/tools/testing/selftests/sgx/test_encl.lds
index 62d37160f..b86c86060 100644
--- a/tools/testing/selftests/sgx/test_encl.lds
+++ b/tools/testing/selftests/sgx/test_encl.lds
@@ -32,6 +32,8 @@ SECTIONS
*(.note*)
*(.debug*)
*(.eh_frame*)
+ *(.dyn*)
+ *(.gnu.hash)
This looks can be in a separate patch, because it's not directly related to what
you are trying to fix.
But I don't want to make things unnecessarily complicated for selftests, so fine
to me if you still want to keep it. But if you do, perhaps you can add some
justification to the changelog saying something like: opportunistically discard
".dyn*" and ".gnu.hash" which the enclave loader cannot handle. Anyway, still
better to make a separate patch for such purpose IMHO.
Thanks, splitting this off in a separate commit for the next iteration.
Best,
Jo
