Re: [PATCH] x86/sgx: fix a NULL pointer

[Haitao Huang]

Hi
On Mon, 17 Jul 2023 20:39:31 -0500, Huang, Kai <kai.huang@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> > ---
> >  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