Date:   Thu, 13 Feb 2020 10:07:37 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jethro Beekman <jethro@...tanix.com>
Cc:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-sgx@...r.kernel.org, akpm@...ux-foundation.org,
        dave.hansen@...el.com, nhorman@...hat.com, npmccallum@...hat.com,
        haitao.huang@...el.com, andriy.shevchenko@...ux.intel.com,
        tglx@...utronix.de, kai.svahn@...el.com, bp@...en8.de,
        josh@...htriplett.org, luto@...nel.org, kai.huang@...el.com,
        rientjes@...gle.com, cedric.xing@...el.com, puiterwijk@...hat.com,
        linux-security-module@...r.kernel.org,
        Suresh Siddha <suresh.b.siddha@...el.com>,
        Haitao Huang <haitao.huang@...ux.intel.com>
Subject: Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver

On Thu, Feb 13, 2020 at 02:59:52PM +0100, Jethro Beekman wrote:
> On 2020-02-09 22:25, Jarkko Sakkinen wrote:
> > +/**
> > + * struct sgx_enclave_add_pages - parameter structure for the
> > + *                                %SGX_IOC_ENCLAVE_ADD_PAGE ioctl
> > + * @src:	start address for the page data
> > + * @offset:	starting page offset
> > + * @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;
> > +	__u64	offset;
> > +	__u64	length;
> > +	__u64	secinfo;
> > +	__u64	flags;
> > +	__u64	count;
> > +};
> 
> Compared to the last time I looked at the patch set, this API removes the
> ability to measure individual pages chunks. That is not acceptable.

Why is it not acceptable?  E.g. what specific use case do you have that
_requires_ on measuring partial 4k pages of an enclave?

> On 2019-10-11 16:37, Sean Christopherson wrote:
> > Hiding the 256-byte granualarity from userspace is a good idea as it's not
> > intrinsically tied to the SGX architecture and exists only because of
> > latency requirements.
> 
> What do you mean by "it's not intrinsically tied to the SGX architecture"?
> This is a fundamental part of the SGX instruction set. This is the
> instruction definition from the SDM: "EEXTEND—Extend Uninitialized Enclave
> Measurement by 256 Bytes".

SGX fundamentally works at a 4k granularity.  EEXTEND is special cased
because extending the measurement is a slow operation, i.e. EEXTEND on more
than 256 byte chunks, *with the current implementation*, would exceeded
latency requirements, e.g. block interrupts for too long and hose the
kernel.

A future implementation of SGX could change the latency of extending the
measurement, e.g. a different algorithm that is slower/faster, and so could
introduce EEXTEND2 which would work at a different granularity than EEXTEND.

EEXTEND could have avoided the latency problems via other methods, e.g. by
being interruptible a la EINIT and/or by being restartable.  But that ship
has sailed, so to avoid future complication in the kernel's ABI we're
proposing/advocating supporting only measuring at a 4k granularity.
 
> The exact sequence of EADD/EEXTEND calls is part of the enclave hash. The OS
> mustn't arbitrarily restrict how an enclave may be loaded. If the enclave

It's not arbitrary, there are good reasons for wanting to work with 4k
granularity.  Regardless, there are many examples of the kernel arbitrarily
restricting what can be done relative to what is physically possible in
hardware.

> loader were to follows OS-specific restrictions, that would result in
> effectively different enclaves. Because of these interoperability concerns,
> 256-byte granularity *must* be exposed through the UAPI.

Interoperability with what?  Other OSes? 

> Besides only partially measuring a page, there are some other fringe cases
> that are technically possible, although I haven't seen any toolchains that do
> that. These include not interleaving EADD and EEXTEND, not using logical
> ordering for the EEXTENDs, and call EEXTEND multiple times on the same chunk.
> Maximum interoperability would require supporting any EADD/EEXTEND sequence.

Same interoperability question as above.

> Maybe we should just add an EEXTEND@...set ioctl? This would give
> fine-grained control when needed (one could set flags=0 in the add pages
> ioctl and interleave with EEXTEND as needed). If you're ok adding an EEXTEND
> ioctl I don't think this issue needs to block landing the driver in its
> current form, in which case:

We've also discussed an EEXTEND ioctl(), but ultimately couldn't come up
with a use case that _required_ partial page measurement.

> Tested-by: Jethro Beekman <jethro@...tanix.com>
> 
> Sorry for being super late with this, I know you asked me for feedback about
> this specific point in October. However, I did previously mention several
> times that being able to measure individual 256-byte chunks is necessary.
> 
> -- Jethro Beekman | Fortanix

Powered by blists - more mailing lists