Re: [PATCH v11 09/13] x86, sgx: basic routines for enclave page cache

[Neil Horman]

On Tue, Jun 19, 2018 at 05:57:53PM +0300, Jarkko Sakkinen wrote:
> On Fri, Jun 08, 2018 at 11:24:12AM -0700, Dave Hansen wrote:
> > On 06/08/2018 10:09 AM, Jarkko Sakkinen wrote:
> > > SGX has a set of data structures to maintain information about the enclaves
> > > and their security properties. BIOS reserves a fixed size region of
> > > physical memory for these structures by setting Processor Reserved Memory
> > > Range Registers (PRMRR). This memory area is called Enclave Page Cache
> > > (EPC).
> > > 
> > > This commit implements the basic routines to allocate and free pages from
> > > different EPC banks. There is also a swapper thread ksgxswapd for EPC pages
> > > that gets woken up by sgx_alloc_page() when we run below the low watermark.
> > > The swapper thread continues swapping pages up until it reaches the high
> > > watermark.
> > 
> > Yay!  A new memory manager in arch-specific code.
> > 
> > > Each subsystem that uses SGX must provide a set of callbacks for EPC
> > > pages that are used to reclaim, block and write an EPC page. Kernel
> > > takes the responsibility of maintaining LRU cache for them.
> > 
> > What does a "subsystem that uses SGX" mean?  Do we have one of those
> > already?
> 
> Driver and KVM.
> 
> > > +struct sgx_secs {
> > > +	uint64_t size;
> > > +	uint64_t base;
> > > +	uint32_t ssaframesize;
> > > +	uint32_t miscselect;
> > > +	uint8_t reserved1[SGX_SECS_RESERVED1_SIZE];
> > > +	uint64_t attributes;
> > > +	uint64_t xfrm;
> > > +	uint32_t mrenclave[8];
> > > +	uint8_t reserved2[SGX_SECS_RESERVED2_SIZE];
> > > +	uint32_t mrsigner[8];
> > > +	uint8_t	reserved3[SGX_SECS_RESERVED3_SIZE];
> > > +	uint16_t isvvprodid;
> > > +	uint16_t isvsvn;
> > > +	uint8_t reserved4[SGX_SECS_RESERVED4_SIZE];
> > > +};
> > 
> > This is a hardware structure, right?  Doesn't it need to be packed?
> 
> Everything is aligned properly in this struct.
> 
I don't think you can guarantee that.  I understand that the reserved size is
likely computed to turn those u8's into 32/64 byte regions, but the uint16t
isvvprodid and isvsvn might get padded to 32 or 64 bytes in software dependent
on the processor you build for.

And even so, its suceptible to being
misaligned if a new version of the hardware adds or removes elements.  Adding a
packed attribute seems like a safe approach (or at least a no-op in the current
state)

Neil