linux-kernel - Re: [PATCH 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a1f3ab9bdffe78b72b22237d46f706a3c19c2900.camel@intel.com>
Date:   Tue, 02 Mar 2021 13:32:49 +1300
From:   Kai Huang <kai.huang@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, x86@...nel.org, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org, jarkko@...nel.org, luto@...nel.org,
        dave.hansen@...el.com, rick.p.edgecombe@...el.com,
        haitao.huang@...el.com, pbonzini@...hat.com, bp@...en8.de,
        tglx@...utronix.de, mingo@...hat.com, hpa@...or.com
Subject: Re: [PATCH 03/25] x86/sgx: Wipe out EREMOVE from sgx_free_epc_page()

On Mon, 2021-03-01 at 09:29 -0800, Sean Christopherson wrote:
> On Mon, Mar 01, 2021, Kai Huang wrote:
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
> > index 7449ef33f081..a7dc86e87a09 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -381,6 +381,26 @@ const struct vm_operations_struct sgx_vm_ops = {
> >  	.access = sgx_vma_access,
> >  };
> >  
> > 
> > 
> > 
> > +static void sgx_encl_free_epc_page(struct sgx_epc_page *epc_page)
> > +{
> > +	int ret;
> > +
> > +	WARN_ON_ONCE(epc_page->flags & SGX_EPC_PAGE_RECLAIMER_TRACKED);
> > +
> > +	ret = __eremove(sgx_get_epc_virt_addr(epc_page));
> > +	if (WARN_ONCE(ret, "EREMOVE returned %d (0x%x)", ret, ret)) {
> 
> This can be ENCLS_WARN, especially if you're printing a separate error message
> about leaking the page.  That being said, I'm not sure a seperate error message
> is a good idea.  If other stuff gets dumped to the kernel log between the WARN
> and the pr_err_once(), it may not be clear to admins that the two events are
> directly connected.  It's even possible the prints could come from two different
> CPUs.

Good point. Thanks for educating me :)

> 
> Why not dump a short blurb in the WARN itself?  The error message can be thrown
> in a define if the line length is too obnoxious (it's ~109 chars if embedded
> directly).
> 
> #define EREMOVE_ERROR_MESSAGE \
> 	"EREMOVE returned %d (0x%x).  EPC page leaked, reboot recommended."
> 
> 	if (WARN_ONCE(ret, EREMOVE_ERROR_MESSAGE, ret, ret))

Will do in your way. Thanks!

> 
> > +		/*
> > +		 * Give a message to remind EPC page is leaked, and requires
> > +		 * machine reboot to get leaked pages back. This can be improved
> > +		 * in the future by adding stats of leaked pages, etc.
> > +		 */
> > +		pr_err_once("EPC page is leaked. Require machine reboot to get leaked pages back.\n");
> > +		return;
> > +	}
> > +
> > +	sgx_free_epc_page(epc_page);
> > +}
> > +
> >  /**
> >   * sgx_encl_release - Destroy an enclave instance
> >   * @kref:	address of a kref inside &sgx_encl