Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Jethro Beekman <jethro@fortanix.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	linux-sgx@vger.kernel.org, akpm@linux-foundation.org,
	dave.hansen@intel.com, nhorman@redhat.com, npmccallum@redhat.com,
	haitao.huang@intel.com, andriy.shevchenko@linux.intel.com,
	tglx@linutronix.de, kai.svahn@intel.com, bp@alien8.de,
	josh@joshtriplett.org, luto@kernel.org, kai.huang@intel.com,
	rientjes@google.com, cedric.xing@intel.com,
	puiterwijk@redhat.com, linux-security-module@vger.kernel.org,
	Haitao Huang <haitao.huang@linux.intel.com>
Subject: Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
Date: Tue, 18 Feb 2020 14:12:08 -0800	[thread overview]
Message-ID: <20200218221208.GJ28156@linux.intel.com> (raw)
In-Reply-To: <033BCE0D-FA8C-40FB-849A-E401A5C6F6A3@amacapital.net>

On Sat, Feb 15, 2020 at 08:56:54AM -0800, Andy Lutomirski wrote:
> 
> > On Feb 14, 2020, at 9:52 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > On Fri, Feb 14, 2020 at 09:40:00AM -0800, Andy Lutomirski wrote:
> >> 
> >> 
> >>>> On Feb 14, 2020, at 9:11 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >>> 
> >>> On Fri, Feb 14, 2020 at 10:24:10AM +0100, Jethro Beekman wrote:
> >>>>> On 2020-02-13 19:07, Sean Christopherson wrote:
> >>>>> 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?
> >>>> 
> >>>> The use case is someone gives me an enclave and I want to load it. If I don't
> >>>> load it exactly as the enclave author specified, the enclave hash will be
> >>>> different, and it won't work.
> >>> 
> >>> And if our ABI says "thou shall measure in 4k chunks", then it's an invalid
> >>> enclave if its author generated MRENCLAVE using a different granularity.
> >> 
> >> ISTM, unless there’s a particularly compelling reason, if an enclave is
> >> valid, we should be able to load it.
> > 
> > That means we have to have a separate ioctl() for EEXTEND, otherwise we
> > can't handle EADD[0]->EADD[1]->EADD[2]->EEXTEND[0]->EEXTEND[1]->EEXTEND[2].
> > 
> > I think we'd still want to keep the MEASURE flag for SGX_IOC_ENCLAVE_ADD_PAGE
> > so that we can optimize EADD[0]->EEXTEND[0]->EADD[1]->EEXTEND[1].
> 
> Seems reasonable to me. I suppose such as ioctl could also be added later if
> there’s a need.

Assuming you're referring to the separate EEXTEND ioctl()...

Everyone:

Is a dedicated EEXTEND ioctl() needed now, i.e. is there an existing or
in-flight use case that would break by having only the MEASURE flag in
ADD_PAGE?

If the answer is 'no' from all parties, my preference would be to hold off
on adding it until there is an actual end user.  To be clear, I'm not
against adding such an ioctl(), I just don't want to add code where the
only user is a kernel selftest.