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: <a2732938-f3db-a0af-3d68-a18060f66e79@cs.kuleuven.be>
Date:   Mon, 7 Aug 2023 11:41:34 +0200
From:   Jo Van Bulck <jo.vanbulck@...kuleuven.be>
To:     Jarkko Sakkinen <jarkko@...nel.org>, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     dave.hansen@...ux.intel.com
Subject: Re: [PATCH 4/5] selftests/sgx: Ensure expected enclave data buffer
 size and placement.

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.

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(?)