From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 25615C433DB for ; Fri, 26 Mar 2021 19:50:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 132B061A21 for ; Fri, 26 Mar 2021 19:50:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230363AbhCZTtg (ORCPT ); Fri, 26 Mar 2021 15:49:36 -0400 Received: from mail.kernel.org ([198.145.29.99]:55332 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230150AbhCZTtP (ORCPT ); Fri, 26 Mar 2021 15:49:15 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id EF2EF6196C; Fri, 26 Mar 2021 19:49:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1616788155; bh=lAxCHO3mwbr8j2AnnBzP3nXUOrdvyMEE2z/EuSlIbfE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GsdDHx35gVb6OrOsB9zBnJHrV4Q/7lE6Tau01U9EJ1T3ftL6Vg5W4VB3j+kYSHQ2U lFcqLQeHKxDCIBzUByhB6fIx2egkA5NL3LC2Q4OdNdu8+PbmA3xoiWJIvrcU+9Fs0w G4Xi/rDnB7r+jAhGxTu0VPMoLqlrWsjApD4UE1xpJs8qHYuPOL0AsXv7DhAns0N9a8 ksvfWqztoxNOk7L54CBrW8dX+6h8vwCNc01+aI+hS3ej6m0GJvWaj5aPP8TBweNfdS 1VAaGmyVMK6Jxd0F3ivqYB1l5Dkhb+ruWnPqu3HmCBA4lbbcHl6okva5iAm455lakP jpaF7mgMPRXcw== Date: Fri, 26 Mar 2021 21:48:45 +0200 From: Jarkko Sakkinen To: Kai Huang 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() Message-ID: References: <062acb801926b2ade2f9fe1672afb7113453a741.1616136308.git.kai.huang@intel.com> <20210325093057.122834-1-kai.huang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210325093057.122834-1-kai.huang@intel.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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