Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver
On Tue, Jul 07, 2020 at 05:39:04AM +0100, Matthew Wilcox wrote:
> although I think you have a simpler task.
>
> XA_STATE(xas, ..., start_index);
>
> for (;;) {
> struct page *page = xas_next();
>
> if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> return -EACCES;
> }
>
> return 0;
>
> should do the trick, I think.
Ah, neato. Thanks!
Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver
On Mon, Jul 06, 2020 at 09:29:04PM -0700, Sean Christopherson wrote:
> > > > + idx_start = PFN_DOWN(start);
> > > > + idx_end = PFN_DOWN(end - 1);
> > > > +
> > > > + for (idx = idx_start; idx <= idx_end; ++idx) {
> > > > + mutex_lock(>lock);
> > > > + page = radix_tree_lookup(>page_tree, idx);
> > > > + mutex_unlock(>lock);
> > > > +
> > > > + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > > > + return -EACCES;
> > >
> > > You should really use an iterator here instead of repeated lookups.
> > > xas_for_each() will probably be what you want.
> >
> > Thank you for your remarks. I'll look into using xarray for this.
>
> Question for Matthew:
>
> To enforce the "page must be populated" rule, is there a clean way to retrieve
> the index of the current entry? Our entries/pages don't have information
> about their index. Or should we just count the number of entries and check
> 'em at the end? E.g.
>
> xas_for_each(...) {
> if (~page->vm_max_prot_bits & vm_prot_bits)
> return -EACCES;
> nr_entries++;
> }
>
> if (nr_entries != (end_index - start_index))
> return -EACCES;
Probably best just to steal the implementation from here:
pgoff_t page_cache_next_miss(struct address_space *mapping,
pgoff_t index, unsigned long max_scan)
{
XA_STATE(xas, >i_pages, index);
while (max_scan--) {
void *entry = xas_next();
if (!entry || xa_is_value(entry))
break;
if (xas.xa_index == 0)
break;
}
return xas.xa_index;
}
although I think you have a simpler task.
XA_STATE(xas, ..., start_index);
for (;;) {
struct page *page = xas_next();
if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
return -EACCES;
}
return 0;
should do the trick, I think.
Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver
Man, I really need to type faster.
On Tue, Jul 07, 2020 at 07:11:51AM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 07, 2020 at 04:36:17AM +0100, Matthew Wilcox wrote:
> > What's a leaf function? Is it like a CPU instruction?
>
> Yeah, the opcode is ENCLS for ring-0 (enclave management and
> construction) and ENCLU for ring-3 (entrance to the enclave etc).
> The leaf function number goes to EAX.
To add to Jarkko's comments, for all intents and purposes they are individual
instructions, e.g. all of their own entries in the SDM, but are buried behind
a single opcode that switches on EAX, e.g. ECREATE is EAX=0, EADD is EAX=1,
EINIT is EAX=2. It's purely a way to save opcode space when the extra
overhead is a non-issue, e.g. SMX/TXT's GETSEC does the same shenanigans.
> > > + atomic_set(>flags, 0);
> > > + kref_init(>refcount);
> > > + INIT_RADIX_TREE(>page_tree, GFP_KERNEL);
> >
> > Why are you using a radix tree instead of an xarray?
>
> Because xarray did not exist in 2017 and nobody has pointed out to use
> it. Now I know it exists (yet do not know what it is).
I've followed xarrays a little, but obviously not closely enough to
understand their advantages over radix trees. At a glance, range-based
iteration alone is probably justification enough to switch.
> > > +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> > > + unsigned long end, unsigned long vm_prot_bits)
> > > +{
> > > + unsigned long idx, idx_start, idx_end;
> > > + struct sgx_encl_page *page;
> > > +
> > > + /*
> > > + * Disallow RIE tasks as their VMA permissions might conflict with the
> > > + * enclave page permissions.
> > > + */
> > > + if (!!(current->personality & READ_IMPLIES_EXEC))
> > > + return -EACCES;
> > > +
> > > + idx_start = PFN_DOWN(start);
> > > + idx_end = PFN_DOWN(end - 1);
> > > +
> > > + for (idx = idx_start; idx <= idx_end; ++idx) {
> > > + mutex_lock(>lock);
> > > + page = radix_tree_lookup(>page_tree, idx);
> > > + mutex_unlock(>lock);
> > > +
> > > + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > > + return -EACCES;
> >
> > You should really use an iterator here instead of repeated lookups.
> > xas_for_each() will probably be what you want.
>
> Thank you for your remarks. I'll look into using xarray for this.
Question for Matthew:
To enforce the "page must be populated" rule, is there a clean way to retrieve
the index of the current entry? Our entries/pages don't have information
about their index. Or should we just count the number of entries and check
'em at the end? E.g.
xas_for_each(...) {
if (~page->vm_max_prot_bits & vm_prot_bits)
return -EACCES;
nr_entries++;
}
if (nr_entries != (end_index - start_index))
return -EACCES;
Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver
On Tue, Jul 07, 2020 at 04:36:17AM +0100, Matthew Wilcox wrote:
> On Tue, Jul 07, 2020 at 06:01:51AM +0300, Jarkko Sakkinen wrote:
> > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > can be used by applications to set aside private regions of code and
> > data. The code outside the SGX hosted software entity is disallowed to
>
> s/disallowed to/prevented from/
>
> > access the memory inside the enclave enforced by the CPU. We call these
>
> s/enforced//
>
> > entities enclaves.
> >
> > Add a driver that provides an ioctl API to construct and run enclaves.
> > Enclaves are constructed from pages residing in reserved physical memory
> > areas. The contents of these pages can only be accessed when they are
> > mapped as part of an enclave, by a hardware thread running inside the
> > enclave.
> >
> > The starting state of an enclave consists of a fixed measured set of
> > pages that are copied to the EPC during the construction process by
> > using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> > that defines the enclave properties.
> >
> > Enclaves are constructed by using ENCLS leaf functions ECREATE, EADD and
> > EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> > the EPC and EINIT checks a given signed measurement and moves the enclave
> > into a state ready for execution.
>
> What's a leaf function? Is it like a CPU instruction?
Yeah, the opcode is ENCLS for ring-0 (enclave management and
construction) and ENCLU for ring-3 (entrance to the enclave etc).
The leaf function number goes to EAX.
>
> > The mmap() permissions are capped by the contained enclave page
> > permissions. The mapped areas must also be opaque, i.e. each page address
> > must contain a page. This logic is implemented in sgx_encl_may_map().
>
> do you mean "populated" instead of "opaque"?
Yes, that would be a better word to use. I'll change this.
>
> > + atomic_set(>flags, 0);
> > + kref_init(>refcount);
> > + INIT_RADIX_TREE(>page_tree, GFP_KERNEL);
>
> Why are you using a radix tree instead of an xarray?
Because xarray did not exist in 2017 and nobody has pointed out to use
it. Now I know it exists (yet do not know what it is).
>
> > +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> > +unsigned long end, unsigned long vm_prot_bits)
> > +{
> > + unsigned long idx, idx_start, idx_end;
> > + struct sgx_encl_page *page;
> > +
> > + /*
> > +* Disallow RIE tasks as their VMA permissions might conflict with the
> > +* enclave page permissions.
> > +*/
> > + if (!!(current->personality & READ_IMPLIES_EXEC))
> > + return -EACCES;
> > +
> > + idx_start = PFN_DOWN(start);
> > + idx_end = PFN_DOWN(end - 1);
> > +
> > + for (idx = idx_start; idx <= idx_end; ++idx) {
> > + mutex_lock(>lock);
> > + page = radix_tree_lookup(>page_tree, idx);
> > + mutex_unlock(>lock);
> > +
> > + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> > + return -EACCES;
>
> You should really use an iterator here instead of repeated lookups.
> xas_for_each() will probably be what you want.
Thank you for your remarks. I'll look into using xarray for this.
/Jarkko
Re: [PATCH v34 11/24] x86/sgx: Add SGX enclave driver
On Tue, Jul 07, 2020 at 06:01:51AM +0300, Jarkko Sakkinen wrote:
> Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> can be used by applications to set aside private regions of code and
> data. The code outside the SGX hosted software entity is disallowed to
s/disallowed to/prevented from/
> access the memory inside the enclave enforced by the CPU. We call these
s/enforced//
> entities enclaves.
>
> Add a driver that provides an ioctl API to construct and run enclaves.
> Enclaves are constructed from pages residing in reserved physical memory
> areas. The contents of these pages can only be accessed when they are
> mapped as part of an enclave, by a hardware thread running inside the
> enclave.
>
> The starting state of an enclave consists of a fixed measured set of
> pages that are copied to the EPC during the construction process by
> using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> that defines the enclave properties.
>
> Enclaves are constructed by using ENCLS leaf functions ECREATE, EADD and
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT checks a given signed measurement and moves the enclave
> into a state ready for execution.
What's a leaf function? Is it like a CPU instruction?
> The mmap() permissions are capped by the contained enclave page
> permissions. The mapped areas must also be opaque, i.e. each page address
> must contain a page. This logic is implemented in sgx_encl_may_map().
do you mean "populated" instead of "opaque"?
> + atomic_set(>flags, 0);
> + kref_init(>refcount);
> + INIT_RADIX_TREE(>page_tree, GFP_KERNEL);
Why are you using a radix tree instead of an xarray?
> +int sgx_encl_may_map(struct sgx_encl *encl, unsigned long start,
> + unsigned long end, unsigned long vm_prot_bits)
> +{
> + unsigned long idx, idx_start, idx_end;
> + struct sgx_encl_page *page;
> +
> + /*
> + * Disallow RIE tasks as their VMA permissions might conflict with the
> + * enclave page permissions.
> + */
> + if (!!(current->personality & READ_IMPLIES_EXEC))
> + return -EACCES;
> +
> + idx_start = PFN_DOWN(start);
> + idx_end = PFN_DOWN(end - 1);
> +
> + for (idx = idx_start; idx <= idx_end; ++idx) {
> + mutex_lock(>lock);
> + page = radix_tree_lookup(>page_tree, idx);
> + mutex_unlock(>lock);
> +
> + if (!page || (~page->vm_max_prot_bits & vm_prot_bits))
> + return -EACCES;
You should really use an iterator here instead of repeated lookups.
xas_for_each() will probably be what you want.
