linux-kernel - Re: [PATCH 3/5] selftests/sgx: Ensure correct secinfo struct alignment in test enclave.

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: <f0e29d32-286d-8973-dc34-93f1d8005787@cs.kuleuven.be>
Date:   Mon, 7 Aug 2023 11:21:08 +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 3/5] selftests/sgx: Ensure correct secinfo struct
 alignment in test enclave.

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