linux-kernel - Re: [PATCH v3 13/25] x86/sgx: Add helpers to expose ECREATE and EINIT to KVM

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: <20210406094421.4fdfbb6c4c11e7ee64c3b0a3@intel.com>
Date:   Tue, 6 Apr 2021 09:44:21 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     kvm@...r.kernel.org, linux-sgx@...r.kernel.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, seanjc@...gle.com, jarkko@...nel.org,
        luto@...nel.org, dave.hansen@...el.com, rick.p.edgecombe@...el.com,
        haitao.huang@...el.com, pbonzini@...hat.com, tglx@...utronix.de,
        mingo@...hat.com, hpa@...or.com
Subject: Re: [PATCH v3 13/25] x86/sgx: Add helpers to expose ECREATE and
 EINIT to KVM

On Mon, 5 Apr 2021 11:07:59 +0200 Borislav Petkov wrote:
> On Fri, Mar 19, 2021 at 08:23:08PM +1300, Kai Huang wrote:
> > +	/*
> > +	 * @secs is an untrusted, userspace-provided address.  It comes from
> > +	 * KVM and is assumed to be a valid pointer which points somewhere in
> > +	 * userspace.  This can fault and call SGX or other fault handlers when
> > +	 * userspace mapping @secs doesn't exist.
> > +	 *
> > +	 * Add a WARN() to make sure @secs is already valid userspace pointer
> > +	 * from caller (KVM), who should already have handled invalid pointer
> > +	 * case (for instance, made by malicious guest).  All other checks,
> > +	 * such as alignment of @secs, are deferred to ENCLS itself.
> > +	 */
> > +	WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE));
> 
> So why do we continue here then? IOW:

The intention was to catch KVM bug, since KVM is the only caller, and in current
implementation KVM won't call this function if @secs is not a valid userspace
pointer. But yes we can also return here, but in this case an exception number
must also be specified to *trapnr so that KVM can inject to guest. It's not that
straightforward to decide which exception should we inject, but I think #GP
should be OK. Please see below.

> 
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index fdfc21263a95..497b06fc6f7f 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -270,7 +270,7 @@ int __init sgx_vepc_init(void)
>   *
>   * Return:
>   * - 0:		ECREATE was successful.
> - * - -EFAULT:	ECREATE returned error.
> + * - <0:	ECREATE returned error.
>   */
>  int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
>  		     int *trapnr)
> @@ -288,7 +288,9 @@ int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
>  	 * case (for instance, made by malicious guest).  All other checks,
>  	 * such as alignment of @secs, are deferred to ENCLS itself.
>  	 */
> -	WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE));
> +	if (WARN_ON_ONCE(!access_ok(secs, PAGE_SIZE)))
> +		return -EINVAL;
> +

*trapnr should also be set to an exception before return -EINVAL. It's not
possible to get it from ENCLS_TRAPNR(ret) like below, since ENCLS hasn't been
run yet. I think it makes sense to just set it to #GP(X86_TRAP_GP), since
above error basically means SECS is not pointing to an valid EPC address, and
in such case #GP should happen based on SDM (SDM 40.3 ECREATE).

>  	__uaccess_begin();
>  	ret = __ecreate(pageinfo, (void *)secs);
>  	__uaccess_end();
> 
> > +	__uaccess_begin();
> > +	ret = __ecreate(pageinfo, (void *)secs);
> > +	__uaccess_end();
> > +
> > +	if (encls_faulted(ret)) {
> > +		*trapnr = ENCLS_TRAPNR(ret);
> > +		return -EFAULT;
> > +	}
> > +
> > +	/* ECREATE doesn't return an error code, it faults or succeeds. */
> > +	WARN_ON_ONCE(ret);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(sgx_virt_ecreate);
> > +
> > +static int __sgx_virt_einit(void __user *sigstruct, void __user *token,
> > +			    void __user *secs)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * Make sure all userspace pointers from caller (KVM) are valid.
> > +	 * All other checks deferred to ENCLS itself.  Also see comment
> > +	 * for @secs in sgx_virt_ecreate().
> > +	 */
> > +#define SGX_EINITTOKEN_SIZE	304
> > +	WARN_ON_ONCE(!access_ok(sigstruct, sizeof(struct sgx_sigstruct)) ||
> > +		     !access_ok(token, SGX_EINITTOKEN_SIZE) ||
> > +		     !access_ok(secs, PAGE_SIZE));
> 
> Ditto.

The same as above, *trapnr should be set to X86_TRAP_GP before return
-EINVAL, although for sigstruct, token, they are just normal memory, but not
EPC.

Sean, do you have comments?