diff mbox series

[v22,09/24] x86/sgx: Add functions to allocate and free EPC pages

Message ID 20190903142655.21943-10-jarkko.sakkinen@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Intel SGX foundations | expand

Commit Message

Jarkko Sakkinen Sept. 3, 2019, 2:26 p.m. UTC 
Add functions for grabbing EPC pages into use:

* sgx_alloc_page(): Iterate the EPC sections and return the first free
  page, or ERR_PTR(-ENOMEM) when no free pages are available.
* __sgx_free_page(): Return the page into uninitialized state and move
  it back to the corresponding EPC section structure. Issues WARN()
  when EREMOVE fails.
* sgx_free_page(): Return the page into uninitialized state and move
  it back to the corresponding EPC section structure. Returns
  ENCLS[EREMOVE] error code back to the caller.

[1] Intel SDM: 40.3 INTEL® SGX SYSTEM LEAF FUNCTION REFERENCE

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kernel/cpu/sgx/main.c | 90 ++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/sgx/sgx.h  |  4 ++
 2 files changed, 94 insertions(+)

Comments

Borislav Petkov Oct. 5, 2019, 4:44 p.m. UTC | #1 
On Tue, Sep 03, 2019 at 05:26:40PM +0300, Jarkko Sakkinen wrote:
> Add functions for grabbing EPC pages into use:
> 
> * sgx_alloc_page(): Iterate the EPC sections and return the first free
>   page, or ERR_PTR(-ENOMEM) when no free pages are available.
> * __sgx_free_page(): Return the page into uninitialized state and move
>   it back to the corresponding EPC section structure. Issues WARN()
>   when EREMOVE fails.
> * sgx_free_page(): Return the page into uninitialized state and move
>   it back to the corresponding EPC section structure. Returns
>   ENCLS[EREMOVE] error code back to the caller.
> 
> [1] Intel SDM: 40.3 INTEL® SGX SYSTEM LEAF FUNCTION REFERENCE
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 90 ++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/sgx/sgx.h  |  4 ++
>  2 files changed, 94 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index e2317f6e4374..6b4727df72ca 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -9,6 +9,7 @@
>  #include <linux/sched/signal.h>
>  #include <linux/slab.h>
>  #include "arch.h"
> +#include "encls.h"
>  #include "sgx.h"
>  
>  struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
> @@ -16,6 +17,95 @@ EXPORT_SYMBOL_GPL(sgx_epc_sections);
>  
>  int sgx_nr_epc_sections;
>  
> +static struct sgx_epc_page *sgx_section_get_page(

That fits into 80 cols (oh well, 81) and even if not, a trailing opening
arg brace is ugly.

> +	struct sgx_epc_section *section)
> +{
> +	struct sgx_epc_page *page;
> +
> +	if (!section->free_cnt)
> +		return NULL;
> +
> +	page = list_first_entry(&section->page_list,
> +				struct sgx_epc_page, list);

That fits in 80-cols too. Why break it?

> +	list_del_init(&page->list);
> +	section->free_cnt--;
> +	return page;
> +}
> +
> +/**
> + * sgx_alloc_page - Allocate an EPC page
> + *
> + * Try to grab a page from the free EPC page list.
> + *
> + * Return:
> + *   a pointer to a &struct sgx_epc_page instance,
> + *   -errno on error
> + */
> +struct sgx_epc_page *sgx_alloc_page(void)
> +{
> +	struct sgx_epc_section *section;
> +	struct sgx_epc_page *page;
> +	int i;
> +
> +	for (i = 0; i < sgx_nr_epc_sections; i++) {
> +		section = &sgx_epc_sections[i];
> +		spin_lock(&section->lock);
> +		page = sgx_section_get_page(section);
> +		spin_unlock(&section->lock);
> +
> +		if (page)
> +			return page;
> +	}
> +
> +	return ERR_PTR(-ENOMEM);
> +}
> +EXPORT_SYMBOL_GPL(sgx_alloc_page);

That export gets removed later too. But you know already...

> +
> +/**
> + * __sgx_free_page - Free an EPC page
> + * @page:	pointer a previously allocated EPC page
> + *
> + * EREMOVE an EPC page and insert it back to the list of free pages.
> + *
> + * Return:
> + *   0 on success
> + *   SGX error code if EREMOVE fails
> + */
> +int __sgx_free_page(struct sgx_epc_page *page)
> +{
> +	struct sgx_epc_section *section = sgx_epc_section(page);
> +	int ret;

Shouldn't you be grabbing the lock here already?

Or nothing can happen to that page from another thread after you
ENCLS[EREMOVE] it and before it is added to the ->page_list of the
section?

> +
> +	ret = __eremove(sgx_epc_addr(page));
> +	if (ret)
> +		return ret;
> +
> +	spin_lock(&section->lock);
> +	list_add_tail(&page->list, &section->page_list);
> +	section->free_cnt++;
> +	spin_unlock(&section->lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(__sgx_free_page);
> +
> +/**
> + * sgx_free_page - Free an EPC page and WARN on failure
> + * @page:	pointer to a previously allocated EPC page
> + *
> + * EREMOVE an EPC page and insert it back to the list of free pages, and WARN
> + * if EREMOVE fails.  For use when the call site cannot (or chooses not to)
> + * handle failure, i.e. the page is leaked on failure.
> + */
> +void sgx_free_page(struct sgx_epc_page *page)
> +{
> +	int ret;
> +
> +	ret = __sgx_free_page(page);
> +	WARN(ret > 0, "sgx: EREMOVE returned %d (0x%x)", ret, ret);

That will potentially flood dmesg. Why are we even accommodating such
callers? They either handle the error or they don't get to alloc EPC
pages. There's also __must_check with which you can enforce the error
code checking or we simply don't allow not handling failure. Fullstop.

Thx.
Sean Christopherson Oct. 7, 2019, 2:50 p.m. UTC | #2 
On Sat, Oct 05, 2019 at 06:44:08PM +0200, Borislav Petkov wrote:
> On Tue, Sep 03, 2019 at 05:26:40PM +0300, Jarkko Sakkinen wrote:
> > +/**
> > + * __sgx_free_page - Free an EPC page
> > + * @page:	pointer a previously allocated EPC page
> > + *
> > + * EREMOVE an EPC page and insert it back to the list of free pages.
> > + *
> > + * Return:
> > + *   0 on success
> > + *   SGX error code if EREMOVE fails
> > + */
> > +int __sgx_free_page(struct sgx_epc_page *page)
> > +{
> > +	struct sgx_epc_section *section = sgx_epc_section(page);
> > +	int ret;
> 
> Shouldn't you be grabbing the lock here already?
> 
> Or nothing can happen to that page from another thread after you
> ENCLS[EREMOVE] it and before it is added to the ->page_list of the
> section?

The caller is responsible for ensuring EREMOVE can be safely executed,
e.g. by holding the enclave's lock.

For many ENCLS leafs, EREMOVE included, the CPU requires exclusive access
to the SGX Enclave Control Structures (SECS)[*] and will signal a #GP if
a different logical CPU is already executing an ENCLS leaf that requires
exclusive SECS access.  The SGX subsystem uses a per-enclave mutex to
serialize such ENCLS leafs, among other things.

[*] The SECS is a per-enclave page that resides in the EPC and can only be
    directly accessed by the CPU.  It's used to track metadata about the
    enclave, e.g. number of child pages, base, size, etc...

> > +
> > +	ret = __eremove(sgx_epc_addr(page));
> > +	if (ret)
> > +		return ret;
> > +
> > +	spin_lock(&section->lock);
> > +	list_add_tail(&page->list, &section->page_list);
> > +	section->free_cnt++;
> > +	spin_unlock(&section->lock);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__sgx_free_page);
> > +
> > +/**
> > + * sgx_free_page - Free an EPC page and WARN on failure
> > + * @page:	pointer to a previously allocated EPC page
> > + *
> > + * EREMOVE an EPC page and insert it back to the list of free pages, and WARN
> > + * if EREMOVE fails.  For use when the call site cannot (or chooses not to)
> > + * handle failure, i.e. the page is leaked on failure.
> > + */
> > +void sgx_free_page(struct sgx_epc_page *page)
> > +{
> > +	int ret;
> > +
> > +	ret = __sgx_free_page(page);
> > +	WARN(ret > 0, "sgx: EREMOVE returned %d (0x%x)", ret, ret);
> 
> That will potentially flood dmesg. Why are we even accommodating such
> callers? They either handle the error or they don't get to alloc EPC
> pages. There's also __must_check with which you can enforce the error
> code checking or we simply don't allow not handling failure. Fullstop.

It was deliberately left as a WARN as having multiple stack traces has
been very helpful for triaging and debugging software bugs.  I agree that
it can and should be changed to a WARN_ONCE() for upstream.

As for why it exists at all, the WARN will only fire if there is a kernel
and/or CPU bug.  In the vast majority of cases, EREMOVE is guaranteed to
be successful (assuming no bugs).  In those situations, if EREMOVE fails
there really isn't a sane option other to WARN and leak the page, e.g.
the kernel can't override the CPUs protection to forcefully EREMOVE the
page.

The non-warn variant, __sgx_free_page(), is provided for the (currently)
one case where EREMOVE failure is expected/tolerable (opportunstically
freeing EPC pages when the enclave is killed).
Jarkko Sakkinen Oct. 7, 2019, 5:55 p.m. UTC | #3 
On Sat, Oct 05, 2019 at 06:44:08PM +0200, Borislav Petkov wrote:
> > +static struct sgx_epc_page *sgx_section_get_page(
> 
> That fits into 80 cols (oh well, 81) and even if not, a trailing opening
> arg brace is ugly.

But checkpatch.pl will complain about it...

/Jarkko
Borislav Petkov Oct. 7, 2019, 6:09 p.m. UTC | #4 
On Mon, Oct 07, 2019 at 08:55:55PM +0300, Jarkko Sakkinen wrote:
> But checkpatch.pl will complain about it...

You should not take checkpatch.pl messages to the letter but always
sanity-check them with common sense. In this particular example,
readability is much more important than some tool measuring line length.

Sure, if you can make the line fit into 80 columns, then fine, but
having to chop it into unreadability just to make some tool happy is
certainly not what the goal should be.
Borislav Petkov Oct. 8, 2019, 9:09 a.m. UTC | #5 
On Mon, Oct 07, 2019 at 07:50:11AM -0700, Sean Christopherson wrote:
> The caller is responsible for ensuring EREMOVE can be safely executed,
> e.g. by holding the enclave's lock.

lockdep_assert_held() here maybe?

> For many ENCLS leafs, EREMOVE included, the CPU requires exclusive access
> to the SGX Enclave Control Structures (SECS)[*] and will signal a #GP if
> a different logical CPU is already executing an ENCLS leaf that requires
> exclusive SECS access.  The SGX subsystem uses a per-enclave mutex to
> serialize such ENCLS leafs, among other things.
>
> [*] The SECS is a per-enclave page that resides in the EPC and can only be
>     directly accessed by the CPU.  It's used to track metadata about the
>     enclave, e.g. number of child pages, base, size, etc...

Ok.
Sean Christopherson Oct. 8, 2019, 1:31 p.m. UTC | #6 
On Tue, Oct 08, 2019 at 11:09:31AM +0200, Borislav Petkov wrote:
> On Mon, Oct 07, 2019 at 07:50:11AM -0700, Sean Christopherson wrote:
> > The caller is responsible for ensuring EREMOVE can be safely executed,
> > e.g. by holding the enclave's lock.
> 
> lockdep_assert_held() here maybe?

There are a few flows where a page can be removed without holding a lock,
e.g. when the enclave is being released, and for Version Array (VA) pages,
which are not associated with a single enclave.  We could probably force
a lockdep assert with an extra parameter, but I'm not sure that'd be a net
positive.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index e2317f6e4374..6b4727df72ca 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -9,6 +9,7 @@ 
 #include <linux/sched/signal.h>
 #include <linux/slab.h>
 #include "arch.h"
+#include "encls.h"
 #include "sgx.h"
 
 struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS];
@@ -16,6 +17,95 @@  EXPORT_SYMBOL_GPL(sgx_epc_sections);
 
 int sgx_nr_epc_sections;
 
+static struct sgx_epc_page *sgx_section_get_page(
+	struct sgx_epc_section *section)
+{
+	struct sgx_epc_page *page;
+
+	if (!section->free_cnt)
+		return NULL;
+
+	page = list_first_entry(&section->page_list,
+				struct sgx_epc_page, list);
+	list_del_init(&page->list);
+	section->free_cnt--;
+	return page;
+}
+
+/**
+ * sgx_alloc_page - Allocate an EPC page
+ *
+ * Try to grab a page from the free EPC page list.
+ *
+ * Return:
+ *   a pointer to a &struct sgx_epc_page instance,
+ *   -errno on error
+ */
+struct sgx_epc_page *sgx_alloc_page(void)
+{
+	struct sgx_epc_section *section;
+	struct sgx_epc_page *page;
+	int i;
+
+	for (i = 0; i < sgx_nr_epc_sections; i++) {
+		section = &sgx_epc_sections[i];
+		spin_lock(&section->lock);
+		page = sgx_section_get_page(section);
+		spin_unlock(&section->lock);
+
+		if (page)
+			return page;
+	}
+
+	return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(sgx_alloc_page);
+
+/**
+ * __sgx_free_page - Free an EPC page
+ * @page:	pointer a previously allocated EPC page
+ *
+ * EREMOVE an EPC page and insert it back to the list of free pages.
+ *
+ * Return:
+ *   0 on success
+ *   SGX error code if EREMOVE fails
+ */
+int __sgx_free_page(struct sgx_epc_page *page)
+{
+	struct sgx_epc_section *section = sgx_epc_section(page);
+	int ret;
+
+	ret = __eremove(sgx_epc_addr(page));
+	if (ret)
+		return ret;
+
+	spin_lock(&section->lock);
+	list_add_tail(&page->list, &section->page_list);
+	section->free_cnt++;
+	spin_unlock(&section->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__sgx_free_page);
+
+/**
+ * sgx_free_page - Free an EPC page and WARN on failure
+ * @page:	pointer to a previously allocated EPC page
+ *
+ * EREMOVE an EPC page and insert it back to the list of free pages, and WARN
+ * if EREMOVE fails.  For use when the call site cannot (or chooses not to)
+ * handle failure, i.e. the page is leaked on failure.
+ */
+void sgx_free_page(struct sgx_epc_page *page)
+{
+	int ret;
+
+	ret = __sgx_free_page(page);
+	WARN(ret > 0, "sgx: EREMOVE returned %d (0x%x)", ret, ret);
+}
+EXPORT_SYMBOL_GPL(sgx_free_page);
+
 static __init void sgx_free_epc_section(struct sgx_epc_section *section)
 {
 	struct sgx_epc_page *page;
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 3009ec816339..210510a28ce0 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -64,4 +64,8 @@  extern int sgx_nr_epc_sections;
 
 int sgx_page_reclaimer_init(void);
 
+struct sgx_epc_page *sgx_alloc_page(void);
+int __sgx_free_page(struct sgx_epc_page *page);
+void sgx_free_page(struct sgx_epc_page *page);
+
 #endif /* _X86_SGX_H */