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?
