From: Jarkko Sakkinen <jarkko@kernel.org>
To: Kai Huang <kai.huang@intel.com>
Cc: kvm@vger.kernel.org, x86@kernel.org, linux-sgx@vger.kernel.org,
linux-kernel@vger.kernel.org, seanjc@google.com, luto@kernel.org,
dave.hansen@intel.com, rick.p.edgecombe@intel.com,
haitao.huang@intel.com, pbonzini@redhat.com, bp@alien8.de,
tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com
Subject: Re: [PATCH v4 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()
Date: Fri, 26 Mar 2021 21:48:45 +0200 [thread overview]
Message-ID: <YF46ndD3rdotgOpl@kernel.org> (raw)
In-Reply-To: <20210325093057.122834-1-kai.huang@intel.com>
On Thu, Mar 25, 2021 at 10:30:57PM +1300, Kai Huang wrote:
> EREMOVE takes a page and removes any association between that page and
> an enclave. It must be run on a page before it can be added into
> another enclave. Currently, EREMOVE is run as part of pages being freed
> into the SGX page allocator. It is not expected to fail, as it would
> indicate a use-after-free of EPC. Rather than add the page back to the
> pool of available EPC, the kernel intentionally leaks the page to avoid
> additional errors in the future.
>
> However, KVM does not track how guest pages are used, which means that
> SGX virtualization use of EREMOVE might fail. Specifically, it is
> legitimate that EREMOVE returns SGX_CHILD_PRESENT for EPC assigned to
> KVM guest, because KVM/kernel doesn't track SECS pages.
>
> To allow SGX/KVM to introduce a more permissive EREMOVE helper and to
> let the SGX virtualization code use the allocator directly, break out
> the EREMOVE call from the SGX page allocator. Rename the original
> sgx_free_epc_page() to sgx_encl_free_epc_page(), indicating that it is
> used to free EPC page assigned host enclave. Replace sgx_free_epc_page()
> with sgx_encl_free_epc_page() in all call sites so there's no functional
> change.
>
> At the same time improve error message when EREMOVE fails, and add
> documentation to explain to user what is the bug and suggest user what
> to do when this bug happens, although extremely unlikely.
>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> Documentation/x86/sgx.rst | 27 +++++++++++++++++++++++++++
> arch/x86/kernel/cpu/sgx/encl.c | 32 +++++++++++++++++++++++++++-----
> arch/x86/kernel/cpu/sgx/encl.h | 1 +
> arch/x86/kernel/cpu/sgx/ioctl.c | 6 +++---
> arch/x86/kernel/cpu/sgx/main.c | 14 +++++---------
> arch/x86/kernel/cpu/sgx/sgx.h | 5 +++++
> 6 files changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/x86/sgx.rst b/Documentation/x86/sgx.rst
> index eaee1368b4fd..5ec7d17e65e0 100644
> --- a/Documentation/x86/sgx.rst
> +++ b/Documentation/x86/sgx.rst
> @@ -209,3 +209,30 @@ An application may be loaded into a container enclave which is specially
> configured with a library OS and run-time which permits the application to run.
> The enclave run-time and library OS work together to execute the application
> when a thread enters the enclave.
> +
> +Impact of Potential Kernel SGX Bugs
> +===================================
> +
> +EPC leaks
> +---------
> +
> +EPC leaks can happen if kernel SGX bug happens, when a WARNING with below
> +message is shown in dmesg:
> +
> +"...EREMOVE returned ... and an EPC page was leaked. SGX may become unusuable.
> +This is likely a kernel bug. Refer to Documentation/x86/sgx.rst for more
> +information."
> +
> +This is effectively a kernel use-after-free of EPC, and due to the way SGX
> +works, the bug is detected at freeing. Rather than add the page back to the pool
> +of available EPC, the kernel intentionally leaks the page to avoid additional
> +errors in the future.
> +
> +When this happens, kernel will likely soon leak majority of EPC pages, and SGX
> +will likely become unusable. However while this may be fatal to SGX, other
> +kernel functionalities are unlikely to be impacted, and should continue to work.
> +
> +As a result, when this happpens, user should stop running any new SGX workloads,
> +(or just any new workloads), and migrate all valuable workloads. Although a
> +machine reboot can recover all EPC, the bug should be reported to Linux
> +developers.
> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> index 7449ef33f081..26c0987153de 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.c
> +++ b/arch/x86/kernel/cpu/sgx/encl.c
> @@ -78,7 +78,7 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page,
>
> ret = __sgx_encl_eldu(encl_page, epc_page, secs_page);
> if (ret) {
> - sgx_free_epc_page(epc_page);
> + sgx_encl_free_epc_page(epc_page);
> return ERR_PTR(ret);
> }
>
> @@ -404,7 +404,7 @@ void sgx_encl_release(struct kref *ref)
> if (sgx_unmark_page_reclaimable(entry->epc_page))
> continue;
>
> - sgx_free_epc_page(entry->epc_page);
> + sgx_encl_free_epc_page(entry->epc_page);
> encl->secs_child_cnt--;
> entry->epc_page = NULL;
> }
> @@ -415,7 +415,7 @@ void sgx_encl_release(struct kref *ref)
> xa_destroy(&encl->page_array);
>
> if (!encl->secs_child_cnt && encl->secs.epc_page) {
> - sgx_free_epc_page(encl->secs.epc_page);
> + sgx_encl_free_epc_page(encl->secs.epc_page);
> encl->secs.epc_page = NULL;
> }
>
> @@ -423,7 +423,7 @@ void sgx_encl_release(struct kref *ref)
> va_page = list_first_entry(&encl->va_pages, struct sgx_va_page,
> list);
> list_del(&va_page->list);
> - sgx_free_epc_page(va_page->epc_page);
> + sgx_encl_free_epc_page(va_page->epc_page);
> kfree(va_page);
> }
>
> @@ -686,7 +686,7 @@ struct sgx_epc_page *sgx_alloc_va_page(void)
> ret = __epa(sgx_get_epc_virt_addr(epc_page));
> if (ret) {
> WARN_ONCE(1, "EPA returned %d (0x%x)", ret, ret);
> - sgx_free_epc_page(epc_page);
> + sgx_encl_free_epc_page(epc_page);
> return ERR_PTR(-EFAULT);
> }
>
> @@ -735,3 +735,25 @@ bool sgx_va_page_full(struct sgx_va_page *va_page)
>
> return slot == SGX_VA_SLOT_COUNT;
> }
> +
> +/**
> + * sgx_encl_free_epc_page - free EPC page assigned to an enclave
> + * @page: EPC page to be freed
> + *
> + * Free EPC page assigned to an enclave. It does EREMOVE for the page, and
> + * only upon success, it puts the page back to free page list. Otherwise, it
> + * gives a WARNING to indicate page is leaked, and require reboot to retrieve
> + * leaked pages.
> + */
> +void sgx_encl_free_epc_page(struct sgx_epc_page *page)
> +{
> + int ret;
> +
> + WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> +
> + ret = __eremove(sgx_get_epc_virt_addr(page));
> + if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret))
> + return;
> +
> + sgx_free_epc_page(page);
> +}
> diff --git a/arch/x86/kernel/cpu/sgx/encl.h b/arch/x86/kernel/cpu/sgx/encl.h
> index d8d30ccbef4c..6e74f85b6264 100644
> --- a/arch/x86/kernel/cpu/sgx/encl.h
> +++ b/arch/x86/kernel/cpu/sgx/encl.h
> @@ -115,5 +115,6 @@ struct sgx_epc_page *sgx_alloc_va_page(void);
> unsigned int sgx_alloc_va_slot(struct sgx_va_page *va_page);
> void sgx_free_va_slot(struct sgx_va_page *va_page, unsigned int offset);
> bool sgx_va_page_full(struct sgx_va_page *va_page);
> +void sgx_encl_free_epc_page(struct sgx_epc_page *page);
>
> #endif /* _X86_ENCL_H */
> diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> index 90a5caf76939..772b9c648cf1 100644
> --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> @@ -47,7 +47,7 @@ static void sgx_encl_shrink(struct sgx_encl *encl, struct sgx_va_page *va_page)
> encl->page_cnt--;
>
> if (va_page) {
> - sgx_free_epc_page(va_page->epc_page);
> + sgx_encl_free_epc_page(va_page->epc_page);
> list_del(&va_page->list);
> kfree(va_page);
> }
> @@ -117,7 +117,7 @@ static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> return 0;
>
> err_out:
> - sgx_free_epc_page(encl->secs.epc_page);
> + sgx_encl_free_epc_page(encl->secs.epc_page);
> encl->secs.epc_page = NULL;
>
> err_out_backing:
> @@ -365,7 +365,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> mmap_read_unlock(current->mm);
>
> err_out_free:
> - sgx_free_epc_page(epc_page);
> + sgx_encl_free_epc_page(epc_page);
> kfree(encl_page);
>
> return ret;
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 13a7599ce7d4..b227629b1e9c 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -294,7 +294,7 @@ static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
>
> sgx_encl_ewb(encl->secs.epc_page, &secs_backing);
>
> - sgx_free_epc_page(encl->secs.epc_page);
> + sgx_encl_free_epc_page(encl->secs.epc_page);
> encl->secs.epc_page = NULL;
>
> sgx_encl_put_backing(&secs_backing, true);
> @@ -609,19 +609,15 @@ struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> * sgx_free_epc_page() - Free an EPC page
> * @page: an EPC page
> *
> - * Call EREMOVE for an EPC page and insert it back to the list of free pages.
> + * Put the EPC page back to the list of free pages. It's the caller's
> + * responsibility to make sure that the page is in uninitialized state. In other
> + * words, do EREMOVE, EWB or whatever operation is necessary before calling
> + * this function.
> */
> void sgx_free_epc_page(struct sgx_epc_page *page)
> {
> struct sgx_epc_section *section = &sgx_epc_sections[page->section];
> struct sgx_numa_node *node = section->node;
> - int ret;
> -
> - WARN_ON_ONCE(page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> -
> - ret = __eremove(sgx_get_epc_virt_addr(page));
> - if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret))
> - return;
>
> spin_lock(&node->lock);
>
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 653af8ca1a25..6b21a165500e 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -13,6 +13,11 @@
> #undef pr_fmt
> #define pr_fmt(fmt) "sgx: " fmt
>
> +/* Error message for EREMOVE failure, when kernel is about to leak EPC page */
> +#define EREMOVE_ERROR_MESSAGE \
> + "EREMOVE returned %d (0x%x) and an EPC page was leaked. SGX may become unusuable. " \
> + "This is likely a kernel bug. Refer to Documentation/x86/sgx.rst for more information."
Why this needs to be here and not open coded where it is used?
> +
> #define SGX_MAX_EPC_SECTIONS 8
> #define SGX_EEXTEND_BLOCK_SIZE 256
> #define SGX_NR_TO_SCAN 16
> --
> 2.30.2
>
>
/Jarkko
next prev parent reply other threads:[~2021-03-26 19:50 UTC|newest]
Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 7:29 [PATCH v3 00/25] KVM SGX virtualization support Kai Huang
2021-03-19 7:22 ` [PATCH v3 01/25] x86/cpufeatures: Make SGX_LC feature bit depend on SGX bit Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Kai Huang
2021-03-19 7:22 ` [PATCH v3 02/25] x86/cpufeatures: Add SGX1 and SGX2 sub-features Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:22 ` [PATCH v3 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page() Kai Huang
2021-03-22 18:16 ` Borislav Petkov
2021-03-22 18:56 ` Sean Christopherson
2021-03-22 19:11 ` Paolo Bonzini
2021-03-22 20:43 ` Kai Huang
2021-03-23 16:40 ` Paolo Bonzini
2021-03-22 19:15 ` Borislav Petkov
2021-03-22 19:37 ` Sean Christopherson
2021-03-22 20:36 ` Kai Huang
2021-03-22 21:06 ` Borislav Petkov
2021-03-22 22:06 ` Kai Huang
2021-03-22 22:37 ` Borislav Petkov
2021-03-22 23:16 ` Kai Huang
2021-03-23 15:45 ` Sean Christopherson
2021-03-23 16:06 ` Borislav Petkov
2021-03-23 16:21 ` Sean Christopherson
2021-03-23 16:32 ` Borislav Petkov
2021-03-23 16:51 ` Sean Christopherson
2021-03-24 9:38 ` Kai Huang
2021-03-24 10:09 ` Paolo Bonzini
2021-03-24 10:48 ` Kai Huang
2021-03-24 11:24 ` Paolo Bonzini
2021-03-24 23:23 ` Kai Huang
2021-03-24 23:39 ` Paolo Bonzini
2021-03-24 23:46 ` Kai Huang
2021-03-25 8:42 ` Borislav Petkov
2021-03-25 9:38 ` Kai Huang
2021-03-25 16:52 ` Borislav Petkov
2021-03-24 9:28 ` Jarkko Sakkinen
2021-03-23 16:38 ` Paolo Bonzini
2021-03-23 17:02 ` Sean Christopherson
2021-03-23 17:06 ` Paolo Bonzini
2021-03-23 17:16 ` Sean Christopherson
2021-03-23 18:16 ` Borislav Petkov
2021-03-24 9:26 ` Jarkko Sakkinen
2021-03-22 22:23 ` Kai Huang
2021-03-25 9:30 ` [PATCH v4 " Kai Huang
2021-03-26 19:48 ` Jarkko Sakkinen [this message]
2021-03-26 20:38 ` Kai Huang
2021-03-26 21:39 ` Jarkko Sakkinen
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Kai Huang
2021-03-19 7:22 ` [PATCH v3 04/25] x86/sgx: Add SGX_CHILD_PRESENT hardware error code Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:22 ` [PATCH v3 05/25] x86/sgx: Introduce virtual EPC for use by KVM guests Kai Huang
2021-03-25 9:36 ` Kai Huang
2021-03-26 15:03 ` Borislav Petkov
2021-03-26 15:17 ` Dave Hansen
2021-03-26 15:29 ` Borislav Petkov
2021-03-26 15:35 ` Dave Hansen
2021-03-26 17:02 ` Borislav Petkov
2021-03-31 1:10 ` Kai Huang
2021-03-31 6:44 ` Boris Petkov
2021-03-31 6:51 ` Kai Huang
2021-03-31 7:44 ` Boris Petkov
2021-03-31 8:53 ` Kai Huang
2021-03-31 12:20 ` Kai Huang
2021-04-01 18:31 ` Borislav Petkov
2021-04-01 23:38 ` Kai Huang
2021-04-01 9:45 ` Kai Huang
2021-04-01 9:42 ` [PATCH v4 " Kai Huang
2021-04-05 9:01 ` [PATCH v3 " Borislav Petkov
2021-04-05 21:46 ` Kai Huang
2021-04-06 8:28 ` Borislav Petkov
2021-04-06 9:04 ` Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:22 ` [PATCH v3 06/25] x86/cpu/intel: Allow SGX virtualization without Launch Control support Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 07/25] x86/sgx: Initialize virtual EPC driver even when SGX driver is disabled Kai Huang
2021-04-02 9:48 ` Borislav Petkov
2021-04-02 11:08 ` Kai Huang
2021-04-02 11:22 ` Borislav Petkov
2021-04-02 11:38 ` Kai Huang
2021-04-02 15:42 ` Sean Christopherson
2021-04-02 19:08 ` Kai Huang
2021-04-02 19:19 ` Borislav Petkov
2021-04-02 19:30 ` Sean Christopherson
2021-04-02 19:46 ` Borislav Petkov
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Kai Huang
2021-03-19 7:23 ` [PATCH v3 08/25] x86/sgx: Expose SGX architectural definitions to the kernel Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 09/25] x86/sgx: Move ENCLS leaf definitions to sgx.h Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 10/25] x86/sgx: Add SGX2 ENCLS leaf definitions (EAUG, EMODPR and EMODT) Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 11/25] x86/sgx: Add encls_faulted() helper Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 12/25] x86/sgx: Add helper to update SGX_LEPUBKEYHASHn MSRs Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Kai Huang
2021-03-19 7:23 ` [PATCH v3 13/25] x86/sgx: Add helpers to expose ECREATE and EINIT to KVM Kai Huang
2021-04-05 9:07 ` Borislav Petkov
2021-04-05 21:44 ` Kai Huang
2021-04-06 7:40 ` Borislav Petkov
2021-04-06 8:59 ` Kai Huang
2021-04-06 9:09 ` Borislav Petkov
2021-04-06 9:24 ` Kai Huang
2021-04-06 9:32 ` Borislav Petkov
2021-04-06 9:41 ` Kai Huang
2021-04-06 17:08 ` Borislav Petkov
2021-04-06 20:33 ` Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 14/25] x86/sgx: Move provisioning device creation out of SGX driver Kai Huang
2021-04-07 10:03 ` [tip: x86/sgx] " tip-bot2 for Sean Christopherson
2021-03-19 7:23 ` [PATCH v3 15/25] KVM: x86: Export kvm_mmu_gva_to_gpa_{read,write}() for SGX (VMX) Kai Huang
2021-03-19 7:23 ` [PATCH v3 16/25] KVM: x86: Define new #PF SGX error code bit Kai Huang
2021-03-19 7:23 ` [PATCH v3 17/25] KVM: x86: Add support for reverse CPUID lookup of scattered features Kai Huang
2021-03-19 7:23 ` [PATCH v3 18/25] KVM: x86: Add reverse-CPUID lookup support for scattered SGX features Kai Huang
2021-03-19 7:23 ` [PATCH v3 19/25] KVM: VMX: Add basic handling of VM-Exit from SGX enclave Kai Huang
2021-03-19 7:23 ` [PATCH v3 20/25] KVM: VMX: Frame in ENCLS handler for SGX virtualization Kai Huang
2021-03-19 7:23 ` [PATCH v3 21/25] KVM: VMX: Add SGX ENCLS[ECREATE] handler to enforce CPUID restrictions Kai Huang
2021-03-19 7:23 ` [PATCH v3 22/25] KVM: VMX: Add emulation of SGX Launch Control LE hash MSRs Kai Huang
2021-03-19 7:23 ` [PATCH v3 23/25] KVM: VMX: Add ENCLS[EINIT] handler to support SGX Launch Control (LC) Kai Huang
2021-03-19 7:23 ` [PATCH v3 24/25] KVM: VMX: Enable SGX virtualization for SGX1, SGX2 and LC Kai Huang
2021-03-19 7:24 ` [PATCH v3 25/25] KVM: x86: Add capability to grant VM access to privileged SGX attribute Kai Huang
2021-03-19 14:52 ` [PATCH v3 00/25] KVM SGX virtualization support Jarkko Sakkinen
2021-03-22 10:03 ` Kai Huang
2021-03-22 10:31 ` Borislav Petkov
2021-03-26 22:46 ` Jarkko Sakkinen
2021-03-28 21:01 ` Huang, Kai
2021-03-31 23:23 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YF46ndD3rdotgOpl@kernel.org \
--to=jarkko@kernel.org \
--cc=bp@alien8.de \
--cc=dave.hansen@intel.com \
--cc=haitao.huang@intel.com \
--cc=hpa@zytor.com \
--cc=kai.huang@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sgx@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=rick.p.edgecombe@intel.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).