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

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Andy Lutomirski <luto@kernel.org>
Cc: Jordan Hand <jorhand@linux.microsoft.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	linux-sgx@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Dave Hansen <dave.hansen@intel.com>,
	Neil Horman <nhorman@redhat.com>,
	npmccallum@redhat.com, "Huang, Haitao" <haitao.huang@intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Svahn, Kai" <kai.svahn@intel.com>,
	Borislav Petkov <bp@alien8.de>,
	Josh Triplett <josh@joshtriplett.org>,
	"Huang, Kai" <kai.huang@intel.com>,
	David Rientjes <rientjes@google.com>,
	"Xing, Cedric" <cedric.xing@intel.com>,
	puiterwijk@redhat.com,
	LSM List <linux-security-module@vger.kernel.org>,
	Suresh Siddha <suresh.b.siddha@intel.com>,
	Haitao Huang <haitao.huang@linux.intel.com>
Subject: Re: [PATCH v26 10/22] x86/sgx: Linux Enclave Driver
Date: Thu, 20 Feb 2020 11:15:10 -0800	[thread overview]
Message-ID: <20200220191510.GF3972@linux.intel.com> (raw)
In-Reply-To: <CALCETrVXnSR8fBQtqv=3zFxJCFhcHE-6XNAy3suPW+uPgFvfvg@mail.gmail.com>

On Thu, Feb 20, 2020 at 10:51:37AM -0800, Andy Lutomirski wrote:
> On Thu, Feb 20, 2020 at 10:13 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > More than likely, the READ_IMPLIES_EXECUTE (RIE) crud rears its head
> > because part of the enclave loader is written in assembly.  Unless
> > explicitly told otherwise, the linker assumes that any program with
> > assembly code may need an executable stack, which leads to the RIE
> > personality being set for the process.  Here's a fantastic write up for
> > more details: https://www.airs.com/blog/archives/518
> >
> > There are essentially two paths we can take:
> >
> >  1) Exempt EPC pages from RIE during mmap()/mprotect(), i.e. don't add
> >     PROT_EXEC for enclaves.
> 
> Seems reasonable.
> 
> Honestly, it probably makes sense to try to exempt almost everything
> from RIE.  I'd be a bit surprised if RIE is actually useful for
> anything other than plain anonymous pages and private file mappings.

Hmm, last I looked at this I was focused on adding a generic protections
manipulator, e.g. vm_ops->mprotect_adjust() and f_op->???, and I thought
those options were too ugly to pursue.

But if we want to start killing RIE specifically, adding a boolean flag
to and f_op wouldn't be _that_ heinous, e.g.

static int do_mprotect_pkey(...)
{
	...

		/* Does the application expect PROT_READ to imply PROT_EXEC */
		if (rier && (vma->vm_flags & VM_MAYEXEC) &&
		    (!vma->vm_file || !vma->vm_file->f_op->no_read_implies_exec))
			prot |= PROT_EXEC;
}

unsigned long do_mmap(...)
{
	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
		if (!file || (!path_noexec(&file->f_path) &&
			      !file->f_op->no_read_implies_exec))
			prot |= PROT_EXEC;
}