Linux-Sgx Archive on lore.kernel.org
 help / color / Atom feed
WARNING: multiple messages refer to this Message-ID
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: <x86@kernel.org>, <platform-driver-x86@vger.kernel.org>,
	<linux-sgx@vger.kernel.org>, <dave.hansen@intel.com>,
	<nhorman@redhat.com>, <npmccallum@redhat.com>,
	<serge.ayoun@intel.com>, <shay.katz-zamir@intel.com>,
	<haitao.huang@intel.com>, <andriy.shevchenko@linux.intel.com>,
	<tglx@linutronix.de>, <kai.svahn@intel.com>,
	Suresh Siddha <suresh.b.siddha@intel.com>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v16 18/22] platform/x86: Intel SGX driver
Date: Wed, 7 Nov 2018 10:00:57 -0800
Message-ID: <20181107180057.GB24807@linux.intel.com> (raw)
In-Reply-To: <20181107163757.GB11509@linux.intel.com>

On Wed, Nov 07, 2018 at 06:37:57PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 08:40:00AM -0800, Sean Christopherson wrote:
> > On Tue, 2018-11-06 at 15:45 +0200, Jarkko Sakkinen wrote:
> > > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > > can be used by applications to set aside private regions of code and
> > > data. The code outside the enclave is disallowed to access the memory
> > > inside the enclave by the CPU access control.
> > > 
> > > SGX driver provides a ioctl API for loading and initializing enclaves.
> > > Address range for enclaves is reserved with mmap() and they are
> > > destroyed with munmap(). Enclave construction, measurement and
> > > initialization is done with the provided the ioctl API.
> > 
> > ...
> > 
> > > +struct sgx_encl {
> > > +	unsigned int flags;
> > > +	uint64_t attributes;
> > > +	uint64_t xfrm;
> > > +	unsigned int page_cnt;
> > > +	unsigned int secs_child_cnt;
> > > +	struct mutex lock;
> > > +	struct mm_struct *mm;
> > > +	struct file *backing;
> > 
> > Is there any particular reason why the kernel manages the backing for
> > the enclave and the PCMDs?  Could we have userspace provide the backing
> > either through the ECREATE ioctl() or maybe a completely new ioctl(),
> > e.g. to give userspace the option to back the enclave with a NVDIMM
> > instead of RAM?  A separate ioctl() with control flags might give us
> > some flexibility in the future, e.g. maybe there are use cases where
> > userspace would prefer to kill enclaves rather than swap EPC.
> 
> Not really except that no one has complained. The very first swapping
> code that I implemented used a VMA as backing storage. I could take
> pieces of that code to replace shmem specifics. The difference was that
> the driver did vm_mmap(). Now that you suggested the above I wonder how
> it did not came to mind back then to provide the VMA as parameter.
> 
> A single buffer that can hold both PCMD entries and swapped pages in its
> address space would probably be the  best way to do it. I would add that
> as a field to struct sgx_enclave_create. If we want the kill-behavior,
> you could signal that with a NULL value.

What do we gain by a single buffer vs. separate buffers?  The ioctl()
would be slightly smaller but it seems like the actual code would be
more complex.

The enclave build process also utilizes the backing as temp storage
to avoid having to alloc kernel memory when queueing pages to be added
by the worker thread (which reminds me that I wanted to document why a
worker thread is used).  Keeping this behavior would effectively make
providing backing mandatory.

Are there any potential complications with ENCLS consuming userspace
pointers?  We'd have to wrap them with user_access_{begin,end}() and
probably tweak the fixup, but I assume having the fixup handler means
we're generally ok?

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: x86@kernel.org, platform-driver-x86@vger.kernel.org,
	linux-sgx@vger.kernel.org, dave.hansen@intel.com,
	nhorman@redhat.com, npmccallum@redhat.com, serge.ayoun@intel.com,
	shay.katz-zamir@intel.com, haitao.huang@intel.com,
	andriy.shevchenko@linux.intel.com, tglx@linutronix.de,
	kai.svahn@intel.com, Suresh Siddha <suresh.b.siddha@intel.com>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v16 18/22] platform/x86: Intel SGX driver
Date: Wed, 7 Nov 2018 10:00:57 -0800
Message-ID: <20181107180057.GB24807@linux.intel.com> (raw)
Message-ID: <20181107180057.y84KyL8GbX4aRtDHzzCVQrv3_g8QqDdkXfeUiE9-GGA@z> (raw)
In-Reply-To: <20181107163757.GB11509@linux.intel.com>

On Wed, Nov 07, 2018 at 06:37:57PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 08:40:00AM -0800, Sean Christopherson wrote:
> > On Tue, 2018-11-06 at 15:45 +0200, Jarkko Sakkinen wrote:
> > > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > > can be used by applications to set aside private regions of code and
> > > data. The code outside the enclave is disallowed to access the memory
> > > inside the enclave by the CPU access control.
> > > 
> > > SGX driver provides a ioctl API for loading and initializing enclaves.
> > > Address range for enclaves is reserved with mmap() and they are
> > > destroyed with munmap(). Enclave construction, measurement and
> > > initialization is done with the provided the ioctl API.
> > 
> > ...
> > 
> > > +struct sgx_encl {
> > > +	unsigned int flags;
> > > +	uint64_t attributes;
> > > +	uint64_t xfrm;
> > > +	unsigned int page_cnt;
> > > +	unsigned int secs_child_cnt;
> > > +	struct mutex lock;
> > > +	struct mm_struct *mm;
> > > +	struct file *backing;
> > 
> > Is there any particular reason why the kernel manages the backing for
> > the enclave and the PCMDs?  Could we have userspace provide the backing
> > either through the ECREATE ioctl() or maybe a completely new ioctl(),
> > e.g. to give userspace the option to back the enclave with a NVDIMM
> > instead of RAM?  A separate ioctl() with control flags might give us
> > some flexibility in the future, e.g. maybe there are use cases where
> > userspace would prefer to kill enclaves rather than swap EPC.
> 
> Not really except that no one has complained. The very first swapping
> code that I implemented used a VMA as backing storage. I could take
> pieces of that code to replace shmem specifics. The difference was that
> the driver did vm_mmap(). Now that you suggested the above I wonder how
> it did not came to mind back then to provide the VMA as parameter.
> 
> A single buffer that can hold both PCMD entries and swapped pages in its
> address space would probably be the  best way to do it. I would add that
> as a field to struct sgx_enclave_create. If we want the kill-behavior,
> you could signal that with a NULL value.

What do we gain by a single buffer vs. separate buffers?  The ioctl()
would be slightly smaller but it seems like the actual code would be
more complex.

The enclave build process also utilizes the backing as temp storage
to avoid having to alloc kernel memory when queueing pages to be added
by the worker thread (which reminds me that I wanted to document why a
worker thread is used).  Keeping this behavior would effectively make
providing backing mandatory.

Are there any potential complications with ENCLS consuming userspace
pointers?  We'd have to wrap them with user_access_{begin,end}() and
probably tweak the fixup, but I assume having the fixup handler means
we're generally ok?

  reply index

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-06 13:45 [PATCH v16 00/22] Intel SGX1 support Jarkko Sakkinen
2018-11-06 13:45 ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 01/22] x86/sgx: Update MAINTAINERS Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 02/22] x86/cpufeatures: Add Intel-defined SGX feature bit Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 03/22] x86/cpufeatures: Add SGX sub-features (as Linux-defined bits) Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 04/22] x86/msr: Add IA32_FEATURE_CONTROL.SGX_ENABLE definition Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 05/22] x86/cpufeatures: Add Intel-defined SGX_LC feature bit Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 06/22] x86/cpu/intel: Detect SGX support and update caps appropriately Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:58   ` Sean Christopherson
2018-11-06 13:58     ` Sean Christopherson
2018-11-07 15:58     ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 07/22] x86/mm: x86/sgx: Add new 'PF_SGX' page fault error code bit Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 08/22] x86/mm: x86/sgx: Signal SIGSEGV for userspace #PFs w/ PF_SGX Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 09/22] x86/sgx: Define SGX1 and SGX2 ENCLS leafs Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 10/22] x86/sgx: Add ENCLS architectural error codes Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 11/22] x86/sgx: Add SGX1 and SGX2 architectural data structures Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 12/22] x86/sgx: Add definitions for SGX's CPUID leaf and variable sub-leafs Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 13/22] x86/msr: Add SGX Launch Control MSR definitions Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 14/22] x86/sgx: Add wrappers for ENCLS leaf functions Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 15/22] x86/sgx: Enumerate and track EPC sections Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 16/22] x86/sgx: Add functions to allocate and free EPC pages Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 17/22] x86/sgx: Add sgx_einit() for initializing enclaves Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 18/22] platform/x86: Intel SGX driver Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 16:40   ` Sean Christopherson
2018-11-06 16:57     ` Dave Hansen
2018-11-07 16:37     ` Jarkko Sakkinen
2018-11-07 18:00       ` Sean Christopherson [this message]
2018-11-07 18:00         ` Sean Christopherson
2018-11-08 14:46         ` Jarkko Sakkinen
2018-11-15 20:00           ` Jarkko Sakkinen
2018-11-15 20:04             ` Jarkko Sakkinen
2018-11-15 20:16               ` Jarkko Sakkinen
2018-11-21 11:46                 ` Jarkko Sakkinen
2018-11-07 10:29   ` David Laight
2018-11-06 13:45 ` [PATCH v16 19/22] platform/x86: sgx: Add swapping functionality to the " Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:45 ` [PATCH v16 20/22] x86/sgx: Add a simple swapper for the EPC memory manager Jarkko Sakkinen
2018-11-06 13:45   ` Jarkko Sakkinen
2018-11-06 13:46 ` [PATCH v16 21/22] platform/x86: ptrace() support for the SGX driver Jarkko Sakkinen
2018-11-06 13:46   ` Jarkko Sakkinen
2018-11-06 13:46 ` [PATCH v16 22/22] x86/sgx: SGX documentation Jarkko Sakkinen
2018-11-06 13:46   ` Jarkko Sakkinen
2018-11-27 20:13   ` Pavel Machek

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181107180057.GB24807@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=andy@infradead.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dvhart@infradead.org \
    --cc=haitao.huang@intel.com \
    --cc=hpa@zytor.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=kai.svahn@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sgx@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=nhorman@redhat.com \
    --cc=npmccallum@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=serge.ayoun@intel.com \
    --cc=shay.katz-zamir@intel.com \
    --cc=suresh.b.siddha@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Sgx Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sgx/0 linux-sgx/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sgx linux-sgx/ https://lore.kernel.org/linux-sgx \
		linux-sgx@vger.kernel.org linux-sgx@archiver.kernel.org
	public-inbox-index linux-sgx


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sgx


AGPL code for this site: git clone https://public-inbox.org/ public-inbox