linux-kernel - Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer size and placement.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a2dfe906-0794-09bd-793a-ef663b6f5cbb@cs.kuleuven.be>
Date:   Mon, 7 Aug 2023 11:50:21 +0200
From:   Jo Van Bulck <jo.vanbulck@...kuleuven.be>
To:     "Huang, Kai" <kai.huang@...el.com>,
        "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
        "jarkko@...nel.org" <jarkko@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer
 size and placement.

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