| Message ID | 20230724165832.15797-4-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: > Declare the secinfo struct as volatile to prevent compiler optimizations > from passing an unaligned pointer to ENCLU. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/test_encl.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index 4e31a6c3d673..aba301abefb8 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -18,7 +18,8 @@ enum sgx_enclu_function { > > static void do_encl_emodpe(void *_op) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + /* declare secinfo volatile to preserve alignment */ > + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > struct encl_op_emodpe *op = _op; > > secinfo.flags = op->flags; > @@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op) > > static void do_encl_eaccept(void *_op) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + /* declare secinfo volatile to preserve alignment */ > + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > struct encl_op_eaccept *op = _op; > int rax; > > -- > 2.34.1 Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
On Mon, 2023-07-24 at 18:58 +0200, Jo Van Bulck wrote: > Declare the secinfo struct as volatile to prevent compiler optimizations > from passing an unaligned pointer to ENCLU. We already have __aligned. Can you provide more information in what circumstances that __aligned isn't enough to guarantee the alignment? We have a lot of kernel code which has __aligned but doesn't have volatile. > > Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> > --- > tools/testing/selftests/sgx/test_encl.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c > index 4e31a6c3d673..aba301abefb8 100644 > --- a/tools/testing/selftests/sgx/test_encl.c > +++ b/tools/testing/selftests/sgx/test_encl.c > @@ -18,7 +18,8 @@ enum sgx_enclu_function { > > static void do_encl_emodpe(void *_op) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + /* declare secinfo volatile to preserve alignment */ > + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > struct encl_op_emodpe *op = _op; > > secinfo.flags = op->flags; > @@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op) > > static void do_encl_eaccept(void *_op) > { > - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > + /* declare secinfo volatile to preserve alignment */ > + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; > struct encl_op_eaccept *op = _op; > int rax; >
On 03.08.23 06:00, Huang, Kai wrote:> We already have __aligned. Can you provide more information in what > circumstances that __aligned isn't enough to guarantee the alignment? > > We have a lot of kernel code which has __aligned but doesn't have volatile. Thank you. I also dug deeper into this and the proper fix is indeed not to make the variable volatile. The real problem is that the inline assembly does not have the "memory" clobber to tell the compiler that the "assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters)" [1]. I checked that, depending on the optimizations and compiler (gcc vs clang), the compiler may indeed reorder the write to secinfo.flags to _after_ the inline asm block. Declaring secinfo as volatile fixed that, but the proper fix is of course to properly include a "memory" clobber for the asm block. I'll include a fix to include a "memory" clobber for the inline asm block in the next patch revision. [1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers-1
diff --git a/tools/testing/selftests/sgx/test_encl.c b/tools/testing/selftests/sgx/test_encl.c index 4e31a6c3d673..aba301abefb8 100644 --- a/tools/testing/selftests/sgx/test_encl.c +++ b/tools/testing/selftests/sgx/test_encl.c @@ -18,7 +18,8 @@ enum sgx_enclu_function { static void do_encl_emodpe(void *_op) { - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; + /* declare secinfo volatile to preserve alignment */ + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; struct encl_op_emodpe *op = _op; secinfo.flags = op->flags; @@ -32,7 +33,8 @@ static void do_encl_emodpe(void *_op) static void do_encl_eaccept(void *_op) { - struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; + /* declare secinfo volatile to preserve alignment */ + volatile struct sgx_secinfo secinfo __aligned(sizeof(struct sgx_secinfo)) = {0}; struct encl_op_eaccept *op = _op; int rax;
Declare the secinfo struct as volatile to prevent compiler optimizations from passing an unaligned pointer to ENCLU. Signed-off-by: Jo Van Bulck <jo.vanbulck@cs.kuleuven.be> --- tools/testing/selftests/sgx/test_encl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)