Re: [PATCH v2 2/2] x86/sgx: account backing pages

[Kristen Carlson Accardi]

On Sat, 2022-01-08 at 18:05 +0200, Jarkko Sakkinen wrote:
> On Fri, Jan 07, 2022 at 10:16:17AM -0800, Kristen Carlson Accardi
> wrote:
> > SGX may allow EPC pages to be overcommitted. If the system is
> > out of enclave memory, EPC pages are swapped to normal RAM via
> > a per enclave shared memory area. This shared memory is not
> > charged to the enclave or the task mapping it, making it hard
> > to account for using normal methods.
> > 
> > In order to avoid unlimited usage of normal RAM, enclaves must be
> > charged for each new page used for backing storage, and uncharged
> > when they are no longer using a backing page.
> > 
> > Modify the existing flow for requesting backing pages to reduce the
> > available backing page counter and confirm that the limit has not
> > been exceeded. Backing page usage for loading EPC pages back out of
> > the shared memory do not incur a charge.
> > 
> > When a backing page is released from usage, increment the available
> > backing page counter.
> > 
> > When swapping EPC pages to RAM, in addition to storing the page
> > contents, SGX must store some additional metadata to protect
> > against a malicious kernel when the page is swapped back in. This
> > additional metadata is called Paging Crypto MetaData. PCMD is
> > allocated from the same shared memory area as the backing page
> > contents and consumes RAM the same way.
> > 
> > PCMD is 128 bytes in size, and there is one PCMD structure per
> > page written to shared RAM. The page index for the PCMD page is
> > calculated from the page index of the backing page, so it is
> > possible
> > that the PCMD structures are not packed into the minimum number of
> > pages possible. If 32 PCMDs can fit onto a single page, then PCMD
> > usage is 1/32 of total EPC pages. In the worst case, PCMD can
> > consume the same amount of RAM as EPC backing pages (1:1). For
> > simplicity, this implementation does not account for PCMD page
> > usage.
> > 
> > Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> > ---
> >  arch/x86/kernel/cpu/sgx/encl.c | 76
> > ++++++++++++++++++++++++++++++++--
> >  arch/x86/kernel/cpu/sgx/encl.h |  6 ++-
> >  arch/x86/kernel/cpu/sgx/main.c |  6 +--
> >  3 files changed, 80 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/sgx/encl.c
> > b/arch/x86/kernel/cpu/sgx/encl.c
> > index 001808e3901c..8be6f0592bdc 100644
> > --- a/arch/x86/kernel/cpu/sgx/encl.c
> > +++ b/arch/x86/kernel/cpu/sgx/encl.c
> > @@ -32,7 +32,7 @@ static int __sgx_encl_eldu(struct sgx_encl_page
> > *encl_page,
> >  	else
> >  		page_index = PFN_DOWN(encl->size);
> >  
> > -	ret = sgx_encl_get_backing(encl, page_index, &b);
> > +	ret = sgx_encl_lookup_backing(encl, page_index, &b);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -407,6 +407,12 @@ void sgx_encl_release(struct kref *ref)
> >  			sgx_encl_free_epc_page(entry->epc_page);
> >  			encl->secs_child_cnt--;
> >  			entry->epc_page = NULL;
> > +		} else {
> > +			/*
> > +			 * If there is no epc_page, it means it has
> > been
> > +			 * swapped out. Uncharge the backing storage.
> > +			 */
> > +			sgx_uncharge_mem();
> >  		}
> >  
> >  		kfree(entry);
> > @@ -574,8 +580,8 @@ static struct page
> > *sgx_encl_get_backing_page(struct sgx_encl *encl,
> >   *   0 on success,
> >   *   -errno otherwise.
> >   */
> > -int sgx_encl_get_backing(struct sgx_encl *encl, unsigned long
> > page_index,
> > -			 struct sgx_backing *backing)
> > +static int sgx_encl_get_backing(struct sgx_encl *encl, unsigned
> > long page_index,
> > +				struct sgx_backing *backing)
> >  {
> >  	pgoff_t pcmd_index = PFN_DOWN(encl->size) + 1 + (page_index >>
> > 5);
> >  	struct page *contents;
> > @@ -601,6 +607,62 @@ int sgx_encl_get_backing(struct sgx_encl
> > *encl, unsigned long page_index,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * sgx_encl_alloc_backing() - allocate a new backing storage page
> > + * @encl:	an enclave pointer
> > + * @page_index:	enclave page index
> > + * @backing:	data for accessing backing storage for the page
> > + *
> > + * Confirm that the global overcommit limit has not been reached
> > before
> > + * requesting a new backing storage page for storing the encrypted
> > contents
> > + * and Paging Crypto MetaData (PCMD) of an enclave page. This is
> > called when
> > + * there is no existing backing page, just before writing to the
> > backing
> > + * storage with EWB.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -errno otherwise.
> > + */
> > +int sgx_encl_alloc_backing(struct sgx_encl *encl, unsigned long
> > page_index,
> > +			   struct sgx_backing *backing)
> > +{
> > +	int ret;
> > +
> > +	if (sgx_charge_mem())
> > +		return -ENOMEM;
> > +
> > +	ret = sgx_encl_get_backing(encl, page_index, backing);
> > +	if (ret)
> > +		sgx_uncharge_mem();
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * sgx_encl_lookup_backing() - retrieve an existing backing
> > storage page
> > + * @encl:	an enclave pointer
> > + * @page_index:	enclave page index
> > + * @backing:	data for accessing backing storage for the page
> > + *
> > + * Retrieve a backing page for loading data back into an EPC page
> > with ELDU.
> > + * This call does not cause a charge to the overcommit limit
> > because a page
> > + * has already been allocated, but has been swapped out or is in
> > RAM
> > + *
> > + * It is the caller's responsibility to ensure that it is
> > appropriate to
> > + * use sgx_encl_lookup_backing() rather than
> > sgx_encl_alloc_backing(). If
> > + * lookup is not used correctly, this will cause an allocation
> > that is
> > + * not accounted for.
> > + *
> > + * Return:
> > + *   0 on success,
> > + *   -errno otherwise.
> > + */
> > +int sgx_encl_lookup_backing(struct sgx_encl *encl, unsigned long
> > page_index,
> > +			    struct sgx_backing *backing)
> > +{
> > +	return sgx_encl_get_backing(encl, page_index, backing);
> > +}
> 
> IMHO, sgx_encl_backing() should be open-coded here.

I can understand your hesitation, but I agree with Dave here that
wrapping the function makes the code more clear. I would prefer to keep
this the way it is.