| Message ID | 20230718004529.16404-1-haitao.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sgx: fix a NULL pointer | expand |
On Mon, 2023-07-17 at 17:45 -0700, Haitao Huang wrote: > Under heavy load, the SGX EPC reclaimer (ksgxd) may reclaim the SECS EPC > page for an enclave and set encl->secs.epc_page to NULL. But the SECS > EPC page is used for EAUG in the SGX #PF handler without checking for > NULL and reloading. > > Fix this by checking if SECS is loaded before EAUG and loading it if it was > reclaimed. Nit: Looks the sentence break of the second paragraph isn't consistent with the first paragraph, i.e., looks the first line is too long and the "was" should be broken to the second line. And I think even for this simple patch, you are sending too frequently. The patch subject should contain the version number too so people can distinguish it from the previous one. Even better, please use "--base=auto" when generating the patch so people can know the base commit to apply to. > > Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave") > Cc: stable@vger.kernel.org > Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> > --- > arch/x86/kernel/cpu/sgx/encl.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c > index 2a0e90fe2abc..2ab544da1664 100644 > --- a/arch/x86/kernel/cpu/sgx/encl.c > +++ b/arch/x86/kernel/cpu/sgx/encl.c > @@ -235,6 +235,16 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, > return epc_page; > } > > +static struct sgx_epc_page *sgx_encl_load_secs(struct sgx_encl *encl) > +{ > + struct sgx_epc_page *epc_page = encl->secs.epc_page; > + > + if (!epc_page) > + epc_page = sgx_encl_eldu(&encl->secs, NULL); > + > + return epc_page; > +} > + > static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, > struct sgx_encl_page *entry) > { > @@ -248,11 +258,9 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, > return entry; > } > > - if (!(encl->secs.epc_page)) { > - epc_page = sgx_encl_eldu(&encl->secs, NULL); > - if (IS_ERR(epc_page)) > - return ERR_CAST(epc_page); > - } > + epc_page = sgx_encl_load_secs(encl); > + if (IS_ERR(epc_page)) > + return ERR_CAST(epc_page); > > epc_page = sgx_encl_eldu(entry, encl->secs.epc_page); > if (IS_ERR(epc_page)) > @@ -339,6 +347,13 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma, > > mutex_lock(&encl->lock); > > + epc_page = sgx_encl_load_secs(encl); > + if (IS_ERR(epc_page)) { > + if (PTR_ERR(epc_page) == -EBUSY) > + vmret = VM_FAULT_NOPAGE; ^ Nit: two spaces here (yeah you copied from the existing code, and sorry forgot to point out in the previous version). > + goto err_out_unlock; > + } > + > epc_page = sgx_alloc_epc_page(encl_page, false); > if (IS_ERR(epc_page)) { > if (PTR_ERR(epc_page) == -EBUSY)
Hi On Mon, 17 Jul 2023 20:39:31 -0500, Huang, Kai <kai.huang@intel.com> wrote: > On Mon, 2023-07-17 at 17:45 -0700, Haitao Huang wrote: >> Under heavy load, the SGX EPC reclaimer (ksgxd) may reclaim the SECS EPC >> page for an enclave and set encl->secs.epc_page to NULL. But the SECS >> EPC page is used for EAUG in the SGX #PF handler without checking for >> NULL and reloading. >> >> Fix this by checking if SECS is loaded before EAUG and loading it if it >> was >> reclaimed. > > Nit: > > Looks the sentence break of the second paragraph isn't consistent with > the first > paragraph, i.e., looks the first line is too long and the "was" should > be broken > to the second line. > Yeah, I think I forgot to reformat this line after revising. > And I think even for this simple patch, you are sending too frequently. > The > patch subject should contain the version number too so people can > distinguish it > from the previous one. Even better, please use "--base=auto" when > generating > the patch so people can know the base commit to apply to. I'll send the next one as V2 and start a separate email thread. >> >> Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an >> initialized enclave") >> Cc: stable@vger.kernel.org >> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> >> --- >> arch/x86/kernel/cpu/sgx/encl.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/sgx/encl.c >> b/arch/x86/kernel/cpu/sgx/encl.c >> index 2a0e90fe2abc..2ab544da1664 100644 >> --- a/arch/x86/kernel/cpu/sgx/encl.c >> +++ b/arch/x86/kernel/cpu/sgx/encl.c >> @@ -235,6 +235,16 @@ static struct sgx_epc_page *sgx_encl_eldu(struct >> sgx_encl_page *encl_page, >> return epc_page; >> } >> >> +static struct sgx_epc_page *sgx_encl_load_secs(struct sgx_encl *encl) >> +{ >> + struct sgx_epc_page *epc_page = encl->secs.epc_page; >> + >> + if (!epc_page) >> + epc_page = sgx_encl_eldu(&encl->secs, NULL); >> + >> + return epc_page; >> +} >> + >> static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl >> *encl, >> struct sgx_encl_page *entry) >> { >> @@ -248,11 +258,9 @@ static struct sgx_encl_page >> *__sgx_encl_load_page(struct sgx_encl *encl, >> return entry; >> } >> >> - if (!(encl->secs.epc_page)) { >> - epc_page = sgx_encl_eldu(&encl->secs, NULL); >> - if (IS_ERR(epc_page)) >> - return ERR_CAST(epc_page); >> - } >> + epc_page = sgx_encl_load_secs(encl); >> + if (IS_ERR(epc_page)) >> + return ERR_CAST(epc_page); >> >> epc_page = sgx_encl_eldu(entry, encl->secs.epc_page); >> if (IS_ERR(epc_page)) >> @@ -339,6 +347,13 @@ static vm_fault_t sgx_encl_eaug_page(struct >> vm_area_struct *vma, >> >> mutex_lock(&encl->lock); >> >> + epc_page = sgx_encl_load_secs(encl); >> + if (IS_ERR(epc_page)) { >> + if (PTR_ERR(epc_page) == -EBUSY) >> + vmret = VM_FAULT_NOPAGE; > ^ > Nit: two spaces here (yeah you copied from the existing code, and sorry > forgot > to point out in the previous version). > Sure. will fix in V2. Thanks Haitao
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c index 2a0e90fe2abc..2ab544da1664 100644 --- a/arch/x86/kernel/cpu/sgx/encl.c +++ b/arch/x86/kernel/cpu/sgx/encl.c @@ -235,6 +235,16 @@ static struct sgx_epc_page *sgx_encl_eldu(struct sgx_encl_page *encl_page, return epc_page; } +static struct sgx_epc_page *sgx_encl_load_secs(struct sgx_encl *encl) +{ + struct sgx_epc_page *epc_page = encl->secs.epc_page; + + if (!epc_page) + epc_page = sgx_encl_eldu(&encl->secs, NULL); + + return epc_page; +} + static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, struct sgx_encl_page *entry) { @@ -248,11 +258,9 @@ static struct sgx_encl_page *__sgx_encl_load_page(struct sgx_encl *encl, return entry; } - if (!(encl->secs.epc_page)) { - epc_page = sgx_encl_eldu(&encl->secs, NULL); - if (IS_ERR(epc_page)) - return ERR_CAST(epc_page); - } + epc_page = sgx_encl_load_secs(encl); + if (IS_ERR(epc_page)) + return ERR_CAST(epc_page); epc_page = sgx_encl_eldu(entry, encl->secs.epc_page); if (IS_ERR(epc_page)) @@ -339,6 +347,13 @@ static vm_fault_t sgx_encl_eaug_page(struct vm_area_struct *vma, mutex_lock(&encl->lock); + epc_page = sgx_encl_load_secs(encl); + if (IS_ERR(epc_page)) { + if (PTR_ERR(epc_page) == -EBUSY) + vmret = VM_FAULT_NOPAGE; + goto err_out_unlock; + } + epc_page = sgx_alloc_epc_page(encl_page, false); if (IS_ERR(epc_page)) { if (PTR_ERR(epc_page) == -EBUSY)
Under heavy load, the SGX EPC reclaimer (ksgxd) may reclaim the SECS EPC page for an enclave and set encl->secs.epc_page to NULL. But the SECS EPC page is used for EAUG in the SGX #PF handler without checking for NULL and reloading. Fix this by checking if SECS is loaded before EAUG and loading it if it was reclaimed. Fixes: 5a90d2c3f5ef ("x86/sgx: Support adding of pages to an initialized enclave") Cc: stable@vger.kernel.org Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com> --- arch/x86/kernel/cpu/sgx/encl.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)