Re: [PATCH] x86/sgx: Roof the number of pages process in SGX_IOC_ENCLAVE_ADD_PAGES

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Wed, Sep 09, 2020 at 10:55:58AM +0100, Darren Kenny wrote:
> On Tuesday, 2020-09-08 at 22:00:42 +03, Jarkko Sakkinen wrote:
> > Remove 'count' from struct sgx_enclave_add_pages and return number of
> > pages processed as a positive return as there is no reasonable use for
> > -EINTR for the caller. Then, roof the processing with a fixed constant
> > and break out the loop also when this value is surpassed.
> >
> > Link: https://lore.kernel.org/linux-sgx/20200626153400.GE27151@zn.tnic/
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  arch/x86/include/uapi/asm/sgx.h |  2 --
> >  arch/x86/kernel/cpu/sgx/ioctl.c | 10 ++++------
> >  arch/x86/kernel/cpu/sgx/sgx.h   |  1 +
> >  3 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > index b3d5ccf5b976..1564d7f88597 100644
> > --- a/arch/x86/include/uapi/asm/sgx.h
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -45,7 +45,6 @@ struct sgx_enclave_create  {
> >   * @length:	length of the data (multiple of the page size)
> >   * @secinfo:	address for the SECINFO data
> >   * @flags:	page control flags
> > - * @count:	number of bytes added (multiple of the page size)
> >   */
> >  struct sgx_enclave_add_pages {
> >  	__u64	src;
> > @@ -53,7 +52,6 @@ struct sgx_enclave_add_pages {
> >  	__u64	length;
> >  	__u64	secinfo;
> >  	__u64	flags;
> > -	__u64	count;
> >  };
> >  
> >  /**
> > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c
> > index 6c3c6177b690..0ae00fa9b589 100644
> > --- a/arch/x86/kernel/cpu/sgx/ioctl.c
> > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c
> > @@ -490,7 +490,7 @@ static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> >   * caller.
> >   *
> >   * Return:
> > - *   0 on success,
> > + *   length of the data processed on success,
> >   *   -EACCES if an executable source page is located in a noexec partition,
> >   *   -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
> >   *   -errno otherwise
> > @@ -530,8 +530,8 @@ static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
> >  		return -EINVAL;
> >  
> >  	for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> > -		if (signal_pending(current)) {
> > -			ret = -EINTR;
> > +		if (c == SGX_MAX_ADD_PAGES_LENGTH || signal_pending(current)) {
> 
> I know that SGX_MAX_ADD_PAGES_LENGTH may be a multiple of PAGE_SIZE, but
> to be sure that nothing breaks here in the future, I think it would be
> better to use '>=' as the comparison rather than just '=='.


I'm not forseeing this. We will change the comparison or remove it if
the constant is no longer required.

> 
> Also, the indentation seems off here w.r.t. using TABs when the break
> below is using spaces - would be good to have them the same at least.

Ugh, that must be my mistake. I'll check these before squashing this.

Thank you.



/Jarkko