| Message ID | bca979301e2fbb55c314fed29c06e72e82074fba.1652131695.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/5] x86/sgx: Disconnect backing page references from dirty status | expand |
On Mon, May 09, 2022 at 02:48:03PM -0700, Reinette Chatre wrote: > A PCMD (Paging Crypto MetaData) page contains the PCMD > structures of enclave pages that have been encrypted and > moved to the shmem backing store. When all enclave pages > sharing a PCMD page are loaded in the enclave, there is no > need for the PCMD page and it can be truncated from the > backing store. > > A few issues appeared around the truncation of PCMD pages. The > known issues have been addressed but the PCMD handling code could > be made more robust by loudly complaining if any new issue appears > in this area. > > Add a check that will complain once with a WARN if the PCMD page is not > actually empty after it has been truncated. There should never be data > in the PCMD page at this point since it is always updated with the > enclave mutex held. > > Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index d1d4e8572702..af972dbad965 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -183,12 +183,19 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, > kunmap_atomic(pcmd_page); > kunmap_atomic((void *)(unsigned long)pginfo.contents); > > + get_page(b.pcmd); > sgx_encl_put_backing(&b); > > sgx_encl_truncate_backing_page(encl, page_index); > > - if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) > + if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) { > sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off)); > + pcmd_page = kmap_atomic(b.pcmd); > + WARN_ON_ONCE(memchr_inv(pcmd_page, 0, PAGE_SIZE)); Is WARN necessary, or would it make more sense to use pr_warn()? It would give a better chance to collect information if "panic_on_warn" is set for the running kernel. > + kunmap_atomic(pcmd_page); > + } > + > + put_page(b.pcmd); > > return ret; > } > -- > 2.25.1 > BR, Jarkko
Hi Jarkko, On 5/11/2022 3:36 AM, Jarkko Sakkinen wrote: > On Mon, May 09, 2022 at 02:48:03PM -0700, Reinette Chatre wrote: >> A PCMD (Paging Crypto MetaData) page contains the PCMD >> structures of enclave pages that have been encrypted and >> moved to the shmem backing store. When all enclave pages >> sharing a PCMD page are loaded in the enclave, there is no >> need for the PCMD page and it can be truncated from the >> backing store. >> >> A few issues appeared around the truncation of PCMD pages. The >> known issues have been addressed but the PCMD handling code could >> be made more robust by loudly complaining if any new issue appears >> in this area. >> >> Add a check that will complain once with a WARN if the PCMD page is not >> actually empty after it has been truncated. There should never be data >> in the PCMD page at this point since it is always updated with the >> enclave mutex held. >> >> Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> arch/x86/kernel/cpu/sgx/encl.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c >> index d1d4e8572702..af972dbad965 100644 >> --- a/arch/x86/kernel/cpu/sgx/encl.c >> +++ b/arch/x86/kernel/cpu/sgx/encl.c >> @@ -183,12 +183,19 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, >> kunmap_atomic(pcmd_page); >> kunmap_atomic((void *)(unsigned long)pginfo.contents); >> >> + get_page(b.pcmd); >> sgx_encl_put_backing(&b); >> >> sgx_encl_truncate_backing_page(encl, page_index); >> >> - if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) >> + if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) { >> sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off)); >> + pcmd_page = kmap_atomic(b.pcmd); >> + WARN_ON_ONCE(memchr_inv(pcmd_page, 0, PAGE_SIZE)); > > Is WARN necessary, or would it make more sense to use pr_warn()? > I will change it to pr_warn(). > It would give a better chance to collect information if "panic_on_warn" is > set for the running kernel. > Reinette
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index d1d4e8572702..af972dbad965 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -183,12 +183,19 @@ static int __sgx_encl_eldu(struct sgx_encl_page *encl_page, kunmap_atomic(pcmd_page); kunmap_atomic((void *)(unsigned long)pginfo.contents); + get_page(b.pcmd); sgx_encl_put_backing(&b); sgx_encl_truncate_backing_page(encl, page_index); - if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) + if (pcmd_page_empty && !pcmd_page_in_use(encl, pcmd_first_page)) { sgx_encl_truncate_backing_page(encl, PFN_DOWN(page_pcmd_off)); + pcmd_page = kmap_atomic(b.pcmd); + WARN_ON_ONCE(memchr_inv(pcmd_page, 0, PAGE_SIZE)); + kunmap_atomic(pcmd_page); + } + + put_page(b.pcmd); return ret; }
A PCMD (Paging Crypto MetaData) page contains the PCMD structures of enclave pages that have been encrypted and moved to the shmem backing store. When all enclave pages sharing a PCMD page are loaded in the enclave, there is no need for the PCMD page and it can be truncated from the backing store. A few issues appeared around the truncation of PCMD pages. The known issues have been addressed but the PCMD handling code could be made more robust by loudly complaining if any new issue appears in this area. Add a check that will complain once with a WARN if the PCMD page is not actually empty after it has been truncated. There should never be data in the PCMD page at this point since it is always updated with the enclave mutex held. Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)