RE: [RFC PATCH v3 01/10] x86/sgx: Introduce mechanism to prevent new initializations of EPC pages

["Zhang, Cathy" <cathy.zhang@intel.com>]

Hi Jarkko,

> -----Original Message-----
> From: Jarkko Sakkinen <jarkko@kernel.org>
> Sent: Sunday, April 3, 2022 4:14 PM
> To: Zhang, Cathy <cathy.zhang@intel.com>
> Cc: linux-sgx@vger.kernel.org; x86@kernel.org; Chatre, Reinette
> <reinette.chatre@intel.com>; Hansen, Dave <dave.hansen@intel.com>; Raj,
> Ashok <ashok.raj@intel.com>
> Subject: Re: [RFC PATCH v3 01/10] x86/sgx: Introduce mechanism to prevent
> new initializations of EPC pages
> 
> On Fri, Apr 01, 2022 at 10:24:00PM +0800, Cathy Zhang wrote:
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c
> > b/arch/x86/kernel/cpu/sgx/ioctl.c index b3c2e8d58142..00668e50848d
> > 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -147,6 +147,7 @@ static int sgx_encl_create(struct sgx_encl *encl,
> > struct sgx_secs *secs)  static long sgx_ioc_enclave_create(struct
> > sgx_encl *encl, void __user *arg)  {
> >  	struct sgx_enclave_create create_arg;
> > +	int srcu_idx;
> >  	void *secs;
> >  	int ret;
> >
> > @@ -162,9 +163,20 @@ static long sgx_ioc_enclave_create(struct
> > sgx_encl *encl, void __user *arg)
> >
> >  	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE))
> >  		ret = -EFAULT;
> > -	else
> > +	else {
> > +		srcu_idx = srcu_read_lock(&sgx_lock_epc_srcu);
> > +		if (sgx_epc_is_locked()) {
> > +			srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +			ret = -EBUSY;
> > +			goto out;
> > +		}
> > +
> >  		ret = sgx_encl_create(encl, secs);
> >
> > +		srcu_read_unlock(&sgx_lock_epc_srcu, srcu_idx);
> > +	}
> > +
> > +out:
> 
> Nit: I'd rename this as 'err:'.

Renamed as 'err'.

> 
> >  	kfree(secs);
> >  	return ret;
> >  }
> 
> Please, take advantage of the label you created anyway:
> 
>   	if (copy_from_user(secs, (void __user *)create_arg.src, PAGE_SIZE {
>   		ret = -EFAULT;
>                 goto out;
>         }
> 
> Then fail cases have the exact same rollback procedure, and the code is
> easier to read because it is less nested.

Update is done. The error branches in this function will all "goto err".

> 
> BR, Jarkko
>