linux-kernel - Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX enclave call

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: <20200313164622.GC5181@linux.intel.com>
Date:   Fri, 13 Mar 2020 09:46:22 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Nathaniel McCallum <npmccallum@...hat.com>
Cc:     Jethro Beekman <jethro@...tanix.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, Neil Horman <nhorman@...hat.com>,
        "Huang, Haitao" <haitao.huang@...el.com>,
        andriy.shevchenko@...ux.intel.com, tglx@...utronix.de,
        "Svahn, Kai" <kai.svahn@...el.com>, bp@...en8.de,
        Josh Triplett <josh@...htriplett.org>, luto@...nel.org,
        kai.huang@...el.com, David Rientjes <rientjes@...gle.com>,
        cedric.xing@...el.com, Patrick Uiterwijk <puiterwijk@...hat.com>,
        Andy Lutomirski <luto@...capital.net>,
        Connor Kuehl <ckuehl@...hat.com>,
        Harald Hoyer <harald@...hat.com>,
        Lily Sturmann <lsturman@...hat.com>
Subject: Re: [PATCH v28 21/22] x86/vdso: Implement a vDSO for Intel SGX
 enclave call

On Fri, Mar 13, 2020 at 11:48:54AM -0400, Nathaniel McCallum wrote:
> Thinking about this more carefully, I still think that at least part
> of my critique still stands.
> 
> __vdso_sgx_enter_enclave() doesn't use the x86-64 ABI. This means that
> there will always be an assembly wrapper for
> __vdso_sgx_enter_enclave(). But because __vdso_sgx_enter_enclave()
> doesn't save %rbx, the wrapper is forced to in order to be called from
> C.
> 
> A common pattern for the wrapper will be to do something like this:
> 
> # void enter_enclave(rdi, rsi, rdx, unused, r8, r9, @tcs, @e,
> @handler, @leaf, @vdso)
> enter_enclave:
>     push %rbx
>     push $0 /* align */
>     push 0x48(%rsp)
>     push 0x48(%rsp)
>     push 0x48(%rsp)
> 
>     mov 0x70(%rsp), %eax
>     call *0x68(%rsp)
> 
>     add $0x20, %rsp
>     pop %rbx
>     ret
> 
> Because __vdso_sgx_enter_enclave() doesn't preserve %rbx, the wrapper
> is forced to reposition stack parameters in a performance-critical
> path. On the other hand, if __vdso_sgx_enter_enclave() preserved %rbx,
> you could implement the above as:
> 
> # void enter_enclave(rdi, rsi, rdx, unused, r8, r9, @tcs, @e,
> @handler, @leaf, @vdso)
> enter_enclave:
>     mov 0x20(%rsp), %eax
>     jmp *0x28(%rsp)
> 
> This also implies that if __vdso_sgx_enter_enclave() took @leaf as a
> stack parameter and preserved %rbx, it would be x86-64 ABI compliant
> enough to call from C if the enclave preserves all callee-saved
> registers besides %rbx (Enarx does).
> 
> What are the downsides of this approach? It also doesn't harm the more
> extended case when you need to use an assembly wrapper to setup
> additional registers. This can still be done. It does imply an extra
> push and mov instruction. But because there are currently an odd
> number of stack function parameters, the push also removes an
> alignment instruction where the stack is aligned before the call to
> __vdso_sgx_enter_enclave() (likely). Further, the push and mov are
> going to be performed by *someone* in order to call
> __vdso_sgx_enter_enclave() from C.
> 
> Therefore, I'd like to propose that __vdso_sgx_enter_enclave():
>   * Preserve %rbx.

At first glance, that looks sane.  Being able to call __vdso... from C
would certainly be nice.

>   * Take the leaf as an additional stack parameter instead of passing
> it in %rax.

Does the leaf even need to be a stack param?  Wouldn't it be possible to
use %rcx as @leaf instead of @unusued?  E.g.

int __vdso_sgx_enter_enclave(unsigned long rdi, unsigned long rsi,
			     unsigned long rdx, unsigned int leaf,
			     unsigned long r8,  unsigned long r9,
			     void *tcs, struct sgx_enclave_exception *e,
			     sgx_enclave_exit_handler_t handler)
{
	push	%rbp
	mov	%rsp, %rbp
	push	%rbx

	mov	%ecx, %eax
.Lenter_enclave
	cmp	$0x2, %eax
	jb	.Linvalid_leaf
	cmp	$0x3, %eax
	ja	.Linvalid_leaf

	mov	0x0x10(%rbp), %rbx
	lea	.Lasync_exit_pointer(%rip), %rcx

.Lasync_exit_pointer:
.Lenclu_eenter_eresume:
	enclu

	xor	%eax, %eax

.Lhandle_exit:
	cmp	$0, 0x20(%rbp)
	jne	.Linvoke_userspace_handler

.Lout
	pop	%rbx
	leave
	ret
}
		

> Then C can call it without additional overhead. And people who need to
> "extend" the C ABI can still do so.
>