Re: [RFC PATCH v6 13/25] x86/sgx: Add helpers to expose ECREATE and EINIT to KVM

[Kai Huang <kai.huang@intel.com>]

On Fri, 5 Mar 2021 09:51:56 -0800 Dave Hansen wrote:
> On 2/26/21 4:15 AM, Kai Huang wrote:
> > +int sgx_virt_ecreate(struct sgx_pageinfo *pageinfo, void __user *secs,
> > +		     int *trapnr)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * @secs is userspace address, and it's not guaranteed @secs points at
> > +	 * an actual EPC page. 
> 
> There are four cases that I can think of:
> 1. @secs points to an EPC page.  Good, return 0 and go on with life.
> 2. @secs points to a non-EPC page.  It will fault and permanently error
>    out
> 3. @secs points to a Present=0 PTE.  It will fault, but we need to call
>    the fault handler to get a page in here.
> 4. @secs points to a kernel address
> 
> #1 and #2 are handled and described.
> 
> #4 is probably impossible because the address comes out of some
> gpa_to_hva() KVM code.  But, it still _looks_ wonky here.  I wouldn't
> hate an access_ok() check on it.
> 
> 	/*
> 	 * @secs is an untrusted, userspace-provided address.  It comes
>          * from KVM and is assumed to point somewhere in userspace.
>  	 * This can fault and call SGX or other fault handlers.
> 	 */
> 
> You can also spend a moment to describe the kinds of faults that are
> handled and what is fatal.

Thanks Dave for the comments. I'll refine accordingly.

> 
> 
> > +	 * to physical EPC page by resolving PFN but using __uaccess_xx() is
> > +	 * simpler.
> > +	 */
> 
> I'd leave the justification for the changelog.

Will do.

> 
> > +	__uaccess_begin();
> > +	ret = __ecreate(pageinfo, (void *)secs);
> > +	__uaccess_end();
> > +
> > +	if (encls_faulted(ret)) {
> > +		*trapnr = ENCLS_TRAPNR(ret);
> > +		return -EFAULT;
> > +	}
>