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: <4022cb20af2759d0e71f72a1b4161b3e43181bca.camel@intel.com>
Date:   Fri, 18 Aug 2023 13:07:14 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "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>,
        "Van Bulck, Jo" <jo.vanbulck@...kuleuven.be>
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 Mon, 2023-08-07 at 11:41 +0200, Jo Van Bulck wrote:
> 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.

Could we use "used" attribute?

https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

used 

	This attribute, attached to a variable with static storage, means that 
	the variable must be emitted even if it appears that the variable is 
	not referenced.

	When applied to a static data member of a C++ class template, the 
	attribute also means that the member is instantiated if the class 
	itself is instantiated.
> 
> 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(?)

This looks sane.