Re: [PATCH v3] x86/sgx: Free backing memory after faulting the enclave page

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Jan 31, 2022 at 03:30:52PM -0800, Dave Hansen wrote:
> On 1/26/22 23:01, Jarkko Sakkinen wrote:
> > On Sat, Jan 08, 2022 at 04:05:10PM +0200, Jarkko Sakkinen wrote:
> >> +static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index)
> >> +{
> >> +	return PFN_DOWN(encl->size) + 1 + (index / sizeof(struct sgx_pcmd));
> >> +}
> > Found it.
> > 
> > This should be 
> > 
> > static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl *encl, pgoff_t index)
> > {
> > 	return PFN_DOWN(encl->size) + 1 + (index * PAGE_SIZE) / sizeof(struct sgx_pcmd);
> > }
> 
> I've looked at this for about 10 minutes and been simultaneously
> confused as to whether it is right or wrong.  That makes it
> automatically wrong. :)
> 
> First, this isn't calculating a "PCMD number".  It's calculating backing
> offset.  The "PCMD number" calculation is only a part of it.  I think
> that makes the naming rather sloppy.
> 
> Second, I think the typing is sloppy.  page_index for example:
> 
> > static int __sgx_encl_eldu(struct sgx_encl_page *encl_page,
> >                            struct sgx_epc_page *epc_page,
> >                            struct sgx_epc_page *secs_page)
> > {
> ...
> >         pgoff_t page_index;
> 
> It's storing a page number:
> 
>                 page_index = PFN_DOWN(encl->size);
> 
> not a real offset-into-a-file.  That makes it even more confusing when
> 'page_index' crosses a function boundary, gets renamed to 'index' and
> then its units get confused.
> 
> /*
>  * Given a page number within an enclave (@epc_page_nr), calculate the
>  * offset in bytes into the backing file where that page's PCMD is
>  * located.
>  */
> -static inline pgoff_t sgx_encl_get_backing_pcmd_nr(struct sgx_encl
> *encl, pgoff_t index)
> +static inline pgoff_t sgx_page_nr_to_pcmd_nr(struct sgx_encl *encl,
> unsigned long epc_page_nr)
> {
> 	pgoff_t last_epc_offset = PFN_DOWN(encl->size);
> 	pgoff_t pcmd_offset;
> 
> 	// The SECS page is stashed in a slot after the
> 	// last normal EPC page.  Leave space for it:
> 	last_epc_offset++;
> 
> 	pcmd_offset = epc_page_nr / sizeof(struct sgx_pcmd);
> 
> 	return last_epc_offset + pcmd_offset;
> }
> 
> Looking at that, I still think your *original* version is correct.
> 
> Am I just all twisted around from looking at this code too much?  Could
> you please take another look and send a new version of the patch?

Yeah, I will. It took me two weeks to make full remote attestation
implementation for SGX in Rust but at least I can confirm that all
the kernel bits work on that (and this will also help me to finish
the man page because it is pretty nasty to document it if you haven't
gone to the rabbit hole yourself).

I'll prioritize now to get SGX2 patches tested with Enarx implementation
but this 2nd thing in my kernel queue.

/Jarkko