Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Mon, Jun 03, 2019 at 02:23:36PM -0700, Sean Christopherson wrote: > Please read through the RFC, I think it address a lot of your questions. > Hopefully that will help us avoid some thrash. I promise to read it through with detail albeit I just said that as a patch set it is broken :-) Internals still need documentation tho... /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 02:36:01PM -0700, Sean Christopherson wrote: > Assuming MRENCLAVE generated by Graphene or any other hosting scheme are > stable[1], then avoiding EXEC means the user can effectively > whitelist what enclaves are runnable by Graphene, even if the kernel > doesn't implement security_enclave_create/init(). > > I agree that it probably isn't all that important, it's more of a "why > not" argument, i.e. what is gained by not using sigstruct as a proxy? > > [1] What in the world is being attested if MRENCLAVE isn't stable? If I've understood correctly, Graphene uses a single loader enclave that loads the executable in. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 11:01:10AM -0700, Sean Christopherson wrote: > - Requires enclave builder to mark enclave pages executable in the > non-enclave VMAs, which may unnecessarily require EXECMOD on the > source file, or even worse, EXECMEM, and potentially increases the > attack surface since the file must be executable. Enclave builder marks *non-enclave pages*? Not following. > W^X handling: > - mmap() to /dev/sgx/enclave only allowed with PROT_NONE, i.e. force > userspace through mprotect() to simplify the kernel implementation. > - Add vm_ops mprotect() ops hook (I'll refer to SGX's implementation > as SGX.mprotect()) > - Take explicit ALLOW_WRITE at ADD_REGION, a.k.a. EADD > - ADD_REGION also used to describe EAUG region (tentatively for SGX2). > - Track "can be written at some point in time (past or future)" as > ALLOW_WRITE (to avoid confusiong with MAY_WRITE). A priori knowledge > of writability avoids having to track/coordinate PROT_WRITE across > VMAs and MMs. Still not sure why you want to use vm_ops instead of file_operations. The approach I've been proposing earlier in this email thread before these new proposals can be summarized from hook perspective as: - Allow mmap() only before ECREATE and require it to be size of the ELRANGE (ECREATE ioctl would check this). This would be with PROT_NONE. - Disallow mprotect() before EINIT. Requires a new callback to file_operations like mmap() has. - After EINIT check for each mprotect() that it matches the permissions of underlying enclave pages. Disallow mmap() after EINIT. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > What is the "source file" i.e. the target of the check? Enclave file, > > sigstruct file, or /dev/sgx/enclave? > > Enclave file -- that is, the file backing the vma from which the data > is loaded. Wonder why KVM gets away without having this given that enclaves are lot alike VMs. > It's provided by userspace based on whether it thinks the data in > question is enclave code. source->vm_file is the file from which the > code is being loaded. I'm assuming that the user code will only set > excute_intent ==true if it actually wants to execute the code, so, if > there's a denial, it will be fatal. The normal case will be that the > request will be granted on the basis of EXECUTE. AFAIK user spaces tells that already with the SECINFO flags. I don't get why we need a duplicate parameter. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 07:31:14AM -0700, Andy Lutomirski wrote: > - To create an X mapping of an enclave page that came from EADD, you > need EXECUTE on the source file. Optionally, we could also permit > this if you have EXECMOD. Source file? EADD ioctl takes memory buffer in right now. > And I have two design proposals. One is static and one is dynamic. > To implement either one, we will probably need a new .may_mprotect vm > operation, and that operation can call an LSM hook. Or we can give > LSMs a way to detect that a given vm_area_struct is an enclave. As I > see it, this is an implementation detail that is certainly solveable. Why VM operation and not file operation? > EADD takes an execute_intent flag. It calls a new hook: > > int security_enclave_load(struct vm_area_struct *source, bool > execute_intent); > > This hook will fail if execute_intent==true and the caller has neither > EXECUTE, EXECMOD, nor EXECMEM. > > EAUG sets execute_intent = false. > > EINIT takes a sigstruct pointer. SGX can (when initially upstreamed > or later on once there's demand) call a new hook: > > security_enclave_init(struct sigstruct *sigstruct, struct > vm_area_struct *source); What is the source VMA in these callbacks? Why is @execute_intent needed anyway as a ioctl arugment and not deduced from SECINFO? /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 11:04:24AM -0400, Stephen Smalley wrote: > Does this occur for both setting initial permissions and runtime permissions > or just runtime? Both userspace- and driver-initiated mmap/mprotect > operations or just userspace-initiated ones? Does the driver use interfaces > that call the mmap/mprotect hooks or lower level functions? The driver never initiates mmap() or mprotect(). /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Mon, Jun 03, 2019 at 11:54:05PM +0300, Jarkko Sakkinen wrote: > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > What is the "source file" i.e. the target of the check? Enclave file, > > > sigstruct file, or /dev/sgx/enclave? > > > > Enclave file -- that is, the file backing the vma from which the data > > is loaded. > > Wonder why KVM gets away without having this given that enclaves are > lot alike VMs. >From a memory management perspective, VMs are not at all like enclaves. An enclave is an extension of its host, i.e. runs in the same address. This isn't strictly necessary, e.g. an enclave could run in a sandbox process, but even then the enclave will be running with the kernel's standard page tables. A VM is a essentially an opaque blob of data that gets loaded into memory. KVM builds a completely different set of page tables for the VM, the VM has it's own file system (or perhaps doesn't have a file system at all), etc... Ignoring Spectre and L1TF, the VM is contained to its own world. There are a lot of ways for a userspace VMM to expose things beyond raw memory, but doing so requires the appropriate permissions. And practically speaking, all traditional VMs will effectively need RWX memory, i.e. Qemu (or any other userspace VMM) would be required to have EXECMEM permissions, which would be a net negative for security. > > It's provided by userspace based on whether it thinks the data in > > question is enclave code. source->vm_file is the file from which the > > code is being loaded. I'm assuming that the user code will only set > > excute_intent ==true if it actually wants to execute the code, so, if > > there's a denial, it will be fatal. The normal case will be that the > > request will be granted on the basis of EXECUTE. > > AFAIK user spaces tells that already with the SECINFO flags. I don't > get why we need a duplicate parameter. Please read through the RFC, I think it address a lot of your questions. Hopefully that will help us avoid some thrash.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Mon, Jun 3, 2019 at 1:54 PM Jarkko Sakkinen wrote: > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > What is the "source file" i.e. the target of the check? Enclave file, > > > sigstruct file, or /dev/sgx/enclave? > > > > Enclave file -- that is, the file backing the vma from which the data > > is loaded. > > Wonder why KVM gets away without having this given that enclaves are > lot alike VMs. > I would argue it's because access to /dev/kvm means you can execute whatever you code you want in a VM. I don't see how this is avoidable. On the other hand, it would be nice for SGX to not imply this same sort of "execute anything" right, especially since, unlike KVM, SGX is not a sandbox.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 02:36:01PM -0700, Sean Christopherson wrote: Good morning, I hope everyone had a pleasant weekend. > Assuming MRENCLAVE generated by Graphene or any other hosting scheme > are stable[1], then avoiding EXEC means the user can > effectively whitelist what enclaves are runnable by Graphene, even > if the kernel doesn't implement security_enclave_create/init(). > > I agree that it probably isn't all that important, it's more of a > "why not" argument, i.e. what is gained by not using sigstruct as a > proxy? > > [1] What in the world is being attested if MRENCLAVE isn't stable? The cryptographic identity of the entity that signed the enclave and generated the SIGSTRUCT. At the risk of being the monotone in the choir, any relevant SGX security controls require verifying the identity of whoever signed the identity characteristics (SIGSTRUCT) of the image that initiates the execution of an SGX TEE. Other then verifying the initial execution image, the MRENCLAVE value isn't all that relevant. This issue is further evidenced by the fact that sealing data to an enclave uses the MRSIGNER variant of ENCLU[EGETKEY] key derivation. The current work on LSM controls seems to focus on the identity of the entity that is requesting the image to be loaded rather then who actually signed, and presumably authored, the code. As I have previously noted, with SGX2/EDMM, a platform owner may not even have any visibility into the code that an SGX TEE may ultimately load and execute. Any security relevant LSM control in this space has to focus on providing the platform owner the ability to take action based on the contents of the SIGSTRUCT of the initiating image. In addition to the identity of who is requesting the image to be loaded. Have a good week. Dr. Greg As always, Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC. 4206 N. 19th Ave. Specializing in information infra-structure Fargo, ND 58102development. PH: 701-281-1686 FAX: 701-281-3949 EMAIL: g...@enjellic.com -- "Experience is something you don't get until just after you need it." -- Olivier
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 02:48:43PM -0700, Xing, Cedric wrote:
> So I think the same rationale applies to enclaves. Your original idea of
> MAXPERM is the policy set forth by system admin and shall *never* change at
> runtime. If an enclave is dynamically linked and needs to bring in code pages
> at runtime, the admin needs to enable it by setting, say ENCLAVE__EXECMOD, in
> the sigstruct file. Then all EAUG'ed pages will receive RWX as MAXPERM. The
> process would then mprotect() selective pages to be RX but which exact set of
> pages doesn't concern LSM usually.
Because passing RWX means the enclave "requires" EXECMOD even if it never
actually does a RW->RX transition. It's not broken per se, but at the
very least it's decidedly odd.
Dynamically detecting the EXECMOD case is not difficult and has the
advantage of simplifying userspace loaders, e.g. all EAUG pages are tagged
ALLOW_WRITE and the kernel takes care of the rest.
I *think* auditing/learning is also messed up with a MAXPERMS approach, as
mprotect() would fail (due to MAXPERMS clearing MAY_{READ,WRITE,EXEC})
before it calls security_file_mprotect(). Hooking mprotect() is the
obvious workaround, but then it's looking a lot like the new proposals.
In other words, the new proposals are rooted in the MAXPERMS concept, e.g.
MAXPERM is effectively "I want EXECMOD", which gets distilled down to
ALLOW_WRITE (or ALLOW_EXEC in Andy's proposal).
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 2:16 PM Sean Christopherson wrote: > > On Thu, May 30, 2019 at 12:20:45PM -0700, Andy Lutomirski wrote: > > On Thu, May 30, 2019 at 11:01 AM Sean Christopherson > > wrote: > > > > > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > > Enclave file -- that is, the file backing the vma from which the data > > > > is loaded. > > > > > > It wasn't explicitly called out in Andy's proposal(s), but the idea is > > > that the SGX driver would effectively inherit permissions from the source > > > VMA (EADD needs a source for the initial value of the encave page). > > > > I actually meant for it to *not* work like this. I don't want the > > source VMA to have to be VM_EXEC. I think the LSM should just check > > permissions on ->vm_file. > > But if ->vm_file is NULL, i.e. the enclave is not backed by a file, > then PROCESS__EXECMEM is required (or more likely, ENCLAVE__EXECMEM). > If ->vm_file is NULL, then I think some privilege is needed. I suppose the policy could have a new lesser permission EXECUNTRUSTED which is like EXECMOD but you can't modify it. I'm not convinced this is particular important.
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Hi Andy, I'm just curious how Sean convinced you that "MAXPERM doesn't work". More specifically, I'm objecting to the statement of "the enclave loader won't know at load time whether a given EAUG-ed page will ever be executed". I'm still new to LSM so my understanding may not be correct. But I think LSM policy defines a boundary that "loosely" restricts what a process can do. By "loosely", I mean it is usually more permissive than a process needs. For example, FILE__EXECMOD basically says there are self-modifying code in a file, but it isn't specific on which pages contain self-modifying code, hence *all* pages are allowed mprotect(RW->RX) even though only a (small) subset actually need it. LSM policies are static too, as FILE__EXECMOD is given by a system admin to be associated with the disk file, instead of being requested/programmed by any processes loading/mapping that file. So I think the same rationale applies to enclaves. Your original idea of MAXPERM is the policy set forth by system admin and shall *never* change at runtime. If an enclave is dynamically linked and needs to bring in code pages at runtime, the admin needs to enable it by setting, say ENCLAVE__EXECMOD, in the sigstruct file. Then all EAUG'ed pages will receive RWX as MAXPERM. The process would then mprotect() selective pages to be RX but which exact set of pages doesn't concern LSM usually. > From: Andy Lutomirski [mailto:l...@kernel.org] > Sent: Thursday, May 30, 2019 12:21 PM > > On Thu, May 30, 2019 at 11:01 AM Sean Christopherson > > wrote: > > > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > On Thu, May 30, 2019 at 8:04 AM Stephen Smalley > > > wrote: > > > > > > > > On 5/30/19 10:31 AM, Andy Lutomirski wrote: > > > > > Hi all- > > > > > > > > > > After an offline discussion with Sean yesterday, here are some > > > > > updates to the user API parts of my proposal. > > > > > > > > > > Unfortunately, Sean convinced me that MAXPERM doesn't work the > > > > > way I described it because, for SGX2, the enclave loader won't > > > > > know at load time whether a given EAUG-ed page will ever be > > > > > executed. So here's an update. > > > > > > > > > > First, here are the requrements as I see them, where EXECUTE, > > > > > EXECMOD, and EXECMEM could be substituted with other rules at > > > > > the LSM's > > > > > discretion: > > > > > > > > > > - You can create a WX or RWX mapping if and only if you have > > > > > EXECMEM. > > > > > > > > > > - To create an X mapping of an enclave page that has ever been > > > > > W, you need EXECMOD. > > > > > > > > EXECMOD to what file? The enclave file from which the page's > > > > content originated, the sigstruct file, or /dev/sgx/enclave? > > > > > > I leave that decision to you :) The user should need permission to > > > do an execmod thing on an enclave, however that wants to be encoded. > > > > But that decision dictates how the SGX API handles sigstruct. If LSMs > > want to associate EXECMOD with sigstruct, then SGX needs to take > > sigstruct early and hold a reference to the file for the lifetime of the > > enclave. > > And if we're going to do that, the whole approach of inheriting > > permissions from source VMAs becomes unnecessary complexity. > > > > > > > > > > > - To create an X mapping of an enclave page that came from > > > > > EADD, you need EXECUTE on the source file. Optionally, we could > > > > > also permit this if you have EXECMOD. > > > > > > > > What is the "source file" i.e. the target of the check? Enclave > > > > file, sigstruct file, or /dev/sgx/enclave? > > > > > > Enclave file -- that is, the file backing the vma from which the data is > > > loaded. > > > > It wasn't explicitly called out in Andy's proposal(s), but the idea is > > that the SGX driver would effectively inherit permissions from the > > source VMA (EADD needs a source for the initial value of the encave page). > > I actually meant for it to *not* work like this. I don't want the source VMA > to have to > be VM_EXEC. I think the LSM should just check permissions on ->vm_file. -Cedric
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 02:23:07PM -0700, Andy Lutomirski wrote: > On Thu, May 30, 2019 at 2:16 PM Sean Christopherson > wrote: > > > > On Thu, May 30, 2019 at 12:20:45PM -0700, Andy Lutomirski wrote: > > > On Thu, May 30, 2019 at 11:01 AM Sean Christopherson > > > wrote: > > > > > > > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > > > Enclave file -- that is, the file backing the vma from which the data > > > > > is loaded. > > > > > > > > It wasn't explicitly called out in Andy's proposal(s), but the idea is > > > > that the SGX driver would effectively inherit permissions from the > > > > source > > > > VMA (EADD needs a source for the initial value of the encave page). > > > > > > I actually meant for it to *not* work like this. I don't want the > > > source VMA to have to be VM_EXEC. I think the LSM should just check > > > permissions on ->vm_file. > > > > But if ->vm_file is NULL, i.e. the enclave is not backed by a file, > > then PROCESS__EXECMEM is required (or more likely, ENCLAVE__EXECMEM). > > > > If ->vm_file is NULL, then I think some privilege is needed. I > suppose the policy could have a new lesser permission EXECUNTRUSTED > which is like EXECMOD but you can't modify it. I'm not convinced this > is particular important. Assuming MRENCLAVE generated by Graphene or any other hosting scheme are stable[1], then avoiding EXEC means the user can effectively whitelist what enclaves are runnable by Graphene, even if the kernel doesn't implement security_enclave_create/init(). I agree that it probably isn't all that important, it's more of a "why not" argument, i.e. what is gained by not using sigstruct as a proxy? [1] What in the world is being attested if MRENCLAVE isn't stable?
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 12:20:45PM -0700, Andy Lutomirski wrote: > On Thu, May 30, 2019 at 11:01 AM Sean Christopherson > wrote: > > > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > > Enclave file -- that is, the file backing the vma from which the data is > > > loaded. > > > > It wasn't explicitly called out in Andy's proposal(s), but the idea is > > that the SGX driver would effectively inherit permissions from the source > > VMA (EADD needs a source for the initial value of the encave page). > > I actually meant for it to *not* work like this. I don't want the > source VMA to have to be VM_EXEC. I think the LSM should just check > permissions on ->vm_file. But if ->vm_file is NULL, i.e. the enclave is not backed by a file, then PROCESS__EXECMEM is required (or more likely, ENCLAVE__EXECMEM). In practice, it's the same net effect of using sigstruct as a proxy, i.e. *something* has to get to the file system to avoid EXECMEM. But putting the entire enclave to the filesystem seems like a heaver lift than dumping the sigstruct. And if sigstruct needs to be in the file system for security_enclave_create/init()...
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 11:01 AM Sean Christopherson wrote: > > On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote: > > On Thu, May 30, 2019 at 8:04 AM Stephen Smalley wrote: > > > > > > On 5/30/19 10:31 AM, Andy Lutomirski wrote: > > > > Hi all- > > > > > > > > After an offline discussion with Sean yesterday, here are some updates > > > > to the user API parts of my proposal. > > > > > > > > Unfortunately, Sean convinced me that MAXPERM doesn't work the way I > > > > described it because, for SGX2, the enclave loader won't know at load > > > > time whether a given EAUG-ed page will ever be executed. So here's an > > > > update. > > > > > > > > First, here are the requrements as I see them, where EXECUTE, EXECMOD, > > > > and EXECMEM could be substituted with other rules at the LSM's > > > > discretion: > > > > > > > > - You can create a WX or RWX mapping if and only if you have EXECMEM. > > > > > > > > - To create an X mapping of an enclave page that has ever been W, you > > > > need EXECMOD. > > > > > > EXECMOD to what file? The enclave file from which the page's content > > > originated, the sigstruct file, or /dev/sgx/enclave? > > > > I leave that decision to you :) The user should need permission to do > > an execmod thing on an enclave, however that wants to be encoded. > > But that decision dictates how the SGX API handles sigstruct. If LSMs > want to associate EXECMOD with sigstruct, then SGX needs to take sigstruct > early and hold a reference to the file for the lifetime of the enclave. > And if we're going to do that, the whole approach of inheriting > permissions from source VMAs becomes unnecessary complexity. > > > > > > > > - To create an X mapping of an enclave page that came from EADD, you > > > > need EXECUTE on the source file. Optionally, we could also permit > > > > this if you have EXECMOD. > > > > > > What is the "source file" i.e. the target of the check? Enclave file, > > > sigstruct file, or /dev/sgx/enclave? > > > > Enclave file -- that is, the file backing the vma from which the data is > > loaded. > > It wasn't explicitly called out in Andy's proposal(s), but the idea is > that the SGX driver would effectively inherit permissions from the source > VMA (EADD needs a source for the initial value of the encave page). I actually meant for it to *not* work like this. I don't want the source VMA to have to be VM_EXEC. I think the LSM should just check permissions on ->vm_file.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote:
> On Thu, May 30, 2019 at 8:04 AM Stephen Smalley wrote:
> >
> > On 5/30/19 10:31 AM, Andy Lutomirski wrote:
> > > Hi all-
> > >
> > > After an offline discussion with Sean yesterday, here are some updates
> > > to the user API parts of my proposal.
> > >
> > > Unfortunately, Sean convinced me that MAXPERM doesn't work the way I
> > > described it because, for SGX2, the enclave loader won't know at load
> > > time whether a given EAUG-ed page will ever be executed. So here's an
> > > update.
> > >
> > > First, here are the requrements as I see them, where EXECUTE, EXECMOD,
> > > and EXECMEM could be substituted with other rules at the LSM's
> > > discretion:
> > >
> > > - You can create a WX or RWX mapping if and only if you have EXECMEM.
> > >
> > > - To create an X mapping of an enclave page that has ever been W, you
> > > need EXECMOD.
> >
> > EXECMOD to what file? The enclave file from which the page's content
> > originated, the sigstruct file, or /dev/sgx/enclave?
>
> I leave that decision to you :) The user should need permission to do
> an execmod thing on an enclave, however that wants to be encoded.
But that decision dictates how the SGX API handles sigstruct. If LSMs
want to associate EXECMOD with sigstruct, then SGX needs to take sigstruct
early and hold a reference to the file for the lifetime of the enclave.
And if we're going to do that, the whole approach of inheriting
permissions from source VMAs becomes unnecessary complexity.
> >
> > > - To create an X mapping of an enclave page that came from EADD, you
> > > need EXECUTE on the source file. Optionally, we could also permit
> > > this if you have EXECMOD.
> >
> > What is the "source file" i.e. the target of the check? Enclave file,
> > sigstruct file, or /dev/sgx/enclave?
>
> Enclave file -- that is, the file backing the vma from which the data is
> loaded.
It wasn't explicitly called out in Andy's proposal(s), but the idea is
that the SGX driver would effectively inherit permissions from the source
VMA (EADD needs a source for the initial value of the encave page).
I have two gripes with that approach:
- Requires enclave builder to mark enclave pages executable in the
non-enclave VMAs, which may unnecessarily require EXECMOD on the
source file, or even worse, EXECMEM, and potentially increases the
attack surface since the file must be executable.
- Is completely unnecessary if the enclave holds a reference to the
sigstruct file, as LSMs can easily apply labels to the sigstruct,
e.g. EXECUTE on the sigstruct instead of EXECUTE on the source file.
After the bajillion mails we've generated, AIUI we've come up with two
concepts that are viable: inheriting permissions from the source VMA
vs. using sigstruct as a proxy for the enclave. Andy's proposals rely on
the inheritance concept. The proposal below is based on the sigstruct
proxy concept.
For those not familiar with SGX details, sigstruct can be used as a proxy
because hardware enforces that the measurement stored in the sigstruct
exactly matches the measurement generated by the enclave build process,
e.g. adding a page as RX instead of R will change the measurement.
Core Concepts:
- FILE_{READ,WRITE,EXEC} on /dev/sgx/enclave effectively gates access to
EPC. All real world enclaves will need all three permissions.
- sigstruct is the proxy for enclave from an LSM perspective, e.g.
SELinux can define ENCLAVE__EXECUTE and ENCLAVE__EXECMOD and apply
them to the sigstruct file.
- Take sigstruct at ECREATE so that ADD_REGION immediately followed by
mprotect() works as expected (because SGX.mprotect() needs sigstruct
to pass to security_enclave_mprotect(), see below).
- SGX driver takes a reference to the backing sigstruct file if it
exists so that the file can be provided to LSMs during mprotect().
- Optional: SGX driver *requires* sigstruct to be backed by file, purely
to enforce userspace infrastructure is in place for LSM support.
W^X handling:
- mmap() to /dev/sgx/enclave only allowed with PROT_NONE, i.e. force
userspace through mprotect() to simplify the kernel implementation.
- Add vm_ops mprotect() ops hook (I'll refer to SGX's implementation
as SGX.mprotect())
- Take explicit ALLOW_WRITE at ADD_REGION, a.k.a. EADD
- ADD_REGION also used to describe EAUG region (tentatively for SGX2).
- Track "can be written at some point in time (past or future)" as
ALLOW_WRITE (to avoid confusiong with MAY_WRITE). A priori knowledge
of writability avoids having to track/coordinate PROT_WRITE across
VMAs and MMs.
- SGX.mprotect() returns -EPERM if PROT_WRITE && !ALLOW_WRITE.
- Add security_enclave_mprotect() LSM hook, called by SGX.mprotect(),
e.g. int security_enclave_mprotect(struct file *sigstruct,
unsigned long prot,
bool
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 29, 2019 at 10:38:06PM -0700, Xing, Cedric wrote: > > From: Christopherson, Sean J > > Sent: Tuesday, May 28, 2019 2:41 PM > > > > On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote: > > > On Tue, May 28, 2019 at 1:24 PM Sean Christopherson > > > wrote: > > > > > > > > Actually, I think we do have everything we need from an LSM perspective. > > > > LSMs just need to understand that sgx_enclave_load() with a NULL vma > > > > implies a transition from RW. For example, SELinux would interpret > > > > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD. > > > > > > You lost me here. What operation triggers this callback? And > > > wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO > > > or just some fresh executable zero bytes? > > > > An explicit ioctl() after EACCEPTCOPY to update the allowed permissions. > > For all intents and purposes, the EAUG'd page must start RW. Maybe a > > better way to phrase > > it is that at some point the page must be writable to have any value > > whatsover. > > EACCEPTCOPY explicitly requires the page to be at least RW. EACCEPT > > technically doesn't > > require RW, but a RO or RX zero page is useless. Userspace could still > > EACCEPT with RO or > > RX, but SGX would assume a minimum of RW for the purposes of the LSM check. > > Why is an explicit ioctl() necessary after EACCEPTCOPY? Or why is mprotect() > not sufficient? Ignore this, I was trying to avoid having to add a vm_ops mprotect(), which Andy pointed out was silly. > > In theory, it's still your MAXPERM model, but with the unnecessary states > > removed and the > > others enforced/handled by the natural SGX transitions instead of explictly > > in ioctls. > > Underneath the hood the SGX driver would still need to track the MAXPERM. > > What are the "unnecessary states" removed? Andy proposed taking full RWX in MAXPERMs, but really we only need "can writes ever happen to this page", as that allows the SGX driver to avoid having to track if a page has been mapped PROT_WRITE by any VMA in any process. > I'm not sure understand the proposal fully. The whole thing looks to me like > the driver is undertaking things that should/would otherwise be done by > mmap()/mprotect() syscalls. It also imposes unnecessary restrictions on user > mode code, such as mmap(PROT_NONE), ACTIVATE_REGION can be called only once, > etc. What'd happen if ACTIVATE_REGION is called with a range spanning > multiple/partial VMAs? What'd happen if an enclave was unmapped than mapped > again? I'd say the proposal is unintuitive at least. > > In theory, if the driver can keep track of MAXPERM for all pages within an > enclave, then it could fail mmap() if the requested prot conflicts with any > page's MAXPERM within that range. Otherwise, MAXPERM could be copied into > VM_MAY* flags then mprotect() will just follow through. Wouldn't that be a > much simpler and more intuitive approach? Ignore all this, again I was trying to avoid hooking mprotect().
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 30, 2019 at 8:04 AM Stephen Smalley wrote: > > On 5/30/19 10:31 AM, Andy Lutomirski wrote: > > Hi all- > > > > After an offline discussion with Sean yesterday, here are some updates > > to the user API parts of my proposal. > > > > Unfortunately, Sean convinced me that MAXPERM doesn't work the way I > > described it because, for SGX2, the enclave loader won't know at load > > time whether a given EAUG-ed page will ever be executed. So here's an > > update. > > > > First, here are the requrements as I see them, where EXECUTE, EXECMOD, > > and EXECMEM could be substituted with other rules at the LSM's > > discretion: > > > > - You can create a WX or RWX mapping if and only if you have EXECMEM. > > > > - To create an X mapping of an enclave page that has ever been W, you > > need EXECMOD. > > EXECMOD to what file? The enclave file from which the page's content > originated, the sigstruct file, or /dev/sgx/enclave? I leave that decision to you :) The user should need permission to do an execmod thing on an enclave, however that wants to be encoded. > > > - To create an X mapping of an enclave page that came from EADD, you > > need EXECUTE on the source file. Optionally, we could also permit > > this if you have EXECMOD. > > What is the "source file" i.e. the target of the check? Enclave file, > sigstruct file, or /dev/sgx/enclave? Enclave file -- that is, the file backing the vma from which the data is loaded. > > > > > And I have two design proposals. One is static and one is dynamic. > > To implement either one, we will probably need a new .may_mprotect vm > > operation, and that operation can call an LSM hook. Or we can give > > LSMs a way to detect that a given vm_area_struct is an enclave. As I > > see it, this is an implementation detail that is certainly solveable. > > > > > > Static proposal: > > > > > > EADD takes an execute_intent flag. It calls a new hook: > > > >int security_enclave_load(struct vm_area_struct *source, bool > > execute_intent); > > > > This hook will fail if execute_intent==true and the caller has neither > > EXECUTE, EXECMOD, nor EXECMEM. > > EADD execute_intent flag is originally provided by whom (userspace or > driver) on what basis? Which file is referenced by source->vm_file? Why > trigger all three checks up front versus only checking if needed? Won't > this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will > need to be dontaudit'd? What if there is a mismatch between > execute_intent and the initial permissions? It's provided by userspace based on whether it thinks the data in question is enclave code. source->vm_file is the file from which the code is being loaded. I'm assuming that the user code will only set excute_intent ==true if it actually wants to execute the code, so, if there's a denial, it will be fatal. The normal case will be that the request will be granted on the basis of EXECUTE. > > > > > EAUG sets execute_intent = false. > > > > EINIT takes a sigstruct pointer. SGX can (when initially upstreamed > > or later on once there's demand) call a new hook: > > > >security_enclave_init(struct sigstruct *sigstruct, struct > > vm_area_struct *source); > > Is struct sigstruct the same as struct sgx_sigstruct in the current > patches (i.e. just the sigstruct data, no file)? What file is > referenced by source->vm_file (the sigstruct or the enclave or > /dev/sgx/enclave)? Is this hook only for enforcing a whitelist on what > enclaves can be loaded? What is the target of the check? sigstruct is just the data. source->vm_file is the file from which the sigstruct came, which could be a .sigstruct file or could be the main executable or a DSO that contains an embedded enclave. The sigstruct data is there so that an LSM (not necessarily SELinux) could check MRENCLAVE or MRSIGNER, and the source is there so that the file's label can be checked. > > > mmap() and mprotect() will require EXECMEM to create WX or RWX > > mappings. They will require EXECMOD to create RX or X mappings of an > > execute_intent==false page. They require no permissions in the other > > cases. > > Does this occur for both setting initial permissions and runtime > permissions or just runtime? Both userspace- and driver-initiated > mmap/mprotect operations or just userspace-initiated ones? Does the > driver use interfaces that call the mmap/mprotect hooks or lower level > functions? These would occur for any mmap(), mprotect(), or ioctl() that changes VMA permissions. Actually arranging for the hooks to be called is an implementation detail that might require a new .mprotect vm_operation. As an alternative, security_enclave_init() or similar could supply may_execmod and may_execmem flags to the driver, and the driver could do these checks on its own when mmap() and mprotect() happen without a new LSM callback. > > > > > > > Dynamic proposal: > > > > > > EADD does not take any special flags. It does something like this > >
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/30/19 10:31 AM, Andy Lutomirski wrote: Hi all- After an offline discussion with Sean yesterday, here are some updates to the user API parts of my proposal. Unfortunately, Sean convinced me that MAXPERM doesn't work the way I described it because, for SGX2, the enclave loader won't know at load time whether a given EAUG-ed page will ever be executed. So here's an update. First, here are the requrements as I see them, where EXECUTE, EXECMOD, and EXECMEM could be substituted with other rules at the LSM's discretion: - You can create a WX or RWX mapping if and only if you have EXECMEM. - To create an X mapping of an enclave page that has ever been W, you need EXECMOD. EXECMOD to what file? The enclave file from which the page's content originated, the sigstruct file, or /dev/sgx/enclave? - To create an X mapping of an enclave page that came from EADD, you need EXECUTE on the source file. Optionally, we could also permit this if you have EXECMOD. What is the "source file" i.e. the target of the check? Enclave file, sigstruct file, or /dev/sgx/enclave? And I have two design proposals. One is static and one is dynamic. To implement either one, we will probably need a new .may_mprotect vm operation, and that operation can call an LSM hook. Or we can give LSMs a way to detect that a given vm_area_struct is an enclave. As I see it, this is an implementation detail that is certainly solveable. Static proposal: EADD takes an execute_intent flag. It calls a new hook: int security_enclave_load(struct vm_area_struct *source, bool execute_intent); This hook will fail if execute_intent==true and the caller has neither EXECUTE, EXECMOD, nor EXECMEM. EADD execute_intent flag is originally provided by whom (userspace or driver) on what basis? Which file is referenced by source->vm_file? Why trigger all three checks up front versus only checking if needed? Won't this trigger a lot of unnecessary EXECMOD and EXECMEM denials that will need to be dontaudit'd? What if there is a mismatch between execute_intent and the initial permissions? EAUG sets execute_intent = false. EINIT takes a sigstruct pointer. SGX can (when initially upstreamed or later on once there's demand) call a new hook: security_enclave_init(struct sigstruct *sigstruct, struct vm_area_struct *source); Is struct sigstruct the same as struct sgx_sigstruct in the current patches (i.e. just the sigstruct data, no file)? What file is referenced by source->vm_file (the sigstruct or the enclave or /dev/sgx/enclave)? Is this hook only for enforcing a whitelist on what enclaves can be loaded? What is the target of the check? mmap() and mprotect() will require EXECMEM to create WX or RWX mappings. They will require EXECMOD to create RX or X mappings of an execute_intent==false page. They require no permissions in the other cases. Does this occur for both setting initial permissions and runtime permissions or just runtime? Both userspace- and driver-initiated mmap/mprotect operations or just userspace-initiated ones? Does the driver use interfaces that call the mmap/mprotect hooks or lower level functions? Dynamic proposal: EADD does not take any special flags. It does something like this internally: bool execute_intent = true; int security_enclave_load(struct vm_area_struct *source, bool *execute_intent); The implementation of security_enclave_load() may set *execute_intent to false. The driver records execute_intent after the LSM is done. On what basis does LSM decide whether to set *execute_intent? If the process lacks all three permissions? What if there is a mismatch with the initial permissions? mmap() and mprotect() will require EXECMEM to create WX or RWX mappings. They will require EXECMOD to create RX or X mappings of an execute_intent==false page. They require no permissions in the other cases. A benefit of the static proposal is that audit failures due to a lack of EXECUTE permission are easy to implement and to understand in the lods. With the dynamic model, we can only really audit the lack of EXECMOD or EXECMEM. A benefit of the dynamic model is that we hide what is arguably a decently large wart from the API.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Hi all- After an offline discussion with Sean yesterday, here are some updates to the user API parts of my proposal. Unfortunately, Sean convinced me that MAXPERM doesn't work the way I described it because, for SGX2, the enclave loader won't know at load time whether a given EAUG-ed page will ever be executed. So here's an update. First, here are the requrements as I see them, where EXECUTE, EXECMOD, and EXECMEM could be substituted with other rules at the LSM's discretion: - You can create a WX or RWX mapping if and only if you have EXECMEM. - To create an X mapping of an enclave page that has ever been W, you need EXECMOD. - To create an X mapping of an enclave page that came from EADD, you need EXECUTE on the source file. Optionally, we could also permit this if you have EXECMOD. And I have two design proposals. One is static and one is dynamic. To implement either one, we will probably need a new .may_mprotect vm operation, and that operation can call an LSM hook. Or we can give LSMs a way to detect that a given vm_area_struct is an enclave. As I see it, this is an implementation detail that is certainly solveable. Static proposal: EADD takes an execute_intent flag. It calls a new hook: int security_enclave_load(struct vm_area_struct *source, bool execute_intent); This hook will fail if execute_intent==true and the caller has neither EXECUTE, EXECMOD, nor EXECMEM. EAUG sets execute_intent = false. EINIT takes a sigstruct pointer. SGX can (when initially upstreamed or later on once there's demand) call a new hook: security_enclave_init(struct sigstruct *sigstruct, struct vm_area_struct *source); mmap() and mprotect() will require EXECMEM to create WX or RWX mappings. They will require EXECMOD to create RX or X mappings of an execute_intent==false page. They require no permissions in the other cases. Dynamic proposal: EADD does not take any special flags. It does something like this internally: bool execute_intent = true; int security_enclave_load(struct vm_area_struct *source, bool *execute_intent); The implementation of security_enclave_load() may set *execute_intent to false. The driver records execute_intent after the LSM is done. mmap() and mprotect() will require EXECMEM to create WX or RWX mappings. They will require EXECMOD to create RX or X mappings of an execute_intent==false page. They require no permissions in the other cases. A benefit of the static proposal is that audit failures due to a lack of EXECUTE permission are easy to implement and to understand in the lods. With the dynamic model, we can only really audit the lack of EXECMOD or EXECMEM. A benefit of the dynamic model is that we hide what is arguably a decently large wart from the API.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/30/19 2:12 AM, Xing, Cedric wrote:
From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx-ow...@vger.kernel.org]
On Behalf
Of Stephen Smalley
Sent: Wednesday, May 29, 2019 7:08 AM
On 5/28/19 4:24 PM, Sean Christopherson wrote:
On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
From: Andy Lutomirski [mailto:l...@kernel.org]
Sent: Saturday, May 25, 2019 5:58 PM
On Sat, May 25, 2019 at 3:40 PM Xing, Cedric wrote:
If we think of EADD as a way of mmap()'ing an enclave file into memory,
would this
security_enclave_load() be the same as
security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
the target is now EPC instead of regular pages?
Hmm, that's clever. Although it seems plausible that an LSM would want to
allow RX or RWX of a given file page but only in the context of an approved
enclave, so I think it should still be its own hook.
What do you mean by "in the context of an approved enclave"? EPC pages are
*inaccessible* to any software until after EINIT. So it would never be a
security concern to EADD a page with wrong permissions as long as the enclave
would be denied eventually by LSM at EINIT.
But I acknowledge the difference between loading a page into regular memory
vs. into EPC. So it's beneficial to have a separate hook, which if not
hooked, would pass through to security_mmap_file() by default?
Mapping the enclave will still go through security_mmap_file(), the extra
security_enclave_load() hook allows the mmap() to use PROT_NONE.
If it's going to be in an arbitrary file, then I think the signature needs to
be more
like:
int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
sigstruct_offset,
const sgx_sigstruct *sigstruct);
So that the LSM still has the opportunity to base its decision on the contents
of the
SIGSTRUCT. Actually, we need that change regardless.
Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
a pointer, because the VMA could be looked up using the pointer and the
offset would then be (pointer - vma->vm_start)?
VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
said, why does the LSM need the VMA? E.g. why not this?
int security_enclave_init(struct file *file, struct sgx_sigstruct
*sigstruct);
Loosely speaking, an enclave (including initial contents of all of its pages and
their
permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision
resistant
property of SHA-2). So only one is needed for a decision, and either one would
lead to
the
same decision. So I don't see anything making any sense here.
Theoretically speaking, if LSM can make a decision at EINIT by means of
security_enclave_load(), then security_enclave_load() is never needed.
In practice, I support keeping both because security_enclave_load() can only
approve
an
enumerable set while security_enclave_load() can approve a non-enumerable set of
enclaves.
Moreover, in order to determine the validity of a MRENCLAVE (as in development
of a
policy
or in creation of a white/black list), system admins will need the audit log
produced
by
security_enclave_load().
I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
up. Also, security_enclave_load() provides no protection against loading a
mishmash of two different enclave files. I see security_enclave_init() as
"verify this SIGSTRUCT against your policy on who may sign enclaves and/or
grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
"implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
check enclave files for some label."
Sorry for the confusion. I was saying the same thing except that the decision
of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
prototype of security_enclave_load(), I think we are on the same page. I made
the above comment to object to the idea of "require that the sigstruct be
supplied before any EADD operations so that the maxperm decisions can depend
on the sigstruct".
Except that having the sigstruct allows using the sigstruct as the proxy
for the enclave. I think the last big disconnect is that Andy and I want
to tie everything to an enclave-specific file, i.e. sigstruct, while you
are proposing labeling /dev/sgx/enclave. If someone wants to cram several
sigstructs into a single file, so be it, but using /dev/sgx/enclave means
users can't do per-enclave permissions, period.
What is your objection to working on the sigstruct?
Passing both would allow tying EXECMOD to /dev/sgx/enclave as
Cedric wanted (without having to play games and pass
/dev/sgx/enclave to security_enclave_load()), but I don't think
there's anything fundamentally broken with using .sigstruct for
EXECMOD. It requires more verbose labeling, but that's not a bad thing.
The benefit of putting it on .sigstruct is that it can be per-enclave.
As I understand it from Fedora packaging, the way this works on
distros is
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: linux-sgx-ow...@vger.kernel.org
> [mailto:linux-sgx-ow...@vger.kernel.org] On Behalf
> Of Stephen Smalley
> Sent: Wednesday, May 29, 2019 7:08 AM
>
> On 5/28/19 4:24 PM, Sean Christopherson wrote:
> > On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
> >>> From: Andy Lutomirski [mailto:l...@kernel.org]
> >>> Sent: Saturday, May 25, 2019 5:58 PM
> >>>
> >>> On Sat, May 25, 2019 at 3:40 PM Xing, Cedric
> >>> wrote:
>
> If we think of EADD as a way of mmap()'ing an enclave file into memory,
> would this
> >>> security_enclave_load() be the same as
> >>> security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
> >>> the target is now EPC instead of regular pages?
> >>>
> >>> Hmm, that's clever. Although it seems plausible that an LSM would want to
> >>> allow RX or RWX of a given file page but only in the context of an
> >>> approved
> >>> enclave, so I think it should still be its own hook.
> >>
> >> What do you mean by "in the context of an approved enclave"? EPC pages are
> >> *inaccessible* to any software until after EINIT. So it would never be a
> >> security concern to EADD a page with wrong permissions as long as the
> >> enclave
> >> would be denied eventually by LSM at EINIT.
> >>
> >> But I acknowledge the difference between loading a page into regular memory
> >> vs. into EPC. So it's beneficial to have a separate hook, which if not
> >> hooked, would pass through to security_mmap_file() by default?
> >
> > Mapping the enclave will still go through security_mmap_file(), the extra
> > security_enclave_load() hook allows the mmap() to use PROT_NONE.
> >
> >>> If it's going to be in an arbitrary file, then I think the signature
> >>> needs to be more
> like:
> >>>
> >>> int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
> sigstruct_offset,
> >>> const sgx_sigstruct *sigstruct);
> >>>
> >>> So that the LSM still has the opportunity to base its decision on the
> >>> contents of the
> >>> SIGSTRUCT. Actually, we need that change regardless.
> >>
> >> Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as
> >> just
> >> a pointer, because the VMA could be looked up using the pointer and the
> >> offset would then be (pointer - vma->vm_start)?
> >
> > VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
> > said, why does the LSM need the VMA? E.g. why not this?
> >
> >int security_enclave_init(struct file *file, struct sgx_sigstruct
> > *sigstruct);
> >
> Loosely speaking, an enclave (including initial contents of all of its
> pages and
> their
> >>> permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the
> >>> collision
> resistant
> >>> property of SHA-2). So only one is needed for a decision, and either one
> >>> would lead to
> the
> >>> same decision. So I don't see anything making any sense here.
>
> Theoretically speaking, if LSM can make a decision at EINIT by means of
> >>> security_enclave_load(), then security_enclave_load() is never needed.
>
> In practice, I support keeping both because security_enclave_load() can
> only approve
> an
> >>> enumerable set while security_enclave_load() can approve a non-enumerable
> >>> set of
> enclaves.
> >>> Moreover, in order to determine the validity of a MRENCLAVE (as in
> >>> development of a
> policy
> >>> or in creation of a white/black list), system admins will need the audit
> >>> log produced
> by
> >>> security_enclave_load().
> >>>
> >>> I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
> >>> up. Also, security_enclave_load() provides no protection against loading
> >>> a
> >>> mishmash of two different enclave files. I see security_enclave_init() as
> >>> "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
> >>> grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
> >>> "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
> >>> check enclave files for some label."
> >>
> >> Sorry for the confusion. I was saying the same thing except that the
> >> decision
> >> of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
> >> prototype of security_enclave_load(), I think we are on the same page. I
> >> made
> >> the above comment to object to the idea of "require that the sigstruct be
> >> supplied before any EADD operations so that the maxperm decisions can
> >> depend
> >> on the sigstruct".
> >
> > Except that having the sigstruct allows using the sigstruct as the proxy
> > for the enclave. I think the last big disconnect is that Andy and I want
> > to tie everything to an enclave-specific file, i.e. sigstruct, while you
> > are proposing labeling /dev/sgx/enclave. If someone wants to cram several
> > sigstructs into a single file, so be it, but using /dev/sgx/enclave means
> > users can't do per-enclave permissions, period.
> >
> > What is your objection to
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: Christopherson, Sean J
> Sent: Tuesday, May 28, 2019 2:41 PM
>
> On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> > On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> > wrote:
> > >
> > > Actually, I think we do have everything we need from an LSM perspective.
> > > LSMs just need to understand that sgx_enclave_load() with a NULL vma
> > > implies a transition from RW. For example, SELinux would interpret
> > > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
> >
> > You lost me here. What operation triggers this callback? And
> > wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> > or just some fresh executable zero bytes?
>
> An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
> For all intents and purposes, the EAUG'd page must start RW. Maybe a better
> way to phrase
> it is that at some point the page must be writable to have any value
> whatsover.
> EACCEPTCOPY explicitly requires the page to be at least RW. EACCEPT
> technically doesn't
> require RW, but a RO or RX zero page is useless. Userspace could still
> EACCEPT with RO or
> RX, but SGX would assume a minimum of RW for the purposes of the LSM check.
Why is an explicit ioctl() necessary after EACCEPTCOPY? Or why is mprotect()
not sufficient?
I tend to agree on Andy's MAXPERM model where MAXPERM never changes once
established.
>
> > > As Cedric mentioned earlier, the host process doesn't necessarily
> > > know which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL,
> > > RX) already has to be invoked at runtime, and when that happens, the
> > > kernel can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
> > >
> > > For simplicity in the kernel and clarity in userspace, it makes
> > > sense to require an explicit ioctl() to add the to-be-EAUG'd range.
> > > That just leaves us wanting an ioctl() to set the post-EACCEPT{COPY}
> > > permissions.
> > >
> > > E.g.:
> > >
> > > ioctl(_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
> > >
> > > mprotect(addr, size, RW);
> > > ...
> > >
> > > EACCEPTCOPY -> EAUG /* page fault handler */
> > >
> > > ioctl(_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
> > >
> > > mprotect(addr, size, RX);
> >
> > In the maxperm model, this mprotect() will fail unless MAXPERM
> > contains RX, which could only happen if MAXPERM=RWX. So, regardless
> > of how it's actually mapped to SELinux policy, MAXPERM=RWX is
> > functionally like EXECMOD and actual RWX PTEs are functionally like
> > EXECMEM.
>
> Yep, same idea, except in the proposed flow ACTIVATE_REGION.
>
> > > ...
> > >
> > > And making ACTIVATE_REGION a single-shot per page eliminates the
> > > need for the MAXPERMS concept (see below).
> > >
> > > > If we keep only one MAXPERM, wouldn't this be the current behavior
> > > > of mmap()/mprotect()?
> > > >
> > > > To be a bit more clear, system admin sets MAXPERM upper bound in
> > > > the form of FILE__{READ|WRITE|EXECUTE|EXECMOD} of
> > > > /dev/sgx/enclave. Then for a process/enclave, if what it requires
> > > > falls below what's allowed on /dev/sgx/enclave, then everything
> > > > will just work. Otherwise, it fails in the form of -EPERM returned
> > > > from mmap()/mprotect(). Please note that MAXPERM here applies to
> > > > "runtime" permissions, while "initial" permissions are taken care
> > > > of by security_enclave_{load|init}. "initial" permissions could be
> > > > more permissive than "runtime" permissions, e.g., RX is still
> > > > required for initial code pages even though system admins could disable
> > > > dynamically
> loaded code pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the
> "initial"
> > > > mapping would still have to be done by the driver (to bypass LSM),
> > > > either via a new ioctl or as part of IOC_EINIT.
> > >
> > > Aha!
> > >
> > > Starting with Cedric's assertion that initial permissions can be
> > > taken directly from SECINFO:
> > >
> > > - Initial permissions for *EADD* pages are explicitly handled via
> > > sgx_enclave_load() with the exact SECINFO permissions.
> > >
> > > - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
> > > requires the target EPC page to be RW, and EACCEPT with RO is useless.
> > >
> > > - Runtime permissions break down as follows:
> > > R - N/A, subset of RW (EAUG)
> > > W - N/A, subset of RW (EAUG) and x86 paging can't do W
> > > X - N/A, subset of RX (x86 paging can't do XO)
> >
> > Sure it can! You just have a hypervisor that maps a PA bit to EPT
> > no-read. Then you can use that PA bit to suppress read. Also, Linux
> > already abuses PKRU to simulate XO, although that won't work for
> > enclaves.
>
> Heh, I intentionally said "x86 paging" to rule out EPT :-) I'm pretty sure
> it's a moot
> point though, I have a hard time believing an LSM will allow RW->X and not
> RW->RX.
>
> > > RW - Handled by EAUG LSM hook
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/28/19 4:24 PM, Sean Christopherson wrote:
On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
From: Andy Lutomirski [mailto:l...@kernel.org]
Sent: Saturday, May 25, 2019 5:58 PM
On Sat, May 25, 2019 at 3:40 PM Xing, Cedric wrote:
If we think of EADD as a way of mmap()'ing an enclave file into memory,
would this
security_enclave_load() be the same as
security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
the target is now EPC instead of regular pages?
Hmm, that's clever. Although it seems plausible that an LSM would want to
allow RX or RWX of a given file page but only in the context of an approved
enclave, so I think it should still be its own hook.
What do you mean by "in the context of an approved enclave"? EPC pages are
*inaccessible* to any software until after EINIT. So it would never be a
security concern to EADD a page with wrong permissions as long as the enclave
would be denied eventually by LSM at EINIT.
But I acknowledge the difference between loading a page into regular memory
vs. into EPC. So it's beneficial to have a separate hook, which if not
hooked, would pass through to security_mmap_file() by default?
Mapping the enclave will still go through security_mmap_file(), the extra
security_enclave_load() hook allows the mmap() to use PROT_NONE.
If it's going to be in an arbitrary file, then I think the signature needs to
be more like:
int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
sigstruct_offset,
const sgx_sigstruct *sigstruct);
So that the LSM still has the opportunity to base its decision on the contents
of the
SIGSTRUCT. Actually, we need that change regardless.
Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
a pointer, because the VMA could be looked up using the pointer and the
offset would then be (pointer - vma->vm_start)?
VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
said, why does the LSM need the VMA? E.g. why not this?
int security_enclave_init(struct file *file, struct sgx_sigstruct
*sigstruct);
Loosely speaking, an enclave (including initial contents of all of its pages
and their
permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the collision
resistant
property of SHA-2). So only one is needed for a decision, and either one would
lead to the
same decision. So I don't see anything making any sense here.
Theoretically speaking, if LSM can make a decision at EINIT by means of
security_enclave_load(), then security_enclave_load() is never needed.
In practice, I support keeping both because security_enclave_load() can only
approve an
enumerable set while security_enclave_load() can approve a non-enumerable set
of enclaves.
Moreover, in order to determine the validity of a MRENCLAVE (as in development
of a policy
or in creation of a white/black list), system admins will need the audit log
produced by
security_enclave_load().
I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
up. Also, security_enclave_load() provides no protection against loading a
mishmash of two different enclave files. I see security_enclave_init() as
"verify this SIGSTRUCT against your policy on who may sign enclaves and/or
grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
"implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
check enclave files for some label."
Sorry for the confusion. I was saying the same thing except that the decision
of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
prototype of security_enclave_load(), I think we are on the same page. I made
the above comment to object to the idea of "require that the sigstruct be
supplied before any EADD operations so that the maxperm decisions can depend
on the sigstruct".
Except that having the sigstruct allows using the sigstruct as the proxy
for the enclave. I think the last big disconnect is that Andy and I want
to tie everything to an enclave-specific file, i.e. sigstruct, while you
are proposing labeling /dev/sgx/enclave. If someone wants to cram several
sigstructs into a single file, so be it, but using /dev/sgx/enclave means
users can't do per-enclave permissions, period.
What is your objection to working on the sigstruct?
Passing both would allow tying EXECMOD to /dev/sgx/enclave as
Cedric wanted (without having to play games and pass
/dev/sgx/enclave to security_enclave_load()), but I don't think
there's anything fundamentally broken with using .sigstruct for
EXECMOD. It requires more verbose labeling, but that's not a bad thing.
The benefit of putting it on .sigstruct is that it can be per-enclave.
As I understand it from Fedora packaging, the way this works on
distros is generally that a package will include some files and
their associated labels, and, if the package needs EXECMOD, then the
files are labeled with EXECMOD and the author of the relevant code might get a
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Tue, May 28, 2019 at 01:48:02PM -0700, Andy Lutomirski wrote:
> On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
> wrote:
> >
> > Actually, I think we do have everything we need from an LSM perspective.
> > LSMs just need to understand that sgx_enclave_load() with a NULL vma
> > implies a transition from RW. For example, SELinux would interpret
> > sgx_enclave_load(NULL, RX) as requiring FILE__EXECMOD.
>
> You lost me here. What operation triggers this callback? And
> wouldn't sgx_enclave_load(NULL, RX) sometimes be a transition from RO
> or just some fresh executable zero bytes?
An explicit ioctl() after EACCEPTCOPY to update the allowed permissions.
For all intents and purposes, the EAUG'd page must start RW. Maybe a
better way to phrase it is that at some point the page must be writable
to have any value whatsover. EACCEPTCOPY explicitly requires the page to
be at least RW. EACCEPT technically doesn't require RW, but a RO or RX
zero page is useless. Userspace could still EACCEPT with RO or RX, but
SGX would assume a minimum of RW for the purposes of the LSM check.
> > As Cedric mentioned earlier, the host process doesn't necessarily know
> > which pages will end up RW vs RX, i.e. sgx_enclave_load(NULL, RX)
> > already has to be invoked at runtime, and when that happens, the kernel
> > can take the opportunity to change the VMAs from MAY_RW to MAY_RX.
> >
> > For simplicity in the kernel and clarity in userspace, it makes sense to
> > require an explicit ioctl() to add the to-be-EAUG'd range. That just
> > leaves us wanting an ioctl() to set the post-EACCEPT{COPY} permissions.
> >
> > E.g.:
> >
> > ioctl(_ADD_REGION, { NULL }) /* NULL == EAUG, MAY_RW */
> >
> > mprotect(addr, size, RW);
> > ...
> >
> > EACCEPTCOPY -> EAUG /* page fault handler */
> >
> > ioctl(_ACTIVATE_REGION, { addr, size, RX}) /* MAY_RX */
> >
> > mprotect(addr, size, RX);
>
> In the maxperm model, this mprotect() will fail unless MAXPERM
> contains RX, which could only happen if MAXPERM=RWX. So, regardless
> of how it's actually mapped to SELinux policy, MAXPERM=RWX is
> functionally like EXECMOD and actual RWX PTEs are functionally like
> EXECMEM.
Yep, same idea, except in the proposed flow ACTIVATE_REGION.
> > ...
> >
> > And making ACTIVATE_REGION a single-shot per page eliminates the need for
> > the MAXPERMS concept (see below).
> >
> > > If we keep only one MAXPERM, wouldn't this be the current behavior of
> > > mmap()/mprotect()?
> > >
> > > To be a bit more clear, system admin sets MAXPERM upper bound in the form
> > > of
> > > FILE__{READ|WRITE|EXECUTE|EXECMOD} of /dev/sgx/enclave. Then for a
> > > process/enclave, if what it requires falls below what's allowed on
> > > /dev/sgx/enclave, then everything will just work. Otherwise, it fails in
> > > the
> > > form of -EPERM returned from mmap()/mprotect(). Please note that MAXPERM
> > > here
> > > applies to "runtime" permissions, while "initial" permissions are taken
> > > care
> > > of by security_enclave_{load|init}. "initial" permissions could be more
> > > permissive than "runtime" permissions, e.g., RX is still required for
> > > initial
> > > code pages even though system admins could disable dynamically loaded code
> > > pages by *not* giving FILE__{EXECUTE|EXECMOD}. Therefore, the "initial"
> > > mapping would still have to be done by the driver (to bypass LSM), either
> > > via
> > > a new ioctl or as part of IOC_EINIT.
> >
> > Aha!
> >
> > Starting with Cedric's assertion that initial permissions can be taken
> > directly from SECINFO:
> >
> > - Initial permissions for *EADD* pages are explicitly handled via
> > sgx_enclave_load() with the exact SECINFO permissions.
> >
> > - Initial permissions for *EAUG* are unconditionally RW. EACCEPTCOPY
> > requires the target EPC page to be RW, and EACCEPT with RO is useless.
> >
> > - Runtime permissions break down as follows:
> > R - N/A, subset of RW (EAUG)
> > W - N/A, subset of RW (EAUG) and x86 paging can't do W
> > X - N/A, subset of RX (x86 paging can't do XO)
>
> Sure it can! You just have a hypervisor that maps a PA bit to EPT
> no-read. Then you can use that PA bit to suppress read. Also, Linux
> already abuses PKRU to simulate XO, although that won't work for
> enclaves.
Heh, I intentionally said "x86 paging" to rule out EPT :-) I'm pretty
sure it's a moot point though, I have a hard time believing an LSM will
allow RW->X and not RW->RX.
> > RW - Handled by EAUG LSM hook (uses RW unconditionally)
> > WX - N/A, subset of RWX (x86 paging can't do WX)
> > RX - Handled by ACTIVATE_REGION
> > RWX - Handled by ACTIVATE_REGION
> >
> > In other words, if we define the SGX -> LSM calls as follows (minus the
> > file pointer and other params for brevity):
> >
> > - _ACTIVATE_REGION(vma, perms) -> sgx_enclave_load(NULL, perms)
> >
> > - _ADD_REGION(vma) -> sgx_enclave_load(vma, SECINFO.perms)
>
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Tue, May 28, 2019 at 1:24 PM Sean Christopherson
wrote:
>
> On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
> > > From: Andy Lutomirski [mailto:l...@kernel.org]
> > > Sent: Saturday, May 25, 2019 5:58 PM
> > >
> > > On Sat, May 25, 2019 at 3:40 PM Xing, Cedric
> > > wrote:
> > > >
> > > > If we think of EADD as a way of mmap()'ing an enclave file into memory,
> > > > would this
> > > security_enclave_load() be the same as
> > > security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
> > > the target is now EPC instead of regular pages?
> > >
> > > Hmm, that's clever. Although it seems plausible that an LSM would want to
> > > allow RX or RWX of a given file page but only in the context of an
> > > approved
> > > enclave, so I think it should still be its own hook.
> >
> > What do you mean by "in the context of an approved enclave"? EPC pages are
> > *inaccessible* to any software until after EINIT. So it would never be a
> > security concern to EADD a page with wrong permissions as long as the
> > enclave
> > would be denied eventually by LSM at EINIT.
> >
> > But I acknowledge the difference between loading a page into regular memory
> > vs. into EPC. So it's beneficial to have a separate hook, which if not
> > hooked, would pass through to security_mmap_file() by default?
>
> Mapping the enclave will still go through security_mmap_file(), the extra
> security_enclave_load() hook allows the mmap() to use PROT_NONE.
>
> > > If it's going to be in an arbitrary file, then I think the signature
> > > needs to be more like:
> > >
> > > int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
> > > sigstruct_offset,
> > > const sgx_sigstruct *sigstruct);
> > >
> > > So that the LSM still has the opportunity to base its decision on the
> > > contents of the
> > > SIGSTRUCT. Actually, we need that change regardless.
> >
> > Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
> > a pointer, because the VMA could be looked up using the pointer and the
> > offset would then be (pointer - vma->vm_start)?
>
> VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
> said, why does the LSM need the VMA? E.g. why not this?
>
> int security_enclave_init(struct file *file, struct sgx_sigstruct
> *sigstruct);
>
> > > > Loosely speaking, an enclave (including initial contents of all of its
> > > > pages and their
> > > permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the
> > > collision resistant
> > > property of SHA-2). So only one is needed for a decision, and either one
> > > would lead to the
> > > same decision. So I don't see anything making any sense here.
> > > >
> > > > Theoretically speaking, if LSM can make a decision at EINIT by means of
> > > security_enclave_load(), then security_enclave_load() is never needed.
> > > >
> > > > In practice, I support keeping both because security_enclave_load() can
> > > > only approve an
> > > enumerable set while security_enclave_load() can approve a non-enumerable
> > > set of enclaves.
> > > Moreover, in order to determine the validity of a MRENCLAVE (as in
> > > development of a policy
> > > or in creation of a white/black list), system admins will need the audit
> > > log produced by
> > > security_enclave_load().
> > >
> > > I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
> > > up. Also, security_enclave_load() provides no protection against loading
> > > a
> > > mishmash of two different enclave files. I see security_enclave_init() as
> > > "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
> > > grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
> > > "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
> > > check enclave files for some label."
> >
> > Sorry for the confusion. I was saying the same thing except that the
> > decision
> > of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
> > prototype of security_enclave_load(), I think we are on the same page. I
> > made
> > the above comment to object to the idea of "require that the sigstruct be
> > supplied before any EADD operations so that the maxperm decisions can depend
> > on the sigstruct".
>
> Except that having the sigstruct allows using the sigstruct as the proxy
> for the enclave. I think the last big disconnect is that Andy and I want
> to tie everything to an enclave-specific file, i.e. sigstruct, while you
> are proposing labeling /dev/sgx/enclave. If someone wants to cram several
> sigstructs into a single file, so be it, but using /dev/sgx/enclave means
> users can't do per-enclave permissions, period.
>
> What is your objection to working on the sigstruct?
>
> > > > > > Passing both would allow tying EXECMOD to /dev/sgx/enclave as
> > > > > > Cedric wanted (without having to play games and pass
> > > > > > /dev/sgx/enclave to security_enclave_load()),
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Sat, May 25, 2019 at 11:09:38PM -0700, Xing, Cedric wrote:
> > From: Andy Lutomirski [mailto:l...@kernel.org]
> > Sent: Saturday, May 25, 2019 5:58 PM
> >
> > On Sat, May 25, 2019 at 3:40 PM Xing, Cedric wrote:
> > >
> > > If we think of EADD as a way of mmap()'ing an enclave file into memory,
> > > would this
> > security_enclave_load() be the same as
> > security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
> > the target is now EPC instead of regular pages?
> >
> > Hmm, that's clever. Although it seems plausible that an LSM would want to
> > allow RX or RWX of a given file page but only in the context of an approved
> > enclave, so I think it should still be its own hook.
>
> What do you mean by "in the context of an approved enclave"? EPC pages are
> *inaccessible* to any software until after EINIT. So it would never be a
> security concern to EADD a page with wrong permissions as long as the enclave
> would be denied eventually by LSM at EINIT.
>
> But I acknowledge the difference between loading a page into regular memory
> vs. into EPC. So it's beneficial to have a separate hook, which if not
> hooked, would pass through to security_mmap_file() by default?
Mapping the enclave will still go through security_mmap_file(), the extra
security_enclave_load() hook allows the mmap() to use PROT_NONE.
> > If it's going to be in an arbitrary file, then I think the signature needs
> > to be more like:
> >
> > int security_enclave_init(struct vm_area_struct *sigstruct_vma, loff_t
> > sigstruct_offset,
> > const sgx_sigstruct *sigstruct);
> >
> > So that the LSM still has the opportunity to base its decision on the
> > contents of the
> > SIGSTRUCT. Actually, we need that change regardless.
>
> Wouldn't the pair of { sigstruct_vma, sigstruct_offset } be the same as just
> a pointer, because the VMA could be looked up using the pointer and the
> offset would then be (pointer - vma->vm_start)?
VMA has vm_file, e.g. the .sigstruct file labeled by LSMs. That being
said, why does the LSM need the VMA? E.g. why not this?
int security_enclave_init(struct file *file, struct sgx_sigstruct *sigstruct);
> > > Loosely speaking, an enclave (including initial contents of all of its
> > > pages and their
> > permissions) and its MRENCLAVE are a 1-to-1 correspondence (given the
> > collision resistant
> > property of SHA-2). So only one is needed for a decision, and either one
> > would lead to the
> > same decision. So I don't see anything making any sense here.
> > >
> > > Theoretically speaking, if LSM can make a decision at EINIT by means of
> > security_enclave_load(), then security_enclave_load() is never needed.
> > >
> > > In practice, I support keeping both because security_enclave_load() can
> > > only approve an
> > enumerable set while security_enclave_load() can approve a non-enumerable
> > set of enclaves.
> > Moreover, in order to determine the validity of a MRENCLAVE (as in
> > development of a policy
> > or in creation of a white/black list), system admins will need the audit
> > log produced by
> > security_enclave_load().
> >
> > I'm confused. Things like MRSIGNER aren't known until the SIGSTRUCT shows
> > up. Also, security_enclave_load() provides no protection against loading a
> > mishmash of two different enclave files. I see security_enclave_init() as
> > "verify this SIGSTRUCT against your policy on who may sign enclaves and/or
> > grant EXECMOD depending on SIGSTRUCT" and security_enclave_load() as
> > "implement your EXECMOD / EXECUTE / WRITE / whatever policy and possibly
> > check enclave files for some label."
>
> Sorry for the confusion. I was saying the same thing except that the decision
> of security_enclave_load() doesn't have to depend on SIGSTRUCT. Given your
> prototype of security_enclave_load(), I think we are on the same page. I made
> the above comment to object to the idea of "require that the sigstruct be
> supplied before any EADD operations so that the maxperm decisions can depend
> on the sigstruct".
Except that having the sigstruct allows using the sigstruct as the proxy
for the enclave. I think the last big disconnect is that Andy and I want
to tie everything to an enclave-specific file, i.e. sigstruct, while you
are proposing labeling /dev/sgx/enclave. If someone wants to cram several
sigstructs into a single file, so be it, but using /dev/sgx/enclave means
users can't do per-enclave permissions, period.
What is your objection to working on the sigstruct?
> > > > > Passing both would allow tying EXECMOD to /dev/sgx/enclave as
> > > > > Cedric wanted (without having to play games and pass
> > > > > /dev/sgx/enclave to security_enclave_load()), but I don't think
> > > > > there's anything fundamentally broken with using .sigstruct for
> > > > > EXECMOD. It requires more verbose labeling, but that's not a bad
> > > > > thing.
> > > >
> > > > The benefit of putting it on .sigstruct is that it can be per-enclave.
>
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> wrote:
> >
> > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > But actually, there's no need to disallow mmap() after ECREATE since the
> > > > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > > > mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> > > > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > > > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> > > > at square one.
> > >
> > > I'm lost with the constraints we want to set.
> >
> > As is today, SELinux policies would require enclave loaders to have
> > FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
> > other LSMs have similar requirements. Requiring all processes to have
> > FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
> > value, e.g. they can't be used to distinguish between an enclave that is
> > being loaded from an unmodified file and an enclave that is being
> > generated on the fly, e.g. Graphene.
> >
> > Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
> > to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
> > special case.
> >
>
> I thought about this some more, and I have a new proposal that helps
> address the ELRANGE alignment issue and the permission issue at the
> cost of some extra verbosity. Maybe you all can poke holes in it :)
> The basic idea is to make everything more explicit from a user's
> perspective. Here's how it works:
>
> Opening /dev/sgx/enclave gives an enclave_fd that, by design, doesn't
> give EXECUTE or WRITE. mmap() on the enclave_fd only works if you
> pass PROT_NONE and gives the correct alignment. The resulting VMA
> cannot be mprotected or mremapped. It can't be mmapped at all until
> after ECREATE because the alignment isn't known before that.
How to deny mprotect()? struct file_operations does not have callback
for that (AFAIK).
> Associated with the enclave are a bunch (up to 7) "enclave segment
> inodes". These are anon_inodes that are created automagically. An
> enclave segment is a group of pages, not necessary contiguous, with an
> upper bound on the memory permissions. Each enclave page belongs to a
> segment. When you do EADD, you tell the driver what segment you're
> adding to. [0] This means that EADD gets an extra argument that is a
> permission mask for the page -- in addition to the initial SECINFO,
> you also pass to EADD something to the effect of "I promise never to
> map this with permissions greater than RX".
>
> Then we just need some way to mmap a region from an enclave segment.
> This could be done by having a way to get an fd for an enclave segment
> or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT. User
> code would use this operation to replace, MAP_FIXED-style, ranges from
> the big PROT_NONE mapping with the relevant pages from the enclave
> segment. The resulting vma would only have VM_MAYWRITE if the segment
> is W, only have VM_MAYEXEC if the segment is X, and only have
> VM_MAYREAD if the segment is R. Depending on implementation details,
> the VMAs might need to restrict mremap() to avoid mapping pages that
> aren't part of the segment in question.
>
> It's plausible that this whole thing works without the magic segment
> inodes under the hood, but figuring that out would need a careful look
> at how all the core mm bits and LSM bits work together.
>
> To get all the LSM stuff to work, SELinux will need some way to
> automatically assign an appropriate label to the segment inodes. I
> assume that such a mechanism already exists and gets used for things
> like sockets, but I haven't actually confirmed this.
>
> [0] There needs to be some vaguely intelligent semantics if you EADD
> the *same* address more than once. A simple solution would be to
> disallow it if the segments don't match.
What if instead simply:
- Require to do PROT_NONE mmap() for the ELRANGE before ECREATE.
- Disallow mprotect() up until EINIT.
- Given that we have a callback for mprotect() check that permissions
match EADD'd permissions.
/Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Mon, May 27, 2019 at 04:34:31PM +0300, Jarkko Sakkinen wrote: > On Thu, May 23, 2019 at 07:17:52AM -0700, Sean Christopherson wrote: > > 1. Do nothing. Userspace would essentially be required to mmap() the > > enclave after EINIT, which is ugly but not breaking since userspace > > could mmap() the enclave with a placeholder VMA prior to building > > the enclave, and then a series of mmap() to establish its "real" > > mapping. > > What it'd break to return error if mmap() is done before EINIT? > > > 2. Propagate the permissions from EADD to the VMAs of the current mm > > if the entire EADD range is mapped and the mapping is PROT_NONE. > > Right now you can do multiple mmap's. If the mmap's must be done after > EINIT, the driver could check that permissions match the permissions in > that range. > > This leaves open how to deal with mprotect() but if the process does not > have FILE__WRITE I guess you cannot do much. > > > 3. Propagate the permissions from EADD to the VMAs of all mm structs > > that have mapped some piece of the enclave, following the matching > > rules from #2. > > For me it looks that allowing mmap's only after EINIT would result the > least confusing implemntation. Obvious problem is of course the requirement of fixed mapping, which is of course nasty. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 07:17:52AM -0700, Sean Christopherson wrote: > 1. Do nothing. Userspace would essentially be required to mmap() the > enclave after EINIT, which is ugly but not breaking since userspace > could mmap() the enclave with a placeholder VMA prior to building > the enclave, and then a series of mmap() to establish its "real" > mapping. What it'd break to return error if mmap() is done before EINIT? > 2. Propagate the permissions from EADD to the VMAs of the current mm > if the entire EADD range is mapped and the mapping is PROT_NONE. Right now you can do multiple mmap's. If the mmap's must be done after EINIT, the driver could check that permissions match the permissions in that range. This leaves open how to deal with mprotect() but if the process does not have FILE__WRITE I guess you cannot do much. > 3. Propagate the permissions from EADD to the VMAs of all mm structs > that have mapped some piece of the enclave, following the matching > rules from #2. For me it looks that allowing mmap's only after EINIT would result the least confusing implemntation. /Jarkko
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: Andy Lutomirski [mailto:l...@kernel.org]
> Sent: Saturday, May 25, 2019 5:58 PM
>
> On Sat, May 25, 2019 at 3:40 PM Xing, Cedric wrote:
> >
> > > From: Andy Lutomirski [mailto:l...@amacapital.net]
> > > Sent: Friday, May 24, 2019 4:42 PM
> > >
> > > > On May 24, 2019, at 3:41 PM, Sean Christopherson
> > > >
> wrote:
> > > >
> > > >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> > > >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> > > >> wrote:
> > > >>>
> > > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric
> > > > wrote:
> > > >
> > > > If "initial permissions" for enclaves are less restrictive
> > > > than shared objects, then it'd become a backdoor for
> > > > circumventing LSM when enclave whitelisting is *not* in place.
> > > > For example, an adversary may load a page, which would
> > > > otherwise never be executable, as an executable
> > > page in EPC.
> > > >
> > > > In the case a RWX page is needed, the calling process has to
> > > > have a RWX page serving as the source for EADD so
> > > > PROCESS__EXECMEM will have been checked. For SGX2, changing an
> > > > EPC page to RWX is subject to FILE__EXECMEM on
> > > > /dev/sgx/enclave, which I see as a security benefit because it
> > > > only affects the enclave but not the whole process hosting
> > > it.
> > >
> > > So the permission would be like FILE__EXECMOD on the source
> > > enclave page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > > MAP_SHARED, PROT_WRITE isn't going to work because that means
> > > you can modify the file.
> > > >>>
> > > >>> Was this in response to Cedric's comment, or to my comment?
> > > >>
> > > >> Yours. I think that requiring source pages to be actually mapped
> > > >> W is not such a great idea.
> > > >
> > > > I wasn't requiring source pages to be mapped W. At least I didn't
> > > > intend to require W. What I was trying to say is that SGX could
> > > > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > > > enclave page with RWX permissions, e.g.:
> > > >
> > > > if ((SECINFO.PERMS & RWX) == RWX) {
> > > > ret = security_mmap_file(NULL, RWX, ???);
> > > > if (ret)
> > > > return ret;
> > > > }
> > > >
> > > > But that's a moot point if we add security_enclave_load() or whatever.
> > > >
> > > >>
> > > >>>
> > > I'm starting to think that looking at the source VMA permission
> > > bits or source PTE permission bits is putting a bit too much
> > > policy into the driver as opposed to the LSM. How about
> > > delegating the whole thing to an LSM hook? The EADD operation
> > > would invoke a new hook, something like:
> > >
> > > int security_enclave_load_bytes(void *source_addr, struct
> > > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > > maxperm);
> > >
> > > Then you don't have to muck with mapping anything PROT_EXEC.
> > > Instead you load from a mapping of a file and the LSM applies
> > > whatever policy it feels appropriate. If the first pass gets
> > > something wrong, the application or library authors can take it
> > > up with the SELinux folks without breaking the whole ABI :)
> > >
> > > (I'm proposing passing in the source_vma because this hook
> > > would be called with mmap_sem held for read to avoid a TOCTOU
> > > race.)
> > >
> > > If we go this route, the only substantial change to the
> > > existing driver that's needed for an initial upstream merge is
> > > the maxperm mechanism and whatever hopefully minimal API
> > > changes are needed to allow users to conveniently set up the
> > > mappings. And we don't need to worry about how to hack around
> > > mprotect() calling into the LSM, because the LSM will actually
> > > be aware of SGX and can just do the right thing.
> > > >>>
> > > >>> This doesn't address restricting which processes can run which
> > > >>> enclaves, it only allows restricting the build flow. Or are you
> > > >>> suggesting this be done in addition to whitelisting sigstructs?
> > > >>
> > > >> In addition.
> > > >>
> > > >> But I named the function badly and gave it a bad signature, which
> > > >> confused you. Let's try again:
> > > >>
> > > >> int security_enclave_load_from_memory(const struct vm_area_struct
> > > >> *source, unsigned int maxperm);
> > > >
> > > > I prefer security_enclave_load(), "from_memory" seems redundant at best.
> > >
> > > Fine with me.
> >
> > If we think of EADD as a way of mmap()'ing an enclave file into memory,
> > would this
> security_enclave_load() be the same as
> security_mmap_file(source_vma->vm_file, maxperm,
> MAP_PRIVATE), except that the target is now EPC instead of regular pages?
>
> Hmm, that's clever. Although it seems
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Sat, May 25, 2019 at 3:40 PM Xing, Cedric wrote:
>
> > From: Andy Lutomirski [mailto:l...@amacapital.net]
> > Sent: Friday, May 24, 2019 4:42 PM
> >
> > > On May 24, 2019, at 3:41 PM, Sean Christopherson
> > > wrote:
> > >
> > >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> > >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> > >> wrote:
> > >>>
> > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric
> > > wrote:
> > >
> > > If "initial permissions" for enclaves are less restrictive than
> > > shared objects, then it'd become a backdoor for circumventing LSM
> > > when enclave whitelisting is *not* in place. For example, an
> > > adversary may load a page, which would otherwise never be executable,
> > > as an executable
> > page in EPC.
> > >
> > > In the case a RWX page is needed, the calling process has to have
> > > a RWX page serving as the source for EADD so PROCESS__EXECMEM will
> > > have been checked. For SGX2, changing an EPC page to RWX is
> > > subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a
> > > security benefit because it only affects the enclave but not the
> > > whole process hosting
> > it.
> >
> > So the permission would be like FILE__EXECMOD on the source enclave
> > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > MAP_SHARED, PROT_WRITE isn't going to work because that means you
> > can modify the file.
> > >>>
> > >>> Was this in response to Cedric's comment, or to my comment?
> > >>
> > >> Yours. I think that requiring source pages to be actually mapped W
> > >> is not such a great idea.
> > >
> > > I wasn't requiring source pages to be mapped W. At least I didn't
> > > intend to require W. What I was trying to say is that SGX could
> > > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > > enclave page with RWX permissions, e.g.:
> > >
> > > if ((SECINFO.PERMS & RWX) == RWX) {
> > > ret = security_mmap_file(NULL, RWX, ???);
> > > if (ret)
> > > return ret;
> > > }
> > >
> > > But that's a moot point if we add security_enclave_load() or whatever.
> > >
> > >>
> > >>>
> > I'm starting to think that looking at the source VMA permission
> > bits or source PTE permission bits is putting a bit too much policy
> > into the driver as opposed to the LSM. How about delegating the
> > whole thing to an LSM hook? The EADD operation would invoke a new
> > hook, something like:
> >
> > int security_enclave_load_bytes(void *source_addr, struct
> > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > maxperm);
> >
> > Then you don't have to muck with mapping anything PROT_EXEC.
> > Instead you load from a mapping of a file and the LSM applies
> > whatever policy it feels appropriate. If the first pass gets
> > something wrong, the application or library authors can take it up
> > with the SELinux folks without breaking the whole ABI :)
> >
> > (I'm proposing passing in the source_vma because this hook would be
> > called with mmap_sem held for read to avoid a TOCTOU race.)
> >
> > If we go this route, the only substantial change to the existing
> > driver that's needed for an initial upstream merge is the maxperm
> > mechanism and whatever hopefully minimal API changes are needed to
> > allow users to conveniently set up the mappings. And we don't need
> > to worry about how to hack around mprotect() calling into the LSM,
> > because the LSM will actually be aware of SGX and can just do the
> > right thing.
> > >>>
> > >>> This doesn't address restricting which processes can run which
> > >>> enclaves, it only allows restricting the build flow. Or are you
> > >>> suggesting this be done in addition to whitelisting sigstructs?
> > >>
> > >> In addition.
> > >>
> > >> But I named the function badly and gave it a bad signature, which
> > >> confused you. Let's try again:
> > >>
> > >> int security_enclave_load_from_memory(const struct vm_area_struct
> > >> *source, unsigned int maxperm);
> > >
> > > I prefer security_enclave_load(), "from_memory" seems redundant at best.
> >
> > Fine with me.
>
> If we think of EADD as a way of mmap()'ing an enclave file into memory, would
> this security_enclave_load() be the same as
> security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that
> the target is now EPC instead of regular pages?
Hmm, that's clever. Although it seems plausible that an LSM would
want to allow RX or RWX of a given file page but only in the context
of an approved enclave, so I think it should still be its own hook.
>
> >
> > >
> > >> Maybe some really fancy future LSM would also want loff_t
> > >> source_offset, but it's probably not terribly useful. This
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Friday, May 24, 2019 4:42 PM
>
> > On May 24, 2019, at 3:41 PM, Sean Christopherson
> > wrote:
> >
> >> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> >> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> >> wrote:
> >>>
> On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric
> > wrote:
> >
> > If "initial permissions" for enclaves are less restrictive than
> > shared objects, then it'd become a backdoor for circumventing LSM
> > when enclave whitelisting is *not* in place. For example, an
> > adversary may load a page, which would otherwise never be executable,
> > as an executable
> page in EPC.
> >
> > In the case a RWX page is needed, the calling process has to have
> > a RWX page serving as the source for EADD so PROCESS__EXECMEM will
> > have been checked. For SGX2, changing an EPC page to RWX is
> > subject to FILE__EXECMEM on /dev/sgx/enclave, which I see as a
> > security benefit because it only affects the enclave but not the whole
> > process hosting
> it.
>
> So the permission would be like FILE__EXECMOD on the source enclave
> page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> MAP_SHARED, PROT_WRITE isn't going to work because that means you
> can modify the file.
> >>>
> >>> Was this in response to Cedric's comment, or to my comment?
> >>
> >> Yours. I think that requiring source pages to be actually mapped W
> >> is not such a great idea.
> >
> > I wasn't requiring source pages to be mapped W. At least I didn't
> > intend to require W. What I was trying to say is that SGX could
> > trigger an EXECMEM check if userspace attempted to EADD or EAUG an
> > enclave page with RWX permissions, e.g.:
> >
> > if ((SECINFO.PERMS & RWX) == RWX) {
> > ret = security_mmap_file(NULL, RWX, ???);
> > if (ret)
> > return ret;
> > }
> >
> > But that's a moot point if we add security_enclave_load() or whatever.
> >
> >>
> >>>
> I'm starting to think that looking at the source VMA permission
> bits or source PTE permission bits is putting a bit too much policy
> into the driver as opposed to the LSM. How about delegating the
> whole thing to an LSM hook? The EADD operation would invoke a new
> hook, something like:
>
> int security_enclave_load_bytes(void *source_addr, struct
> vm_area_struct *source_vma, loff_t source_offset, unsigned int
> maxperm);
>
> Then you don't have to muck with mapping anything PROT_EXEC.
> Instead you load from a mapping of a file and the LSM applies
> whatever policy it feels appropriate. If the first pass gets
> something wrong, the application or library authors can take it up
> with the SELinux folks without breaking the whole ABI :)
>
> (I'm proposing passing in the source_vma because this hook would be
> called with mmap_sem held for read to avoid a TOCTOU race.)
>
> If we go this route, the only substantial change to the existing
> driver that's needed for an initial upstream merge is the maxperm
> mechanism and whatever hopefully minimal API changes are needed to
> allow users to conveniently set up the mappings. And we don't need
> to worry about how to hack around mprotect() calling into the LSM,
> because the LSM will actually be aware of SGX and can just do the
> right thing.
> >>>
> >>> This doesn't address restricting which processes can run which
> >>> enclaves, it only allows restricting the build flow. Or are you
> >>> suggesting this be done in addition to whitelisting sigstructs?
> >>
> >> In addition.
> >>
> >> But I named the function badly and gave it a bad signature, which
> >> confused you. Let's try again:
> >>
> >> int security_enclave_load_from_memory(const struct vm_area_struct
> >> *source, unsigned int maxperm);
> >
> > I prefer security_enclave_load(), "from_memory" seems redundant at best.
>
> Fine with me.
If we think of EADD as a way of mmap()'ing an enclave file into memory, would
this security_enclave_load() be the same as
security_mmap_file(source_vma->vm_file, maxperm, MAP_PRIVATE), except that the
target is now EPC instead of regular pages?
>
> >
> >> Maybe some really fancy future LSM would also want loff_t
> >> source_offset, but it's probably not terribly useful. This same
> >> callback would be used for EAUG.
EAUG always zeroes the EPC page before making it available to an enclave. So I
don't think there's anything needed to done here.
> >>
> >> Following up on your discussion with Cedric about sigstruct, the
> >> other callback would be something like:
> >>
> >> int security_enclave_init(struct file *sigstruct_file);
I'd still insist in using a pointer rather than a file, for reasons that we've
discussed before. For
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 01:03:33PM -0700, Sean Christopherson wrote: Good morning, I hope the weekend is going well for everyone. Skunky holiday weather out here in West-Central Minnesota. > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote: > > If we go this route, the only substantial change to the existing > > driver that's needed for an initial upstream merge is the maxperm > > mechanism and whatever hopefully minimal API changes are needed to > > allow users to conveniently set up the mappings. And we don't need to > > worry about how to hack around mprotect() calling into the LSM, > > because the LSM will actually be aware of SGX and can just do the > > right thing. > This doesn't address restricting which processes can run which > enclaves, it only allows restricting the build flow. Or are you > suggesting this be done in addition to whitelisting sigstructs? > > What's the value prop beyond whitelisting sigstructs? > Realistically, I doubt LSMs/users will want to take the performance > hit of scanning the source bytes every time an enclave is loaded. > > We could add seomthing like security_enclave_mprotect() in lieu of > abusing security_file_mprotect(), but passing the full source bytes > seems a bit much. It would seem that we hold the moniker of responsibility for this conversation, since without our provocation regarding cryptographic verification of enclave source, there would be a driver headed upstream whose only constraint against W^X sourced executable code, running with full confidentiality and integrity protections, would be a character device with o666 permissions. Given that, a couple of reflections to facilitate further conversation, if nothing else for the benefit of Jonathan Corbet and his bystanders... :-) As the conversations to date have indicated, imposing LSM controls on enclave executable code is a bit problematic, in no small part since it is the theological equivalent of driving a square peg into a round hole. SGX, as a technology, was designed around the concept of cryptographic verification of code provenance and origin. The decision to take that off the table, for reasons of political idealogy only, means that mainstream Linux will not be a platform that can achieve the full hardware security capabilities and protections of SGX, nor will mainstream Linux be able to enjoy full protections from the technology itself. We will be dealing with that, from a driver and runtime perspective, but that is a conversation for another day. The issue of SGX2 and Enclave Dynamic Memory Management (EDMM) has come up and to date there doesn't appear to have been a serious conversation regarding whether or not all of the LSM machinations in the world will make any difference when this technology goes mainline. The agenda driving mainlining of the driver is to support Graphene for cloud based solutions and without EDMM, dynamic code loading support is decidedly more problematic. Dynamic enclave code loading isn't problematic from a security perspective when the code is being loaded from the platform itself, since presumably, the encompassing conversation will result in LSM controls being applied to the necessary code paths. However, with the ability to exploit SGX2 instructions, an enclave with adverserial intent could simply setup a mutually attested security context and pull whatever executable code it wants from the INTERNET at large, using an encrypted and integrity protected communications channel. That has at least been our interpretation and experience with the ENCLU[EMODPE] and ENCLU[EACCEPTCOPY] instructions and the out-of-tree driver. Given the use of an encrypted channel, and the fact that these instructions are ring 3 enclave mode only, it would seem that all of the LSM controls in the world won't have visibility or control over code that is being loaded and executed using such a mechanism. We could have arguably missed something that the new driver will do to address this issue. To date the only discussion seems to have been about controls over ENCLS[EAUG], which are arguably a bit blunt for this purpose. In the land of SGX, if one is intellectually honest from an engineering perspective, the only solid security contract one has to work with is the notion of cryptographic identity. Hence our concern and patches that implemented an absolutely minimal footprint ring-0 control infrastructure over the contents of an enclave's SIGSTRUCT. Which is where we have arguably circled back to after 3-4 months and one kernel release cycle. Wrapping an LSM hook around our policy mechanism would seem to achieve, from a security perspective, about the same level of security effect that more major and invasive modifications would achieve, given Cedric's proposal to inherit page permissions from the source, which is what our runtime already does. As always, apologies for excessive verbosity beyond LKML sensibilities. Best wishes for a pleasant remainder of
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 24, 2019, at 3:41 PM, Sean Christopherson
> wrote:
>
>> On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
>> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
>> wrote:
>>>
On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> On Fri, May 24, 2019 at 11:34 AM Xing, Cedric
> wrote:
>
> If "initial permissions" for enclaves are less restrictive than shared
> objects, then it'd become a backdoor for circumventing LSM when enclave
> whitelisting is *not* in place. For example, an adversary may load a page,
> which would otherwise never be executable, as an executable page in EPC.
>
> In the case a RWX page is needed, the calling process has to have a RWX
> page serving as the source for EADD so PROCESS__EXECMEM will have been
> checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM
> on /dev/sgx/enclave, which I see as a security benefit because it only
> affects the enclave but not the whole process hosting it.
So the permission would be like FILE__EXECMOD on the source enclave
page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
MAP_SHARED, PROT_WRITE isn't going to work because that means you can
modify the file.
>>>
>>> Was this in response to Cedric's comment, or to my comment?
>>
>> Yours. I think that requiring source pages to be actually mapped W is
>> not such a great idea.
>
> I wasn't requiring source pages to be mapped W. At least I didn't intend
> to require W. What I was trying to say is that SGX could trigger an
> EXECMEM check if userspace attempted to EADD or EAUG an enclave page with
> RWX permissions, e.g.:
>
> if ((SECINFO.PERMS & RWX) == RWX) {
> ret = security_mmap_file(NULL, RWX, ???);
> if (ret)
> return ret;
> }
>
> But that's a moot point if we add security_enclave_load() or whatever.
>
>>
>>>
I'm starting to think that looking at the source VMA permission bits
or source PTE permission bits is putting a bit too much policy into
the driver as opposed to the LSM. How about delegating the whole
thing to an LSM hook? The EADD operation would invoke a new hook,
something like:
int security_enclave_load_bytes(void *source_addr, struct
vm_area_struct *source_vma, loff_t source_offset, unsigned int
maxperm);
Then you don't have to muck with mapping anything PROT_EXEC. Instead
you load from a mapping of a file and the LSM applies whatever policy
it feels appropriate. If the first pass gets something wrong, the
application or library authors can take it up with the SELinux folks
without breaking the whole ABI :)
(I'm proposing passing in the source_vma because this hook would be
called with mmap_sem held for read to avoid a TOCTOU race.)
If we go this route, the only substantial change to the existing
driver that's needed for an initial upstream merge is the maxperm
mechanism and whatever hopefully minimal API changes are needed to
allow users to conveniently set up the mappings. And we don't need to
worry about how to hack around mprotect() calling into the LSM,
because the LSM will actually be aware of SGX and can just do the
right thing.
>>>
>>> This doesn't address restricting which processes can run which enclaves,
>>> it only allows restricting the build flow. Or are you suggesting this
>>> be done in addition to whitelisting sigstructs?
>>
>> In addition.
>>
>> But I named the function badly and gave it a bad signature, which
>> confused you. Let's try again:
>>
>> int security_enclave_load_from_memory(const struct vm_area_struct
>> *source, unsigned int maxperm);
>
> I prefer security_enclave_load(), "from_memory" seems redundant at best.
Fine with me.
>
>> Maybe some really fancy future LSM would also want loff_t
>> source_offset, but it's probably not terribly useful. This same
>> callback would be used for EAUG.
>>
>> Following up on your discussion with Cedric about sigstruct, the other
>> callback would be something like:
>>
>> int security_enclave_init(struct file *sigstruct_file);
>>
>> The main issue I see is that we also want to control the enclave's
>> ability to have RWX pages or to change a W page to X. We might also
>> want:
>>
>> int security_enclave_load_zeros(unsigned int maxperm);
>
> What's the use case for this? @maxperm will always be at least RW in
> this case, otherwise the page is useless to the enclave, and if the
> enclave can write the page, the fact that it started as zeros is
> irrelevant.
This is how EAUG could ask if RWX is okay. If an enclave is internally doing
dynamic loading, the it will need a heap page with maxperm = RWX. (If it’s
well designed, it will make it RW and then RX, either by changing SECINFO or by
asking the host to mprotect() it, but it still needs the overall RWX mask.).
Also, do real
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 02:27:34PM -0700, Andy Lutomirski wrote:
> On Fri, May 24, 2019 at 1:03 PM Sean Christopherson
> wrote:
> >
> > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote:
> > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric
> > > wrote:
> > > >
> > > > If "initial permissions" for enclaves are less restrictive than shared
> > > > objects, then it'd become a backdoor for circumventing LSM when enclave
> > > > whitelisting is *not* in place. For example, an adversary may load a
> > > > page,
> > > > which would otherwise never be executable, as an executable page in EPC.
> > > >
> > > > In the case a RWX page is needed, the calling process has to have a RWX
> > > > page serving as the source for EADD so PROCESS__EXECMEM will have been
> > > > checked. For SGX2, changing an EPC page to RWX is subject to
> > > > FILE__EXECMEM
> > > > on /dev/sgx/enclave, which I see as a security benefit because it only
> > > > affects the enclave but not the whole process hosting it.
> > >
> > > So the permission would be like FILE__EXECMOD on the source enclave
> > > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
> > > MAP_SHARED, PROT_WRITE isn't going to work because that means you can
> > > modify the file.
> >
> > Was this in response to Cedric's comment, or to my comment?
>
> Yours. I think that requiring source pages to be actually mapped W is
> not such a great idea.
I wasn't requiring source pages to be mapped W. At least I didn't intend
to require W. What I was trying to say is that SGX could trigger an
EXECMEM check if userspace attempted to EADD or EAUG an enclave page with
RWX permissions, e.g.:
if ((SECINFO.PERMS & RWX) == RWX) {
ret = security_mmap_file(NULL, RWX, ???);
if (ret)
return ret;
}
But that's a moot point if we add security_enclave_load() or whatever.
>
> >
> > > I'm starting to think that looking at the source VMA permission bits
> > > or source PTE permission bits is putting a bit too much policy into
> > > the driver as opposed to the LSM. How about delegating the whole
> > > thing to an LSM hook? The EADD operation would invoke a new hook,
> > > something like:
> > >
> > > int security_enclave_load_bytes(void *source_addr, struct
> > > vm_area_struct *source_vma, loff_t source_offset, unsigned int
> > > maxperm);
> > >
> > > Then you don't have to muck with mapping anything PROT_EXEC. Instead
> > > you load from a mapping of a file and the LSM applies whatever policy
> > > it feels appropriate. If the first pass gets something wrong, the
> > > application or library authors can take it up with the SELinux folks
> > > without breaking the whole ABI :)
> > >
> > > (I'm proposing passing in the source_vma because this hook would be
> > > called with mmap_sem held for read to avoid a TOCTOU race.)
> > >
> > > If we go this route, the only substantial change to the existing
> > > driver that's needed for an initial upstream merge is the maxperm
> > > mechanism and whatever hopefully minimal API changes are needed to
> > > allow users to conveniently set up the mappings. And we don't need to
> > > worry about how to hack around mprotect() calling into the LSM,
> > > because the LSM will actually be aware of SGX and can just do the
> > > right thing.
> >
> > This doesn't address restricting which processes can run which enclaves,
> > it only allows restricting the build flow. Or are you suggesting this
> > be done in addition to whitelisting sigstructs?
>
> In addition.
>
> But I named the function badly and gave it a bad signature, which
> confused you. Let's try again:
>
> int security_enclave_load_from_memory(const struct vm_area_struct
> *source, unsigned int maxperm);
I prefer security_enclave_load(), "from_memory" seems redundant at best.
> Maybe some really fancy future LSM would also want loff_t
> source_offset, but it's probably not terribly useful. This same
> callback would be used for EAUG.
>
> Following up on your discussion with Cedric about sigstruct, the other
> callback would be something like:
>
> int security_enclave_init(struct file *sigstruct_file);
>
> The main issue I see is that we also want to control the enclave's
> ability to have RWX pages or to change a W page to X. We might also
> want:
>
> int security_enclave_load_zeros(unsigned int maxperm);
What's the use case for this? @maxperm will always be at least RW in
this case, otherwise the page is useless to the enclave, and if the
enclave can write the page, the fact that it started as zeros is
irrelevant.
> An enclave that's going to modify its own code will need memory with
> maxperm = RWX or WX.
>
> But this is a bit awkward if the LSM's decision depends on the
> sigstruct. We could get fancy and require that the sigstruct be
> supplied before any EADD operations so that the maxperm decisions can
> depend on the sigstruct.
>
> Am I making more sense now?
Yep. Requiring .sigstruct at ECREATE would be trivial. If we
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 1:03 PM Sean Christopherson wrote: > > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote: > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric wrote: > > > > > > If "initial permissions" for enclaves are less restrictive than shared > > > objects, then it'd become a backdoor for circumventing LSM when enclave > > > whitelisting is *not* in place. For example, an adversary may load a page, > > > which would otherwise never be executable, as an executable page in EPC. > > > > > > In the case a RWX page is needed, the calling process has to have a RWX > > > page serving as the source for EADD so PROCESS__EXECMEM will have been > > > checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM > > > on /dev/sgx/enclave, which I see as a security benefit because it only > > > affects the enclave but not the whole process hosting it. > > > > So the permission would be like FILE__EXECMOD on the source enclave > > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE? > > MAP_SHARED, PROT_WRITE isn't going to work because that means you can > > modify the file. > > Was this in response to Cedric's comment, or to my comment? Yours. I think that requiring source pages to be actually mapped W is not such a great idea. > > > I'm starting to think that looking at the source VMA permission bits > > or source PTE permission bits is putting a bit too much policy into > > the driver as opposed to the LSM. How about delegating the whole > > thing to an LSM hook? The EADD operation would invoke a new hook, > > something like: > > > > int security_enclave_load_bytes(void *source_addr, struct > > vm_area_struct *source_vma, loff_t source_offset, unsigned int > > maxperm); > > > > Then you don't have to muck with mapping anything PROT_EXEC. Instead > > you load from a mapping of a file and the LSM applies whatever policy > > it feels appropriate. If the first pass gets something wrong, the > > application or library authors can take it up with the SELinux folks > > without breaking the whole ABI :) > > > > (I'm proposing passing in the source_vma because this hook would be > > called with mmap_sem held for read to avoid a TOCTOU race.) > > > > If we go this route, the only substantial change to the existing > > driver that's needed for an initial upstream merge is the maxperm > > mechanism and whatever hopefully minimal API changes are needed to > > allow users to conveniently set up the mappings. And we don't need to > > worry about how to hack around mprotect() calling into the LSM, > > because the LSM will actually be aware of SGX and can just do the > > right thing. > > This doesn't address restricting which processes can run which enclaves, > it only allows restricting the build flow. Or are you suggesting this > be done in addition to whitelisting sigstructs? In addition. But I named the function badly and gave it a bad signature, which confused you. Let's try again: int security_enclave_load_from_memory(const struct vm_area_struct *source, unsigned int maxperm); Maybe some really fancy future LSM would also want loff_t source_offset, but it's probably not terribly useful. This same callback would be used for EAUG. Following up on your discussion with Cedric about sigstruct, the other callback would be something like: int security_enclave_init(struct file *sigstruct_file); The main issue I see is that we also want to control the enclave's ability to have RWX pages or to change a W page to X. We might also want: int security_enclave_load_zeros(unsigned int maxperm); An enclave that's going to modify its own code will need memory with maxperm = RWX or WX. But this is a bit awkward if the LSM's decision depends on the sigstruct. We could get fancy and require that the sigstruct be supplied before any EADD operations so that the maxperm decisions can depend on the sigstruct. Am I making more sense now?
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 01:42:13PM -0700, Xing, Cedric wrote: > > From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx- > > ow...@vger.kernel.org] On Behalf Of Sean Christopherson > > Sent: Friday, May 24, 2019 12:14 PM > > > > My point is that enclaves have different properties than shared objects. > > > > Normal LSM behavior with regard to executing files is to label files > > with e.g. FILE__EXECUTE. Because an enclave must be built to the exact > > specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct > > is effectively the same as requiring FILE__EXECUTE on the enclave itself. > > > > Addressing your scenario of loading an executable page in EPC, doing so > > would require one of the following: > > > > - Ability to install a .sigstruct with FILE__EXECUTE > > > > - PROCESS__EXECMEM > > > > - FILE__EXECMOD and SGX2 support > > Now I got your point. It sounds a great idea to me! > > But instead of using .sigstruct file, I'd still recommend using file mapping > (i.e. SIGSTRUCT needs to reside in executable memory). But then there'll be a Why? Even in the Graphene case the final .sigstruct can be known ahead of time. Userspace can always use memfd() if it's generating SIGSTRUCT on the fly. > hole - a process having FILE__EXECMOD on any file could use that file as a > SIGSTRUCT. Probably we'll need a new type in SELinux to label > enclave/sigstruct files. > > > I don't see a fundamental difference between having RWX in an enclave > > and RWX in normal memory, either way the process can execute arbitrary > > code, i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on > > certain instructions, but that's easily sidestepped by having a > > trampoline in the host (marked RX) and piping arbitrary code into the > > enclave. Or using EEXIT to do a bit of ROP. > > I'm with you. > > With your proposal only FILE__EXECMOD is needed on /dev/sgx/enclave to launch > Graphene enclaves or the like. It wouldn't even need FILE__EXECMOD, assuming Graphene does all of its libc rewriting before building the enclave, i.e. doesn't EADD RWX pages. > > > > > No changes are required to LSMs, SGX1 has a single LSM touchpoint > > > > > in > > > > its > > > > > mmap(), and I *think* the only required userspace change is to > > > > > mmap() PROT_NONE when allocating the enclave's virtual address > > range. > > > > > > I'm not sure I understand the motivation behind this proposal to > > > decouple initial EPC permissions from source pages. > > > > Pulling permissions from source pages means userspace needs to fully map > > the in normal memory, including marking pages executable. That exposes > > the loader to having executable pages in its address space that it has > > no intention of executing (outside of the enclave). And for Graphene, > > it means having to actively avoid PROCESS__EXECMEM, e.g. by using a > > dummy backing file to build the enclave instead of anon memory. > > Agreed. > > > > > > I don't think it a big deal to fully mmap() enclave files, which have > > > to be parsed by user mode anyway to determine various things including > > > but not limited to the size of heap(s), size and number of > > > TCSs/stacks/TLS areas, and the overall enclave size. So with PHDRs > > > parsed, it's trivial to mmap() each segment with permissions from its > > PHDR. > > > > > > > > As for Graphene, it doesn't need extra permissions to run its > > > > > enclaves, it just needs a way to install .sigstruct, which is a > > > > > generic permissions problem and not SGX specific. > > > > > > > > > > > > > > > For SGX2 maybe: > > > > > > > > > > - No additional requirements to map an EAUG'd page as RW page. > > Not > > > > > aligned with standard MAP_SHARED behavior, but we really don't > > want > > > > > to require FILE__WRITE, and thus allow writes to .sigstruct. > > > > > > > > > > - Require FILE__EXECMOD on the .sigstruct to map previously > > writable > > > > > page as executable (which indirectly includes all EAUG'd > > pages). > > > > > Wiring this up will be a little funky, but we again we don't > > want > > > > > to require FILE__WRITE on .sigstruct. > > > > > > > > > > > I'm lost. Why is EAUG tied to permissions on .sigstruct? > > > > Because for the purposes of LSM checks, .sigstruct is the enclave's > > backing file, and mapping a previously writable enclave page as > > exectuable is roughly equivalent to mapping a CoW'd page as exectuable. > > I think I've got your idea. You are trying to use permissions on .sigstruct > to determine whether EAUG will be available to that specific enclave. Am I > right? Yep. > I'd tie EAUG to the permissions of /dev/sgx/enclave instead. But why? There > are couple of reasons. For one, a SIGSTRUCT identifies the behavior of the > enclave, hence the SGX features needed by that enclave. So if an enclave > requires EAUG, the .sigstruct has to allow EAUG or the enclave wouldn't work. > That means the system admin
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx- > ow...@vger.kernel.org] On Behalf Of Sean Christopherson > Sent: Friday, May 24, 2019 1:04 PM > > On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote: > > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric > wrote: > > > > > > If "initial permissions" for enclaves are less restrictive than > > > shared objects, then it'd become a backdoor for circumventing LSM > > > when enclave whitelisting is *not* in place. For example, an > > > adversary may load a page, which would otherwise never be executable, > as an executable page in EPC. > > > > > > In the case a RWX page is needed, the calling process has to have a > > > RWX page serving as the source for EADD so PROCESS__EXECMEM will > > > have been checked. For SGX2, changing an EPC page to RWX is subject > > > to FILE__EXECMEM on /dev/sgx/enclave, which I see as a security > > > benefit because it only affects the enclave but not the whole > process hosting it. > > > > So the permission would be like FILE__EXECMOD on the source enclave > > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE? > > MAP_SHARED, PROT_WRITE isn't going to work because that means you can > > modify the file. > > Was this in response to Cedric's comment, or to my comment? Creating RWX source page requires PROCESS_EXECMEM. But as I responded to Sean earlier, I think his proposal of "aggregating" all "initial" permission checks into a single SIGSTRUCT check is probably a better approach. > > > I'm starting to think that looking at the source VMA permission bits > > or source PTE permission bits is putting a bit too much policy into > > the driver as opposed to the LSM. How about delegating the whole > > thing to an LSM hook? The EADD operation would invoke a new hook, > > something like: > > > > int security_enclave_load_bytes(void *source_addr, struct > > vm_area_struct *source_vma, loff_t source_offset, unsigned int > > maxperm); This is exactly what I was thinking. But with Sean's proposal this is probably no longer necessary. > > > > Then you don't have to muck with mapping anything PROT_EXEC. Instead > > you load from a mapping of a file and the LSM applies whatever policy > > it feels appropriate. If the first pass gets something wrong, the > > application or library authors can take it up with the SELinux folks > > without breaking the whole ABI :) > > > > (I'm proposing passing in the source_vma because this hook would be > > called with mmap_sem held for read to avoid a TOCTOU race.) > > > > If we go this route, the only substantial change to the existing > > driver that's needed for an initial upstream merge is the maxperm > > mechanism and whatever hopefully minimal API changes are needed to > > allow users to conveniently set up the mappings. And we don't need to > > worry about how to hack around mprotect() calling into the LSM, > > because the LSM will actually be aware of SGX and can just do the > > right thing. > > This doesn't address restricting which processes can run which enclaves, > it only allows restricting the build flow. Or are you suggesting this > be done in addition to whitelisting sigstructs? In the context of SELinux, new types could be defined to be associated with SIGSTRUCT (or more precisely, files containing SIGSTRUCTs). Then the LSM hook (I'd propose security_sgx_initialize_enclave) could enforce whatever... > > What's the value prop beyond whitelisting sigstructs? Realistically, I > doubt LSMs/users will want to take the performance hit of scanning the > source bytes every time an enclave is loaded. > > We could add seomthing like security_enclave_mprotect() in lieu of > abusing security_file_mprotect(), but passing the full source bytes > seems a bit much. I'd just use /dev/sgx/enclave to govern "runtime" permissions any EPC page can mmap()/mprotect() to. Then we won't need any code changes in LSM. -Cedric
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx- > ow...@vger.kernel.org] On Behalf Of Sean Christopherson > Sent: Friday, May 24, 2019 12:14 PM > > My point is that enclaves have different properties than shared objects. > > Normal LSM behavior with regard to executing files is to label files > with e.g. FILE__EXECUTE. Because an enclave must be built to the exact > specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct > is effectively the same as requiring FILE__EXECUTE on the enclave itself. > > Addressing your scenario of loading an executable page in EPC, doing so > would require one of the following: > > - Ability to install a .sigstruct with FILE__EXECUTE > > - PROCESS__EXECMEM > > - FILE__EXECMOD and SGX2 support Now I got your point. It sounds a great idea to me! But instead of using .sigstruct file, I'd still recommend using file mapping (i.e. SIGSTRUCT needs to reside in executable memory). But then there'll be a hole - a process having FILE__EXECMOD on any file could use that file as a SIGSTRUCT. Probably we'll need a new type in SELinux to label enclave/sigstruct files. > > > In the case a RWX page is needed, the calling process has to have a > > RWX page serving as the source for EADD so PROCESS__EXECMEM will have > been checked. > > For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on > > /dev/sgx/enclave, which I see as a security benefit because it only > > affects the enclave but not the whole process hosting it. > > There is no FILE__EXECMEM check, only PROCESS__EXECMEM and FILE__EXECMOD. > I assume you're referring to the latter? Yes. > > I don't see a fundamental difference between having RWX in an enclave > and RWX in normal memory, either way the process can execute arbitrary > code, i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on > certain instructions, but that's easily sidestepped by having a > trampoline in the host (marked RX) and piping arbitrary code into the > enclave. Or using EEXIT to do a bit of ROP. I'm with you. With your proposal only FILE__EXECMOD is needed on /dev/sgx/enclave to launch Graphene enclaves or the like. > > > > > No changes are required to LSMs, SGX1 has a single LSM touchpoint > > > > in > > > its > > > > mmap(), and I *think* the only required userspace change is to > > > > mmap() PROT_NONE when allocating the enclave's virtual address > range. > > > > I'm not sure I understand the motivation behind this proposal to > > decouple initial EPC permissions from source pages. > > Pulling permissions from source pages means userspace needs to fully map > the in normal memory, including marking pages executable. That exposes > the loader to having executable pages in its address space that it has > no intention of executing (outside of the enclave). And for Graphene, > it means having to actively avoid PROCESS__EXECMEM, e.g. by using a > dummy backing file to build the enclave instead of anon memory. Agreed. > > > I don't think it a big deal to fully mmap() enclave files, which have > > to be parsed by user mode anyway to determine various things including > > but not limited to the size of heap(s), size and number of > > TCSs/stacks/TLS areas, and the overall enclave size. So with PHDRs > > parsed, it's trivial to mmap() each segment with permissions from its > PHDR. > > > > > > As for Graphene, it doesn't need extra permissions to run its > > > > enclaves, it just needs a way to install .sigstruct, which is a > > > > generic permissions problem and not SGX specific. > > > > > > > > > > > > For SGX2 maybe: > > > > > > > > - No additional requirements to map an EAUG'd page as RW page. > Not > > > > aligned with standard MAP_SHARED behavior, but we really don't > want > > > > to require FILE__WRITE, and thus allow writes to .sigstruct. > > > > > > > > - Require FILE__EXECMOD on the .sigstruct to map previously > writable > > > > page as executable (which indirectly includes all EAUG'd > pages). > > > > Wiring this up will be a little funky, but we again we don't > want > > > > to require FILE__WRITE on .sigstruct. > > > > > > > > I'm lost. Why is EAUG tied to permissions on .sigstruct? > > Because for the purposes of LSM checks, .sigstruct is the enclave's > backing file, and mapping a previously writable enclave page as > exectuable is roughly equivalent to mapping a CoW'd page as exectuable. I think I've got your idea. You are trying to use permissions on .sigstruct to determine whether EAUG will be available to that specific enclave. Am I right? I'd tie EAUG to the permissions of /dev/sgx/enclave instead. But why? There are couple of reasons. For one, a SIGSTRUCT identifies the behavior of the enclave, hence the SGX features needed by that enclave. So if an enclave requires EAUG, the .sigstruct has to allow EAUG or the enclave wouldn't work. That means the system admin wouldn't have a choice but to match up what's needed by the
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 12:37:44PM -0700, Andy Lutomirski wrote: > On Fri, May 24, 2019 at 11:34 AM Xing, Cedric wrote: > > > > If "initial permissions" for enclaves are less restrictive than shared > > objects, then it'd become a backdoor for circumventing LSM when enclave > > whitelisting is *not* in place. For example, an adversary may load a page, > > which would otherwise never be executable, as an executable page in EPC. > > > > In the case a RWX page is needed, the calling process has to have a RWX > > page serving as the source for EADD so PROCESS__EXECMEM will have been > > checked. For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM > > on /dev/sgx/enclave, which I see as a security benefit because it only > > affects the enclave but not the whole process hosting it. > > So the permission would be like FILE__EXECMOD on the source enclave > page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE? > MAP_SHARED, PROT_WRITE isn't going to work because that means you can > modify the file. Was this in response to Cedric's comment, or to my comment? > I'm starting to think that looking at the source VMA permission bits > or source PTE permission bits is putting a bit too much policy into > the driver as opposed to the LSM. How about delegating the whole > thing to an LSM hook? The EADD operation would invoke a new hook, > something like: > > int security_enclave_load_bytes(void *source_addr, struct > vm_area_struct *source_vma, loff_t source_offset, unsigned int > maxperm); > > Then you don't have to muck with mapping anything PROT_EXEC. Instead > you load from a mapping of a file and the LSM applies whatever policy > it feels appropriate. If the first pass gets something wrong, the > application or library authors can take it up with the SELinux folks > without breaking the whole ABI :) > > (I'm proposing passing in the source_vma because this hook would be > called with mmap_sem held for read to avoid a TOCTOU race.) > > If we go this route, the only substantial change to the existing > driver that's needed for an initial upstream merge is the maxperm > mechanism and whatever hopefully minimal API changes are needed to > allow users to conveniently set up the mappings. And we don't need to > worry about how to hack around mprotect() calling into the LSM, > because the LSM will actually be aware of SGX and can just do the > right thing. This doesn't address restricting which processes can run which enclaves, it only allows restricting the build flow. Or are you suggesting this be done in addition to whitelisting sigstructs? What's the value prop beyond whitelisting sigstructs? Realistically, I doubt LSMs/users will want to take the performance hit of scanning the source bytes every time an enclave is loaded. We could add seomthing like security_enclave_mprotect() in lieu of abusing security_file_mprotect(), but passing the full source bytes seems a bit much.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 11:34 AM Xing, Cedric wrote:
>
> > From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx-
> > ow...@vger.kernel.org] On Behalf Of Sean Christopherson
> > Sent: Friday, May 24, 2019 10:55 AM
> >
> > On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> > > Hmm, I've been thinking more about pulling permissions from the source
> > > page. Conceptually I'm not sure we need to meet the same requirements
> > as
> > > non-enclave DSOs while the enclave is being built, i.e. do we really
> > need
> > > to force userspace to fully map the enclave in normal memory?
> > >
> > > Consider the Graphene scenario where it's building an enclave on the
> > fly.
> > > Pulling permissions from the source VMAs means Graphene has to map the
> > > code pages of the enclave with X. This means Graphene will need
> > EXEDMOD
> > > (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this
> > makes
> > > perfect sense since there is no way to verify the end result of RW->RX.
> > >
> > > But for SGX, assuming enclaves are whitelisted by their sigstruct
> > (checked
> > > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it
> > *is*
> > > possible to verify the resulting RX contents. E.g. for the purposes
> > of
> > > LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> > > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
> > >
> > > Stephen, is my logic sound?
> > >
> > >
> > > If so...
> > >
> > > - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the
> > enclave.
> > >
> > > - Prevent userspace from mapping the enclave with permissions beyond
> > the
> > > original permissions of the enclave. This can be done by
> > populating
> > > VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as
> > Andy's
> > > proposals). E.g. pre-EINIT, mmap() and mprotect() can only
> > succeed
> > > with PROT_NONE.
> > >
> > > - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for
> > simplicity,
> > > or provide an alternate SGX_IOC_MPROTECT if we want to sidestep
> > the
> > > FILE__WRITE requirement.
> >
> > One more thought. EADD (and the equivalent SGX2 flow) could do
> > security_mmap_file() with a NULL file on the SECINFO permissions, which
> > would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
>
> If "initial permissions" for enclaves are less restrictive than shared
> objects, then it'd become a backdoor for circumventing LSM when enclave
> whitelisting is *not* in place. For example, an adversary may load a page,
> which would otherwise never be executable, as an executable page in EPC.
>
> In the case a RWX page is needed, the calling process has to have a RWX page
> serving as the source for EADD so PROCESS__EXECMEM will have been checked.
> For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
> /dev/sgx/enclave, which I see as a security benefit because it only affects
> the enclave but not the whole process hosting it.
So the permission would be like FILE__EXECMOD on the source enclave
page, because it would be mapped MAP_ANONYMOUS, PROT_WRITE?
MAP_SHARED, PROT_WRITE isn't going to work because that means you can
modify the file.
I'm starting to think that looking at the source VMA permission bits
or source PTE permission bits is putting a bit too much policy into
the driver as opposed to the LSM. How about delegating the whole
thing to an LSM hook? The EADD operation would invoke a new hook,
something like:
int security_enclave_load_bytes(void *source_addr, struct
vm_area_struct *source_vma, loff_t source_offset, unsigned int
maxperm);
Then you don't have to muck with mapping anything PROT_EXEC. Instead
you load from a mapping of a file and the LSM applies whatever policy
it feels appropriate. If the first pass gets something wrong, the
application or library authors can take it up with the SELinux folks
without breaking the whole ABI :)
(I'm proposing passing in the source_vma because this hook would be
called with mmap_sem held for read to avoid a TOCTOU race.)
If we go this route, the only substantial change to the existing
driver that's needed for an initial upstream merge is the maxperm
mechanism and whatever hopefully minimal API changes are needed to
allow users to conveniently set up the mappings. And we don't need to
worry about how to hack around mprotect() calling into the LSM,
because the LSM will actually be aware of SGX and can just do the
right thing.
--Andy
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 12:13 PM Sean Christopherson wrote: > > On Fri, May 24, 2019 at 11:34:32AM -0700, Xing, Cedric wrote: > > > From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx- > > > ow...@vger.kernel.org] On Behalf Of Sean Christopherson > > > Sent: Friday, May 24, 2019 10:55 AM > I don't see a fundamental difference between having RWX in an enclave and > RWX in normal memory, either way the process can execute arbitrary code, > i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on certain > instructions, but that's easily sidestepped by having a trampoline in the > host (marked RX) and piping arbitrary code into the enclave. Or using > EEXIT to do a bit of ROP. There's a difference, albeit a somewhat weak one, if sigstructs are whitelisted. FILE__EXECMOD on either /dev/sgx/enclave or on the sigstruct is not an entirely crazy way to express this.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 11:34:32AM -0700, Xing, Cedric wrote:
> > From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx-
> > ow...@vger.kernel.org] On Behalf Of Sean Christopherson
> > Sent: Friday, May 24, 2019 10:55 AM
> >
> > On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> > > Hmm, I've been thinking more about pulling permissions from the source
> > > page. Conceptually I'm not sure we need to meet the same requirements as
> > > non-enclave DSOs while the enclave is being built, i.e. do we really need
> > > to force userspace to fully map the enclave in normal memory?
> > >
> > > Consider the Graphene scenario where it's building an enclave on the fly.
> > > Pulling permissions from the source VMAs means Graphene has to map the
> > > code pages of the enclave with X. This means Graphene will need EXEDMOD
> > > (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this makes
> > > perfect sense since there is no way to verify the end result of RW->RX.
> > >
> > > But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
> > > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
> > > possible to verify the resulting RX contents. E.g. for the purposes of
> > > LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> > > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
> > >
> > > Stephen, is my logic sound?
> > >
> > >
> > > If so...
> > >
> > > - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the enclave.
> > >
> > > - Prevent userspace from mapping the enclave with permissions beyond the
> > > original permissions of the enclave. This can be done by populating
> > > VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as Andy's
> > > proposals). E.g. pre-EINIT, mmap() and mprotect() can only succeed
> > > with PROT_NONE.
> > >
> > > - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for simplicity,
> > > or provide an alternate SGX_IOC_MPROTECT if we want to sidestep the
> > > FILE__WRITE requirement.
> >
> > One more thought. EADD (and the equivalent SGX2 flow) could do
> > security_mmap_file() with a NULL file on the SECINFO permissions, which
> > would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
>
> If "initial permissions" for enclaves are less restrictive than shared
> objects, then it'd become a backdoor for circumventing LSM when enclave
> whitelisting is *not* in place. For example, an adversary may load a page,
> which would otherwise never be executable, as an executable page in EPC.
My point is that enclaves have different properties than shared objects.
Normal LSM behavior with regard to executing files is to label files with
e.g. FILE__EXECUTE. Because an enclave must be built to the exact
specifications of .sigstruct, requring FILE__EXECUTE on the .sigstruct is
effectively the same as requiring FILE__EXECUTE on the enclave itself.
Addressing your scenario of loading an executable page in EPC, doing so
would require one of the following:
- Ability to install a .sigstruct with FILE__EXECUTE
- PROCESS__EXECMEM
- FILE__EXECMOD and SGX2 support
> In the case a RWX page is needed, the calling process has to have a RWX page
> serving as the source for EADD so PROCESS__EXECMEM will have been checked.
> For SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
> /dev/sgx/enclave, which I see as a security benefit because it only affects
> the enclave but not the whole process hosting it.
There is no FILE__EXECMEM check, only PROCESS__EXECMEM and FILE__EXECMOD.
I assume you're referring to the latter?
I don't see a fundamental difference between having RWX in an enclave and
RWX in normal memory, either way the process can execute arbitrary code,
i.e. PROCESS__EXECMEM is appropriate. Yes, an enclave will #UD on certain
instructions, but that's easily sidestepped by having a trampoline in the
host (marked RX) and piping arbitrary code into the enclave. Or using
EEXIT to do a bit of ROP.
> > > No changes are required to LSMs, SGX1 has a single LSM touchpoint in
> > its
> > > mmap(), and I *think* the only required userspace change is to mmap()
> > > PROT_NONE when allocating the enclave's virtual address range.
>
> I'm not sure I understand the motivation behind this proposal to decouple
> initial EPC permissions from source pages.
Pulling permissions from source pages means userspace needs to fully map
the in normal memory, including marking pages executable. That exposes
the loader to having executable pages in its address space that it has no
intention of executing (outside of the enclave). And for Graphene, it
means having to actively avoid PROCESS__EXECMEM, e.g. by using a dummy
backing file to build the enclave instead of anon memory.
> I don't think it a big deal to fully mmap() enclave files, which have to be
> parsed by user mode anyway to determine various things
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> From: linux-sgx-ow...@vger.kernel.org [mailto:linux-sgx-
> ow...@vger.kernel.org] On Behalf Of Sean Christopherson
> Sent: Friday, May 24, 2019 10:55 AM
>
> On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> > Hmm, I've been thinking more about pulling permissions from the source
> > page. Conceptually I'm not sure we need to meet the same requirements
> as
> > non-enclave DSOs while the enclave is being built, i.e. do we really
> need
> > to force userspace to fully map the enclave in normal memory?
> >
> > Consider the Graphene scenario where it's building an enclave on the
> fly.
> > Pulling permissions from the source VMAs means Graphene has to map the
> > code pages of the enclave with X. This means Graphene will need
> EXEDMOD
> > (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this
> makes
> > perfect sense since there is no way to verify the end result of RW->RX.
> >
> > But for SGX, assuming enclaves are whitelisted by their sigstruct
> (checked
> > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it
> *is*
> > possible to verify the resulting RX contents. E.g. for the purposes
> of
> > LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
> >
> > Stephen, is my logic sound?
> >
> >
> > If so...
> >
> > - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the
> enclave.
> >
> > - Prevent userspace from mapping the enclave with permissions beyond
> the
> > original permissions of the enclave. This can be done by
> populating
> > VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as
> Andy's
> > proposals). E.g. pre-EINIT, mmap() and mprotect() can only
> succeed
> > with PROT_NONE.
> >
> > - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for
> simplicity,
> > or provide an alternate SGX_IOC_MPROTECT if we want to sidestep
> the
> > FILE__WRITE requirement.
>
> One more thought. EADD (and the equivalent SGX2 flow) could do
> security_mmap_file() with a NULL file on the SECINFO permissions, which
> would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
If "initial permissions" for enclaves are less restrictive than shared objects,
then it'd become a backdoor for circumventing LSM when enclave whitelisting is
*not* in place. For example, an adversary may load a page, which would
otherwise never be executable, as an executable page in EPC.
In the case a RWX page is needed, the calling process has to have a RWX page
serving as the source for EADD so PROCESS__EXECMEM will have been checked. For
SGX2, changing an EPC page to RWX is subject to FILE__EXECMEM on
/dev/sgx/enclave, which I see as a security benefit because it only affects the
enclave but not the whole process hosting it.
>
> > No changes are required to LSMs, SGX1 has a single LSM touchpoint in
> its
> > mmap(), and I *think* the only required userspace change is to mmap()
> > PROT_NONE when allocating the enclave's virtual address range.
I'm not sure I understand the motivation behind this proposal to decouple
initial EPC permissions from source pages.
I don't think it a big deal to fully mmap() enclave files, which have to be
parsed by user mode anyway to determine various things including but not
limited to the size of heap(s), size and number of TCSs/stacks/TLS areas, and
the overall enclave size. So with PHDRs parsed, it's trivial to mmap() each
segment with permissions from its PHDR.
> >
> > As for Graphene, it doesn't need extra permissions to run its enclaves,
> > it just needs a way to install .sigstruct, which is a generic
> permissions
> > problem and not SGX specific.
> >
> >
> > For SGX2 maybe:
> >
> > - No additional requirements to map an EAUG'd page as RW page. Not
> > aligned with standard MAP_SHARED behavior, but we really don't
> want
> > to require FILE__WRITE, and thus allow writes to .sigstruct.
> >
> > - Require FILE__EXECMOD on the .sigstruct to map previously writable
> > page as executable (which indirectly includes all EAUG'd pages).
> > Wiring this up will be a little funky, but we again we don't want
> > to require FILE__WRITE on .sigstruct.
> >
I'm lost. Why is EAUG tied to permissions on .sigstruct?
-Cedric
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 10:54:34AM -0700, Andy Lutomirski wrote: > > > On May 24, 2019, at 10:42 AM, Sean Christopherson > > wrote: > > > > Hmm, I've been thinking more about pulling permissions from the source > > page. Conceptually I'm not sure we need to meet the same requirements as > > non-enclave DSOs while the enclave is being built, i.e. do we really need > > to force userspace to fully map the enclave in normal memory? > > > > Consider the Graphene scenario where it's building an enclave on the fly. > > Pulling permissions from the source VMAs means Graphene has to map the > > code pages of the enclave with X. This means Graphene will need EXEDMOD > > (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this makes > > perfect sense since there is no way to verify the end result of RW->RX. > > > > But for SGX, assuming enclaves are whitelisted by their sigstruct (checked > > at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is* > > possible to verify the resulting RX contents. E.g. for the purposes of > > LSMs, can't we use the .sigstruct file as a proxy for the enclave and > > require FILE__EXECUTE on the .sigstruct inode to map/run the enclave? > > I think it’s sound for some but not all use cases. I would imagine that a lot > of users won’t restrict sigstruct at all — the “use this as a sigstruct” > permission will be granted to everything and maybe even to memfd. But even > users like that might want to force their enclaves to be hardened such that > writable pages are never executable, in which case Graphene may need an > exception to run. Heh, I belatedly had the same thought. See my follow-up about EXECMEM. > But maybe I’m nuts.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 10:42:43AM -0700, Sean Christopherson wrote:
> Hmm, I've been thinking more about pulling permissions from the source
> page. Conceptually I'm not sure we need to meet the same requirements as
> non-enclave DSOs while the enclave is being built, i.e. do we really need
> to force userspace to fully map the enclave in normal memory?
>
> Consider the Graphene scenario where it's building an enclave on the fly.
> Pulling permissions from the source VMAs means Graphene has to map the
> code pages of the enclave with X. This means Graphene will need EXEDMOD
> (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this makes
> perfect sense since there is no way to verify the end result of RW->RX.
>
> But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
> at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
> possible to verify the resulting RX contents. E.g. for the purposes of
> LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
>
> Stephen, is my logic sound?
>
>
> If so...
>
> - Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the enclave.
>
> - Prevent userspace from mapping the enclave with permissions beyond the
> original permissions of the enclave. This can be done by populating
> VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as Andy's
> proposals). E.g. pre-EINIT, mmap() and mprotect() can only succeed
> with PROT_NONE.
>
> - Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for simplicity,
> or provide an alternate SGX_IOC_MPROTECT if we want to sidestep the
> FILE__WRITE requirement.
One more thought. EADD (and the equivalent SGX2 flow) could do
security_mmap_file() with a NULL file on the SECINFO permissions, which
would trigger PROCESS_EXECMEM if an enclave attempts to map a page RWX.
> No changes are required to LSMs, SGX1 has a single LSM touchpoint in its
> mmap(), and I *think* the only required userspace change is to mmap()
> PROT_NONE when allocating the enclave's virtual address range.
>
> As for Graphene, it doesn't need extra permissions to run its enclaves,
> it just needs a way to install .sigstruct, which is a generic permissions
> problem and not SGX specific.
>
>
> For SGX2 maybe:
>
> - No additional requirements to map an EAUG'd page as RW page. Not
> aligned with standard MAP_SHARED behavior, but we really don't want
> to require FILE__WRITE, and thus allow writes to .sigstruct.
>
> - Require FILE__EXECMOD on the .sigstruct to map previously writable
> page as executable (which indirectly includes all EAUG'd pages).
> Wiring this up will be a little funky, but we again we don't want
> to require FILE__WRITE on .sigstruct.
>
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 24, 2019, at 10:42 AM, Sean Christopherson
> wrote:
>
>> On Fri, May 24, 2019 at 11:41:29AM -0400, Stephen Smalley wrote:
>>> On 5/24/19 3:24 AM, Xing, Cedric wrote:
>>> /**
>>> * Summary:
>>> * - The enclave file resembles a shared object that contains RO/RX/RW
>>> segments
>>> * - FILE__* are assigned to /dev/sgx/enclave, to determine acceptable
>>> permissions to mmap()/mprotect(), valid combinations are
>>> * + FILE__READ - Allow SGX1 enclaves only
>>> * + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data segments
>>> (e.g. heaps, stacks, etc.)
>>> * + FILE__READ|FILE__WRITE|FILE__EXECUTE - Allow SGX2 enclaves to expend
>>> both data and code segments. This is necessary to support dynamically
>>> linked enclaves (e.g. Graphene)
>>> * + FILE__READ|FILE__EXECUTE - Allow RW->RX changes for SGX1 enclaves -
>>> necessary to support dynamically linked enclaves (e.g. Graphene) on SGX1.
>>> EXECMEM is also required for this to work
>>
>> I think EXECMOD would fit better than EXECMEM for this case; the former is
>> applied for RW->RX changes for private file mappings while the latter is
>> applied for WX private file mappings.
>>
>>> * + - Disallow the calling process to launch any enclaves
>>> */
>>>
>>> /* Step 1: mmap() the enclave file according to the segment attributes
>>> (similar to what dlopen() would do for regular shared objects) */
>>> int image_fd = open("/path/to/enclave/file", O_RDONLY);
>>
>> FILE__READ checked to enclave file upon open().
>>
>>> foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
>>>/* below is subject to LSM checks */
>>>loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, >> permission>, image_fd, phdr->p_offset);
>>
>> FILE__READ revalidated and FILE__EXECUTE checked to enclave file upon mmap()
>> for PROT_READ and PROT_EXEC respectively. FILE__WRITE not checked even for
>> PROT_WRITE mappings since it is a private file mapping and writes do not
>> reach the file. EXECMEM checked if any segment permission has both W and X
>> simultaneously. EXECMOD checked on any subsequent mprotect() RW->RX changes
>> (if modified).
>
> Hmm, I've been thinking more about pulling permissions from the source
> page. Conceptually I'm not sure we need to meet the same requirements as
> non-enclave DSOs while the enclave is being built, i.e. do we really need
> to force userspace to fully map the enclave in normal memory?
>
> Consider the Graphene scenario where it's building an enclave on the fly.
> Pulling permissions from the source VMAs means Graphene has to map the
> code pages of the enclave with X. This means Graphene will need EXEDMOD
> (or EXECMEM if Graphene isn't careful). In a non-SGX scenario this makes
> perfect sense since there is no way to verify the end result of RW->RX.
>
> But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
> at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
> possible to verify the resulting RX contents. E.g. for the purposes of
> LSMs, can't we use the .sigstruct file as a proxy for the enclave and
> require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
I think it’s sound for some but not all use cases. I would imagine that a lot
of users won’t restrict sigstruct at all — the “use this as a sigstruct”
permission will be granted to everything and maybe even to memfd. But even
users like that might want to force their enclaves to be hardened such that
writable pages are never executable, in which case Graphene may need an
exception to run.
But maybe I’m nuts.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 24, 2019, at 10:07 AM, Sean Christopherson > wrote: > >> On Fri, May 24, 2019 at 09:43:27AM -0700, Andy Lutomirski wrote: >>> On Fri, May 24, 2019 at 12:24 AM Xing, Cedric wrote: >>> /** >>> * Summary: >>> * - The enclave file resembles a shared object that contains RO/RX/RW >>> segments >>> * - FILE__* are assigned to /dev/sgx/enclave, to determine acceptable >>> permissions to mmap()/mprotect(), valid combinations are >>> * + FILE__READ - Allow SGX1 enclaves only >>> * + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data segments >>> (e.g. heaps, stacks, etc.) >> >> I think this is a non-starter :( FILE__WRITE also means that you can >> write to the file, and the admin / policy author will almost never >> want to allow that. > > Why would FILE__WRITE on /dev/sgx/enclave be a problem? An actual > write to /dev/sgx/enclave would yield -EINVAL, no? Bah, read it wrong — FILE__WRITE on the enclave file on disk is no good.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 11:41:29AM -0400, Stephen Smalley wrote:
> On 5/24/19 3:24 AM, Xing, Cedric wrote:
> >/**
> > * Summary:
> > * - The enclave file resembles a shared object that contains RO/RX/RW
> > segments
> > * - FILE__* are assigned to /dev/sgx/enclave, to determine acceptable
> > permissions to mmap()/mprotect(), valid combinations are
> > * + FILE__READ - Allow SGX1 enclaves only
> > * + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data segments
> > (e.g. heaps, stacks, etc.)
> > * + FILE__READ|FILE__WRITE|FILE__EXECUTE - Allow SGX2 enclaves to expend
> > both data and code segments. This is necessary to support dynamically
> > linked enclaves (e.g. Graphene)
> > * + FILE__READ|FILE__EXECUTE - Allow RW->RX changes for SGX1 enclaves -
> > necessary to support dynamically linked enclaves (e.g. Graphene) on SGX1.
> > EXECMEM is also required for this to work
>
> I think EXECMOD would fit better than EXECMEM for this case; the former is
> applied for RW->RX changes for private file mappings while the latter is
> applied for WX private file mappings.
>
> > * + - Disallow the calling process to launch any enclaves
> > */
> >
> >/* Step 1: mmap() the enclave file according to the segment attributes
> >(similar to what dlopen() would do for regular shared objects) */
> >int image_fd = open("/path/to/enclave/file", O_RDONLY);
>
> FILE__READ checked to enclave file upon open().
>
> >foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
> > /* below is subject to LSM checks */
> > loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, > permission>, image_fd, phdr->p_offset);
>
> FILE__READ revalidated and FILE__EXECUTE checked to enclave file upon mmap()
> for PROT_READ and PROT_EXEC respectively. FILE__WRITE not checked even for
> PROT_WRITE mappings since it is a private file mapping and writes do not
> reach the file. EXECMEM checked if any segment permission has both W and X
> simultaneously. EXECMOD checked on any subsequent mprotect() RW->RX changes
> (if modified).
Hmm, I've been thinking more about pulling permissions from the source
page. Conceptually I'm not sure we need to meet the same requirements as
non-enclave DSOs while the enclave is being built, i.e. do we really need
to force userspace to fully map the enclave in normal memory?
Consider the Graphene scenario where it's building an enclave on the fly.
Pulling permissions from the source VMAs means Graphene has to map the
code pages of the enclave with X. This means Graphene will need EXEDMOD
(or EXECMEM if Graphene isn't careful). In a non-SGX scenario this makes
perfect sense since there is no way to verify the end result of RW->RX.
But for SGX, assuming enclaves are whitelisted by their sigstruct (checked
at EINIT) and because page permissions affect sigstruct.MRENCLAVE, it *is*
possible to verify the resulting RX contents. E.g. for the purposes of
LSMs, can't we use the .sigstruct file as a proxy for the enclave and
require FILE__EXECUTE on the .sigstruct inode to map/run the enclave?
Stephen, is my logic sound?
If so...
- Require FILE__READ+FILE__EXECUTE on .sigstruct to mmap() the enclave.
- Prevent userspace from mapping the enclave with permissions beyond the
original permissions of the enclave. This can be done by populating
VM_MAY{READ,WRITE,EXEC} from the SECINFO (same basic concept as Andy's
proposals). E.g. pre-EINIT, mmap() and mprotect() can only succeed
with PROT_NONE.
- Require FILE__{READ,WRITE,EXECUTE} on /dev/sgx/enclave for simplicity,
or provide an alternate SGX_IOC_MPROTECT if we want to sidestep the
FILE__WRITE requirement.
No changes are required to LSMs, SGX1 has a single LSM touchpoint in its
mmap(), and I *think* the only required userspace change is to mmap()
PROT_NONE when allocating the enclave's virtual address range.
As for Graphene, it doesn't need extra permissions to run its enclaves,
it just needs a way to install .sigstruct, which is a generic permissions
problem and not SGX specific.
For SGX2 maybe:
- No additional requirements to map an EAUG'd page as RW page. Not
aligned with standard MAP_SHARED behavior, but we really don't want
to require FILE__WRITE, and thus allow writes to .sigstruct.
- Require FILE__EXECMOD on the .sigstruct to map previously writable
page as executable (which indirectly includes all EAUG'd pages).
Wiring this up will be a little funky, but we again we don't want
to require FILE__WRITE on .sigstruct.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 09:43:27AM -0700, Andy Lutomirski wrote: > On Fri, May 24, 2019 at 12:24 AM Xing, Cedric wrote: > > /** > > * Summary: > > * - The enclave file resembles a shared object that contains RO/RX/RW > > segments > > * - FILE__* are assigned to /dev/sgx/enclave, to determine acceptable > > permissions to mmap()/mprotect(), valid combinations are > > * + FILE__READ - Allow SGX1 enclaves only > > * + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data segments > > (e.g. heaps, stacks, etc.) > > I think this is a non-starter :( FILE__WRITE also means that you can > write to the file, and the admin / policy author will almost never > want to allow that. Why would FILE__WRITE on /dev/sgx/enclave be a problem? An actual write to /dev/sgx/enclave would yield -EINVAL, no?
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Hi Stephen,
> On 5/24/19 3:24 AM, Xing, Cedric wrote:
> >
> > When we talk about EPC page permissions with SGX2 in mind, I think we
> should distinguish between initial permissions and runtime permissions.
> Initial permissions refer to the page permissions set at EADD. They are
> technically set by "untrusted" code so should go by policies similar to
> those applicable to regular shared objects. Runtime permissions refer to
> the permissions granted by EMODPE, EAUG and EACCEPTCOPY. They are
> resulted from inherent behavior of the enclave, which in theory is
> determined by the enclave's measurements (MRENCLAVE and/or MRSIGNER).
> >
> > And we have 2 distinct files to work with - the enclave file and
> /dev/sgx/enclave. And I consider the enclave file a logical source for
> initial permissions while /dev/sgx/enclave is a means to control runtime
> permissions. Then we can have a simpler approach like the pseudo code
> below.
> >
> > /**
> > * Summary:
> > * - The enclave file resembles a shared object that contains
> RO/RX/RW segments
> > * - FILE__* are assigned to /dev/sgx/enclave, to determine
> acceptable permissions to mmap()/mprotect(), valid combinations are
> > * + FILE__READ - Allow SGX1 enclaves only
> > * + FILE__READ|FILE__WRITE - Allow SGX2 enclaves to expand data
> segments (e.g. heaps, stacks, etc.)
> > * + FILE__READ|FILE__WRITE|FILE__EXECUTE - Allow SGX2 enclaves to
> expend both data and code segments. This is necessary to support
> dynamically linked enclaves (e.g. Graphene)
> > * + FILE__READ|FILE__EXECUTE - Allow RW->RX changes for SGX1
> enclaves - necessary to support dynamically linked enclaves (e.g.
> Graphene) on SGX1. EXECMEM is also required for this to work
>
> I think EXECMOD would fit better than EXECMEM for this case; the former
> is applied for RW->RX changes for private file mappings while the latter
> is applied for WX private file mappings.
>
> > * + - Disallow the calling process to launch any enclaves
> > */
> >
> > /* Step 1: mmap() the enclave file according to the segment attributes
> > (similar to what dlopen() would do for regular shared objects) */ int
> > image_fd = open("/path/to/enclave/file", O_RDONLY);
>
> FILE__READ checked to enclave file upon open().
Yes. We'd like to have the enclave file pass LSM/IMA checks and let EPC pages
"inherit" the permissions from it as "initial" permissions.
>
> > foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
> > /* below is subject to LSM checks */
> > loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE,
> > , image_fd, phdr->p_offset);
>
> FILE__READ revalidated and FILE__EXECUTE checked to enclave file upon
> mmap() for PROT_READ and PROT_EXEC respectively. FILE__WRITE not
> checked even for PROT_WRITE mappings since it is a private file mapping
> and writes do not reach the file. EXECMEM checked if any segment
> permission has both W and X simultaneously. EXECMOD checked on any
> subsequent mprotect() RW->RX changes (if modified).
Yes. The intention here is to make sure all X pages come directly from file
(unless EXECMEM or EXECMOD is granted). And because the driver will grant X
only if the source page also has X, we can assert that all executable EPC pages
are loaded from a file that has passed LSM/IMA checks.
>
> > }
> >
> > /* Step 2: Create enclave */
> > int enclave_fd = open("/dev/sgx/enclave", O_RDONLY /* or O_RDWR for
> > SGX2 enclaves */);
>
> FILE__READ checked (SGX1) or both FILE__READ and FILE__WRITE checked
> (SGX2) to /dev/sgx/enclave upon open(). Assuming that we are returning
> an open file referencing the /dev/sgx/enclave inode and not an anon
> inode, else we lose all subsequent FILE__* checking on mmap/mprotect and
> trigger EXECMEM on any mmap/mprotect PROT_EXEC.
Yes, the returned fd will be referencing /dev/sgx/enclave. The intention here
is to limit EPC "runtime" permissions by the permissions granted to
/dev/sgx/enclave, in order to allow user/administrator to specify what kinds of
enclaves a given process can launch. Per your earlier comments, FILE__EXECMOD
is probably also needed to support dynamically linked enclaves (that require
RW->RX changes).
>
> > void *enclave_base = mmap(NULL, , MAP_SHARED, PROT_READ,
> > enclave_fd, 0); /* Only FILE__READ is required here */
>
> FILE__READ revalidated to /dev/sgx/enclave upon mmap().
Yes. This mmap() is to set "default" permissions for regions that do *not* have
EPC pages populated. It is significant only for SGX2, to specify what action to
take by the SGX driver upon #PF with those regions. For example, a R attempt
(usually triggered by EACCEPT) within a RW region will cause SGX driver to EAUG
a page at the fault address.
>
> > ioctl(enclave_fd, IOC_ECREATE, ...);
> >
> > /* Step 3: EADD and map initial EPC pages */ foreach s in
> > loadable_segments {
> > /* IOC_EADD_AND_MAP_SEGMENT will make sure s->perm is a subset of
> VMA permissions of the
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 24, 2019 at 12:24 AM Xing, Cedric wrote:
>
> Hi Andy,
>
> > From: Andy Lutomirski [mailto:l...@kernel.org]
> > Sent: Thursday, May 23, 2019 6:18 PM
> >
> > On Thu, May 23, 2019 at 4:40 PM Sean Christopherson
> >
> > wrote:
> > >
> > > On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> > > > On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> > > > wrote:
> > > > >
> > > > > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > > > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > > > > But actually, there's no need to disallow mmap() after ECREATE
> > > > > > > since the LSM checks also apply to mmap(), e.g. FILE__EXECUTE
> > > > > > > would be needed to
> > > > > > > mmap() any enclave pages PROT_EXEC. I guess my past self
> > > > > > > thought mmap() bypassed LSM checks? The real problem is that
> > > > > > > mmap()'ng an existing enclave would require FILE__WRITE and
> > > > > > > FILE__EXECUTE, which puts us back at square one.
> > > > > >
> > > > > > I'm lost with the constraints we want to set.
> > > > >
> > > > > As is today, SELinux policies would require enclave loaders to
> > > > > have FILE__WRITE and FILE__EXECUTE permissions on
> > > > > /dev/sgx/enclave. Presumably other LSMs have similar
> > > > > requirements. Requiring all processes to have
> > > > > FILE__{WRITE,EXECUTE} permissions means the permissions don't add
> > > > > much value, e.g. they can't be used to distinguish between an
> > > > > enclave that is being loaded from an unmodified file and an enclave
> > > > > that is being
> > generated on the fly, e.g. Graphene.
> > > > >
> > > > > Looking back at Andy's mail, he was talking about requiring
> > > > > FILE__EXECUTE to run an enclave, so perhaps it's only FILE__WRITE
> > > > > that we're trying to special case.
> > > > >
> > > >
> > > > I thought about this some more, and I have a new proposal that helps
> > > > address the ELRANGE alignment issue and the permission issue at the
> > > > cost of some extra verbosity. Maybe you all can poke holes in it :)
> > > > The basic idea is to make everything more explicit from a user's
> > > > perspective. Here's how it works:
> > > >
> > > > Opening /dev/sgx/enclave gives an enclave_fd that, by design,
> > > > doesn't give EXECUTE or WRITE. mmap() on the enclave_fd only works
> > > > if you pass PROT_NONE and gives the correct alignment. The
> > > > resulting VMA cannot be mprotected or mremapped. It can't be
> > > > mmapped at all until
> > >
> > > I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
> > >
> > > > after ECREATE because the alignment isn't known before that.
> > >
> > > I don't follow. The alignment is known because userspace knows the
> > > size of its enclave. The initial unknown is the address, but that
> > > becomes known once the initial mmap() completes.
> >
> > [...]
> >
> > I think I made the mistake of getting too carried away with implementation
> > details rather
> > than just getting to the point. And I misremembered the ECREATE flow --
> > oops. Let me try
> > again. First, here are some problems with some earlier proposals (mine,
> > yours
> > Cedric's):
> >
> > - Having the EADD operation always work but have different effects
> > depending on the
> > source memory permissions is, at the very least, confusing.
>
> Inheriting permissions from source pages IMHO is the easiest way to validate
> the EPC permissions without any changes to LSM. And the argument about its
> security is also easy to make.
>
> I understand that it may take some effort to document it properly but
> otherwise don't see any practical issues with it.
My objection is to the fact that it's implicit. I have no problem
with some operation succeeding if the source address is X and failing
if it's !X, but I don't think it's fantastic to have it succeed in
either case but do different things.
For what it's worth, while this is a bit of a theoretical issue for X,
but I think it's a real problem with W. To avoid accidentally mapping
an enclave page X and then later mapping the same page W (potentially
in a different VMA), I think it will be a lot simpler if the driver
can track which pages are allowed to ever be W. We definitely *don't*
want an interface in which the eventual writability of a page is
inferred from the W permission on the source address, since we do
*not* want to force anyone to map their enclave file PROT_WRITE or
even to open it O_RDWR.
With the explicit MAXPERM passed in, this issue goes away. You can
specify W if you want W.
>
> >
> > - If we want to encourage user programs to be well-behaved, we want to
> > make it easy to
> > map the RX parts of an enclave RX, the RW parts RW, the RO parts R, etc.
> > But this
> > interacts poorly with the sgx_mmap() alignment magic, as you've pointed out.
> >
> > - We don't want to couple LSMs with SGX too tightly.
> >
> > So here's how a nice interface
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/24/19 3:24 AM, Xing, Cedric wrote:
Hi Andy,
From: Andy Lutomirski [mailto:l...@kernel.org]
Sent: Thursday, May 23, 2019 6:18 PM
On Thu, May 23, 2019 at 4:40 PM Sean Christopherson
wrote:
On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
wrote:
On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
But actually, there's no need to disallow mmap() after ECREATE
since the LSM checks also apply to mmap(), e.g. FILE__EXECUTE
would be needed to
mmap() any enclave pages PROT_EXEC. I guess my past self
thought mmap() bypassed LSM checks? The real problem is that
mmap()'ng an existing enclave would require FILE__WRITE and
FILE__EXECUTE, which puts us back at square one.
I'm lost with the constraints we want to set.
As is today, SELinux policies would require enclave loaders to
have FILE__WRITE and FILE__EXECUTE permissions on
/dev/sgx/enclave. Presumably other LSMs have similar
requirements. Requiring all processes to have
FILE__{WRITE,EXECUTE} permissions means the permissions don't add
much value, e.g. they can't be used to distinguish between an
enclave that is being loaded from an unmodified file and an enclave that is
being
generated on the fly, e.g. Graphene.
Looking back at Andy's mail, he was talking about requiring
FILE__EXECUTE to run an enclave, so perhaps it's only FILE__WRITE
that we're trying to special case.
I thought about this some more, and I have a new proposal that helps
address the ELRANGE alignment issue and the permission issue at the
cost of some extra verbosity. Maybe you all can poke holes in it :)
The basic idea is to make everything more explicit from a user's
perspective. Here's how it works:
Opening /dev/sgx/enclave gives an enclave_fd that, by design,
doesn't give EXECUTE or WRITE. mmap() on the enclave_fd only works
if you pass PROT_NONE and gives the correct alignment. The
resulting VMA cannot be mprotected or mremapped. It can't be
mmapped at all until
I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
after ECREATE because the alignment isn't known before that.
I don't follow. The alignment is known because userspace knows the
size of its enclave. The initial unknown is the address, but that
becomes known once the initial mmap() completes.
[...]
I think I made the mistake of getting too carried away with implementation
details rather
than just getting to the point. And I misremembered the ECREATE flow -- oops.
Let me try
again. First, here are some problems with some earlier proposals (mine, yours
Cedric's):
- Having the EADD operation always work but have different effects depending
on the
source memory permissions is, at the very least, confusing.
Inheriting permissions from source pages IMHO is the easiest way to validate
the EPC permissions without any changes to LSM. And the argument about its
security is also easy to make.
I understand that it may take some effort to document it properly but otherwise
don't see any practical issues with it.
- If we want to encourage user programs to be well-behaved, we want to make
it easy to
map the RX parts of an enclave RX, the RW parts RW, the RO parts R, etc. But
this
interacts poorly with the sgx_mmap() alignment magic, as you've pointed out.
- We don't want to couple LSMs with SGX too tightly.
So here's how a nice interface might work:
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
/* enclave_fd points to a totally blank enclave. Before ECREATE, we need to
decide on an
address. */
void *addr = mmap(NULL, size, PROT_NONE, MAP_SHARED, enclave_fd, 0);
/* we have an address! */
ioctl(enclave_fd, ECREATE, ...);
/* now add some data to the enclave. We want the RWX addition to fail
immediately unless we have the relevant LSM pemission. Similarly, we
want the RX addition to fail immediately unless the source VMA is appropriate.
*/
ioctl(enclave_fd, EADD, rx_source_1, MAXPERM=RX, ...); [the ...
includes SECINFO, which the kernel doesn't really care about] ioctl(enclave_fd,
EADD,
ro_source_1, MAXPERM=RX ...); ioctl(enclave_fd, EADD, rw_source_1, MAXPERM=RW
...);
ioctl(enclave_fd, EADD, rwx_source_1, MAXPERM=RWX ...);
If MAXPERM is taken from ioctl parameters, the real question here is how to
validate MAXPERM. Guess we shouldn't allow arbitrary MAXPERM to be specified by
user code, and the only logical source I can think of is from the source pages
(or from the enclave source file, but memory mapping is preferred because it
offers more flexibility).
ioctl(enclave_fd, EINIT, ...); /* presumably pass sigstruct_fd here, too. */
/* at this point, all is well except that the enclave is mapped PROT_NONE.
There are a
couple ways I can imagine to fix this. */
We could use mmap:
mmap(baseaddr+offset, len, PROT_READ, MAP_SHARED | MAP_FIXED, enclave_fd, 0);
/* only
succeeds if
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/23/19 11:38 AM, Andy Lutomirski wrote:
On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
wrote:
On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
But actually, there's no need to disallow mmap() after ECREATE since the
LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
bypassed LSM checks? The real problem is that mmap()'ng an existing
enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
at square one.
I'm lost with the constraints we want to set.
As is today, SELinux policies would require enclave loaders to have
FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
other LSMs have similar requirements. Requiring all processes to have
FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
value, e.g. they can't be used to distinguish between an enclave that is
being loaded from an unmodified file and an enclave that is being
generated on the fly, e.g. Graphene.
Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
special case.
I thought about this some more, and I have a new proposal that helps
address the ELRANGE alignment issue and the permission issue at the
cost of some extra verbosity. Maybe you all can poke holes in it :)
The basic idea is to make everything more explicit from a user's
perspective. Here's how it works:
Opening /dev/sgx/enclave gives an enclave_fd that, by design, doesn't
give EXECUTE or WRITE. mmap() on the enclave_fd only works if you
pass PROT_NONE and gives the correct alignment. The resulting VMA
cannot be mprotected or mremapped. It can't be mmapped at all until
after ECREATE because the alignment isn't known before that.
Associated with the enclave are a bunch (up to 7) "enclave segment
inodes". These are anon_inodes that are created automagically. An
enclave segment is a group of pages, not necessary contiguous, with an
upper bound on the memory permissions. Each enclave page belongs to a
segment. When you do EADD, you tell the driver what segment you're
adding to. [0] This means that EADD gets an extra argument that is a
permission mask for the page -- in addition to the initial SECINFO,
you also pass to EADD something to the effect of "I promise never to
map this with permissions greater than RX".
Then we just need some way to mmap a region from an enclave segment.
This could be done by having a way to get an fd for an enclave segment
or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT. User
code would use this operation to replace, MAP_FIXED-style, ranges from
the big PROT_NONE mapping with the relevant pages from the enclave
segment. The resulting vma would only have VM_MAYWRITE if the segment
is W, only have VM_MAYEXEC if the segment is X, and only have
VM_MAYREAD if the segment is R. Depending on implementation details,
the VMAs might need to restrict mremap() to avoid mapping pages that
aren't part of the segment in question.
It's plausible that this whole thing works without the magic segment
inodes under the hood, but figuring that out would need a careful look
at how all the core mm bits and LSM bits work together.
To get all the LSM stuff to work, SELinux will need some way to
automatically assign an appropriate label to the segment inodes. I
assume that such a mechanism already exists and gets used for things
like sockets, but I haven't actually confirmed this.
I don't follow that. socket inodes are not anon inodes, and anon inodes
have no per-instance data by definition, and typically you're only
dealing with a single anon inode for all files, and hence they were long
ago marked S_PRIVATE and exempted from all LSM checking except for
EXECMEM on mmap/mprotect PROT_EXEC. We have no way to perform useful
security checking on them currently. socket inodes we can label from
their creating process but even that's not going to support multiple
labels for different sockets created by the same process unless the
process explicitly used setsockcreatecon(3) aka /proc/self/attr/sockcreate
[0] There needs to be some vaguely intelligent semantics if you EADD
the *same* address more than once. A simple solution would be to
disallow it if the segments don't match.
RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
Hi Andy,
> From: Andy Lutomirski [mailto:l...@kernel.org]
> Sent: Thursday, May 23, 2019 6:18 PM
>
> On Thu, May 23, 2019 at 4:40 PM Sean Christopherson
>
> wrote:
> >
> > On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> > > On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> > > wrote:
> > > >
> > > > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > > > But actually, there's no need to disallow mmap() after ECREATE
> > > > > > since the LSM checks also apply to mmap(), e.g. FILE__EXECUTE
> > > > > > would be needed to
> > > > > > mmap() any enclave pages PROT_EXEC. I guess my past self
> > > > > > thought mmap() bypassed LSM checks? The real problem is that
> > > > > > mmap()'ng an existing enclave would require FILE__WRITE and
> > > > > > FILE__EXECUTE, which puts us back at square one.
> > > > >
> > > > > I'm lost with the constraints we want to set.
> > > >
> > > > As is today, SELinux policies would require enclave loaders to
> > > > have FILE__WRITE and FILE__EXECUTE permissions on
> > > > /dev/sgx/enclave. Presumably other LSMs have similar
> > > > requirements. Requiring all processes to have
> > > > FILE__{WRITE,EXECUTE} permissions means the permissions don't add
> > > > much value, e.g. they can't be used to distinguish between an
> > > > enclave that is being loaded from an unmodified file and an enclave
> > > > that is being
> generated on the fly, e.g. Graphene.
> > > >
> > > > Looking back at Andy's mail, he was talking about requiring
> > > > FILE__EXECUTE to run an enclave, so perhaps it's only FILE__WRITE
> > > > that we're trying to special case.
> > > >
> > >
> > > I thought about this some more, and I have a new proposal that helps
> > > address the ELRANGE alignment issue and the permission issue at the
> > > cost of some extra verbosity. Maybe you all can poke holes in it :)
> > > The basic idea is to make everything more explicit from a user's
> > > perspective. Here's how it works:
> > >
> > > Opening /dev/sgx/enclave gives an enclave_fd that, by design,
> > > doesn't give EXECUTE or WRITE. mmap() on the enclave_fd only works
> > > if you pass PROT_NONE and gives the correct alignment. The
> > > resulting VMA cannot be mprotected or mremapped. It can't be
> > > mmapped at all until
> >
> > I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
> >
> > > after ECREATE because the alignment isn't known before that.
> >
> > I don't follow. The alignment is known because userspace knows the
> > size of its enclave. The initial unknown is the address, but that
> > becomes known once the initial mmap() completes.
>
> [...]
>
> I think I made the mistake of getting too carried away with implementation
> details rather
> than just getting to the point. And I misremembered the ECREATE flow --
> oops. Let me try
> again. First, here are some problems with some earlier proposals (mine, yours
> Cedric's):
>
> - Having the EADD operation always work but have different effects depending
> on the
> source memory permissions is, at the very least, confusing.
Inheriting permissions from source pages IMHO is the easiest way to validate
the EPC permissions without any changes to LSM. And the argument about its
security is also easy to make.
I understand that it may take some effort to document it properly but otherwise
don't see any practical issues with it.
>
> - If we want to encourage user programs to be well-behaved, we want to make
> it easy to
> map the RX parts of an enclave RX, the RW parts RW, the RO parts R, etc. But
> this
> interacts poorly with the sgx_mmap() alignment magic, as you've pointed out.
>
> - We don't want to couple LSMs with SGX too tightly.
>
> So here's how a nice interface might work:
>
> int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>
> /* enclave_fd points to a totally blank enclave. Before ECREATE, we need to
> decide on an
> address. */
>
> void *addr = mmap(NULL, size, PROT_NONE, MAP_SHARED, enclave_fd, 0);
>
> /* we have an address! */
>
> ioctl(enclave_fd, ECREATE, ...);
>
> /* now add some data to the enclave. We want the RWX addition to fail
> immediately unless we have the relevant LSM pemission. Similarly, we
> want the RX addition to fail immediately unless the source VMA is
> appropriate. */
>
> ioctl(enclave_fd, EADD, rx_source_1, MAXPERM=RX, ...); [the ...
> includes SECINFO, which the kernel doesn't really care about]
> ioctl(enclave_fd, EADD,
> ro_source_1, MAXPERM=RX ...); ioctl(enclave_fd, EADD, rw_source_1, MAXPERM=RW
> ...);
> ioctl(enclave_fd, EADD, rwx_source_1, MAXPERM=RWX ...);
If MAXPERM is taken from ioctl parameters, the real question here is how to
validate MAXPERM. Guess we shouldn't allow arbitrary MAXPERM to be specified by
user code, and the only logical source I can think of is from the source pages
(or from the enclave source file, but
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 4:40 PM Sean Christopherson
wrote:
>
> On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> > On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> > wrote:
> > >
> > > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > > But actually, there's no need to disallow mmap() after ECREATE since
> > > > > the
> > > > > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > > > > mmap() any enclave pages PROT_EXEC. I guess my past self thought
> > > > > mmap()
> > > > > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > > > > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us
> > > > > back
> > > > > at square one.
> > > >
> > > > I'm lost with the constraints we want to set.
> > >
> > > As is today, SELinux policies would require enclave loaders to have
> > > FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
> > > other LSMs have similar requirements. Requiring all processes to have
> > > FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
> > > value, e.g. they can't be used to distinguish between an enclave that is
> > > being loaded from an unmodified file and an enclave that is being
> > > generated on the fly, e.g. Graphene.
> > >
> > > Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
> > > to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
> > > special case.
> > >
> >
> > I thought about this some more, and I have a new proposal that helps
> > address the ELRANGE alignment issue and the permission issue at the
> > cost of some extra verbosity. Maybe you all can poke holes in it :)
> > The basic idea is to make everything more explicit from a user's
> > perspective. Here's how it works:
> >
> > Opening /dev/sgx/enclave gives an enclave_fd that, by design, doesn't
> > give EXECUTE or WRITE. mmap() on the enclave_fd only works if you
> > pass PROT_NONE and gives the correct alignment. The resulting VMA
> > cannot be mprotected or mremapped. It can't be mmapped at all until
>
> I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
>
> > after ECREATE because the alignment isn't known before that.
>
> I don't follow. The alignment is known because userspace knows the size
> of its enclave. The initial unknown is the address, but that becomes
> known once the initial mmap() completes.
[...]
I think I made the mistake of getting too carried away with
implementation details rather than just getting to the point. And I
misremembered the ECREATE flow -- oops. Let me try again. First,
here are some problems with some earlier proposals (mine, yours
Cedric's):
- Having the EADD operation always work but have different effects
depending on the source memory permissions is, at the very least,
confusing.
- If we want to encourage user programs to be well-behaved, we want
to make it easy to map the RX parts of an enclave RX, the RW parts RW,
the RO parts R, etc. But this interacts poorly with the sgx_mmap()
alignment magic, as you've pointed out.
- We don't want to couple LSMs with SGX too tightly.
So here's how a nice interface might work:
int enclave_fd = open("/dev/sgx/enclave", O_RDWR);
/* enclave_fd points to a totally blank enclave. Before ECREATE, we
need to decide on an address. */
void *addr = mmap(NULL, size, PROT_NONE, MAP_SHARED, enclave_fd, 0);
/* we have an address! */
ioctl(enclave_fd, ECREATE, ...);
/* now add some data to the enclave. We want the RWX addition to fail
immediately unless we have the relevant LSM pemission. Similarly, we
want the RX addition to fail immediately unless the source VMA is
appropriate. */
ioctl(enclave_fd, EADD, rx_source_1, MAXPERM=RX, ...); [the ...
includes SECINFO, which the kernel doesn't really care about]
ioctl(enclave_fd, EADD, ro_source_1, MAXPERM=RX ...);
ioctl(enclave_fd, EADD, rw_source_1, MAXPERM=RW ...);
ioctl(enclave_fd, EADD, rwx_source_1, MAXPERM=RWX ...);
ioctl(enclave_fd, EINIT, ...); /* presumably pass sigstruct_fd here, too. */
/* at this point, all is well except that the enclave is mapped
PROT_NONE. There are a couple ways I can imagine to fix this. */
We could use mmap:
mmap(baseaddr+offset, len, PROT_READ, MAP_SHARED | MAP_FIXED,
enclave_fd, 0); /* only succeeds if MAXPERM & R == R */
But this has some annoying implications with regard to
sgx_get_unmapped_area(). We could use an ioctl:
ioctl(enclave_fd, SGX_IOC_MPROTECT, offset, len, PROT_READ);
which has the potentially nice property that we can completely bypass
the LSM hooks, because the LSM has *already* vetted everything when
the EADD calls were allowed. Or we could maybe even just use
mprotect() itself:
mprotect(baseaddr + offset, len, PROT_READ);
Or, for the really evil option, we could use a bit of magic in .fault
and do nothing here. Instead we'd
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> wrote:
> >
> > On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > > But actually, there's no need to disallow mmap() after ECREATE since the
> > > > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > > > mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> > > > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > > > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> > > > at square one.
> > >
> > > I'm lost with the constraints we want to set.
> >
> > As is today, SELinux policies would require enclave loaders to have
> > FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
> > other LSMs have similar requirements. Requiring all processes to have
> > FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
> > value, e.g. they can't be used to distinguish between an enclave that is
> > being loaded from an unmodified file and an enclave that is being
> > generated on the fly, e.g. Graphene.
> >
> > Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
> > to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
> > special case.
> >
>
> I thought about this some more, and I have a new proposal that helps
> address the ELRANGE alignment issue and the permission issue at the
> cost of some extra verbosity. Maybe you all can poke holes in it :)
> The basic idea is to make everything more explicit from a user's
> perspective. Here's how it works:
>
> Opening /dev/sgx/enclave gives an enclave_fd that, by design, doesn't
> give EXECUTE or WRITE. mmap() on the enclave_fd only works if you
> pass PROT_NONE and gives the correct alignment. The resulting VMA
> cannot be mprotected or mremapped. It can't be mmapped at all until
I assume you're thinking of clearing all VM_MAY* flags in sgx_mmap()?
> after ECREATE because the alignment isn't known before that.
I don't follow. The alignment is known because userspace knows the size
of its enclave. The initial unknown is the address, but that becomes
known once the initial mmap() completes.
> Associated with the enclave are a bunch (up to 7) "enclave segment
I assume 7 = R, W, X, RW, RX, WX and RWX?
> inodes". These are anon_inodes that are created automagically. An
> enclave segment is a group of pages, not necessary contiguous, with an
> upper bound on the memory permissions. Each enclave page belongs to a
> segment. When you do EADD, you tell the driver what segment you're
> adding to. [0] This means that EADD gets an extra argument that is a
> permission mask for the page -- in addition to the initial SECINFO,
> you also pass to EADD something to the effect of "I promise never to
> map this with permissions greater than RX".
>
> Then we just need some way to mmap a region from an enclave segment.
> This could be done by having a way to get an fd for an enclave segment
> or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT. User
> code would use this operation to replace, MAP_FIXED-style, ranges from
> the big PROT_NONE mapping with the relevant pages from the enclave
> segment. The resulting vma would only have VM_MAYWRITE if the segment
> is W, only have VM_MAYEXEC if the segment is X, and only have
> VM_MAYREAD if the segment is R. Depending on implementation details,
> the VMAs might need to restrict mremap() to avoid mapping pages that
> aren't part of the segment in question.
If my above assumptions regarding VM_MAY* and the "7 segments" are
correct, IIUC you're proposing that an LSM could have policies for each
of the anon inodes, e.g. grant/deny RWX vs. RW vs RX. Am I in the
ballpark?
> It's plausible that this whole thing works without the magic segment
> inodes under the hood, but figuring that out would need a careful look
> at how all the core mm bits and LSM bits work together.
>
> To get all the LSM stuff to work, SELinux will need some way to
> automatically assign an appropriate label to the segment inodes. I
> assume that such a mechanism already exists and gets used for things
> like sockets, but I haven't actually confirmed this.
I (obviously) don't fully understand your proposal, but I don't think we
want to hook inodes, e.g. AppArmor doesn't implement inode_permission()
but does implement file_mprotect() and mmap_file(), which feel like the
natural hooks for this sort of thing. I also think it's overkill, e.g.
AppArmor doesn't have a concept of EXECMOD, EXECMEM, EXECHEAP, etc.., so
I don't think we need to go beyond detecting W+X scenarios.
Starting with your original idea of tracking "safe to execute" and
Cedric's of propagating the permissions from the source VMA, but tweaked
with your new idea of clearing VM_MAY* and a custom
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 07:17:52AM -0700, Sean Christopherson wrote:
> On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > But actually, there's no need to disallow mmap() after ECREATE since the
> > > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > > mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> > > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> > > at square one.
> >
> > I'm lost with the constraints we want to set.
>
> As is today, SELinux policies would require enclave loaders to have
> FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
> other LSMs have similar requirements. Requiring all processes to have
> FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
> value, e.g. they can't be used to distinguish between an enclave that is
> being loaded from an unmodified file and an enclave that is being
> generated on the fly, e.g. Graphene.
>
> Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
> to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
> special case.
Argh, as I was working through Andy's latest proposal I realized that I
was subconciously making FILE__READ imply FILE__EXECUTE.
The idea behind inheriting permissions from the source VMA is to exempt
"standard" enclaves from needing FILE__WRITE. But if we don't add an
exemption for FILE__EXECUTE as well, then all enclaves need FILE__EXECUTE,
which means FILE__EXECUTE can't be used to identify the case where
userspace is mapping an inherited PROT_WRITE page as PROT_EXEC. And if
the SGX magic exempts FILE__EXECUTE, then FILE__READ implies FILE__EXECUTE.
Yuck.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
wrote:
>
> On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> > On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > > But actually, there's no need to disallow mmap() after ECREATE since the
> > > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > > mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> > > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> > > at square one.
> >
> > I'm lost with the constraints we want to set.
>
> As is today, SELinux policies would require enclave loaders to have
> FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
> other LSMs have similar requirements. Requiring all processes to have
> FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
> value, e.g. they can't be used to distinguish between an enclave that is
> being loaded from an unmodified file and an enclave that is being
> generated on the fly, e.g. Graphene.
>
> Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
> to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
> special case.
>
I thought about this some more, and I have a new proposal that helps
address the ELRANGE alignment issue and the permission issue at the
cost of some extra verbosity. Maybe you all can poke holes in it :)
The basic idea is to make everything more explicit from a user's
perspective. Here's how it works:
Opening /dev/sgx/enclave gives an enclave_fd that, by design, doesn't
give EXECUTE or WRITE. mmap() on the enclave_fd only works if you
pass PROT_NONE and gives the correct alignment. The resulting VMA
cannot be mprotected or mremapped. It can't be mmapped at all until
after ECREATE because the alignment isn't known before that.
Associated with the enclave are a bunch (up to 7) "enclave segment
inodes". These are anon_inodes that are created automagically. An
enclave segment is a group of pages, not necessary contiguous, with an
upper bound on the memory permissions. Each enclave page belongs to a
segment. When you do EADD, you tell the driver what segment you're
adding to. [0] This means that EADD gets an extra argument that is a
permission mask for the page -- in addition to the initial SECINFO,
you also pass to EADD something to the effect of "I promise never to
map this with permissions greater than RX".
Then we just need some way to mmap a region from an enclave segment.
This could be done by having a way to get an fd for an enclave segment
or it could be done by having a new ioctl SGX_IOC_MAP_SEGMENT. User
code would use this operation to replace, MAP_FIXED-style, ranges from
the big PROT_NONE mapping with the relevant pages from the enclave
segment. The resulting vma would only have VM_MAYWRITE if the segment
is W, only have VM_MAYEXEC if the segment is X, and only have
VM_MAYREAD if the segment is R. Depending on implementation details,
the VMAs might need to restrict mremap() to avoid mapping pages that
aren't part of the segment in question.
It's plausible that this whole thing works without the magic segment
inodes under the hood, but figuring that out would need a careful look
at how all the core mm bits and LSM bits work together.
To get all the LSM stuff to work, SELinux will need some way to
automatically assign an appropriate label to the segment inodes. I
assume that such a mechanism already exists and gets used for things
like sockets, but I haven't actually confirmed this.
[0] There needs to be some vaguely intelligent semantics if you EADD
the *same* address more than once. A simple solution would be to
disallow it if the segments don't match.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 01:26:28PM +0300, Jarkko Sakkinen wrote:
> On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> > But actually, there's no need to disallow mmap() after ECREATE since the
> > LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> > mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> > bypassed LSM checks? The real problem is that mmap()'ng an existing
> > enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> > at square one.
>
> I'm lost with the constraints we want to set.
As is today, SELinux policies would require enclave loaders to have
FILE__WRITE and FILE__EXECUTE permissions on /dev/sgx/enclave. Presumably
other LSMs have similar requirements. Requiring all processes to have
FILE__{WRITE,EXECUTE} permissions means the permissions don't add much
value, e.g. they can't be used to distinguish between an enclave that is
being loaded from an unmodified file and an enclave that is being
generated on the fly, e.g. Graphene.
Looking back at Andy's mail, he was talking about requiring FILE__EXECUTE
to run an enclave, so perhaps it's only FILE__WRITE that we're trying to
special case.
> We can still support fork() if we take a step back from v20 and require
> the mmap(). Given the recent comments, I'd guess that is the best
> compromise i.e. multiple processes can still share an enclave within
> the limitations of ancestor hierarchy. Is this the constraint we agree
> now upon? Some emails are a bit contradicting in this sense.
>
> > Tracking permissions per enclave page isn't difficult, it's the new SGX
> > specific LSM hooks and mprotect() interactions that I want to avoid.
> >
> > Jumping back to mmap(), AIUI the fundamental issue is that we want to
> > allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
> > otherwise FILE__WRITE and FILE__EXECUTE become meaningless. Assuming I'm
> > not off in the weeds, that means we really just need to special case
> > mmap() on enclaves so it can map enclave memory using the verified page
> > permissions so as not to run afoul of LSM checks. All other behaviors,
> > e.g. mprotect(), can reuse the existing LSM checks for shared mappings.
> >
> > So, what if we snapshot the permissions for each enclave page at EADD,
> > and then special case mmap() to propagate flags from the snapshot to the
> > VMA? More or less the same idea as doing mprotect_fixup() using the
> > source VMA during EADD. We could define the EADD semantics to match
> > this as well, e.g. only propagate the flags from the source VMA to the
> > enclave VMA if the EADD range is fully mapped with PROT_NONE. This would
> > allow the enclave builder concept, albeit with funky semantics, and
> > wouldn't require new LSM hooks.
>
> Dropped off here completely. What if the mmap() is done before any of
> the EADD operations?
Three options I can think of, in descending order of magic required:
1. Do nothing. Userspace would essentially be required to mmap() the
enclave after EINIT, which is ugly but not breaking since userspace
could mmap() the enclave with a placeholder VMA prior to building
the enclave, and then a series of mmap() to establish its "real"
mapping.
2. Propagate the permissions from EADD to the VMAs of the current mm
if the entire EADD range is mapped and the mapping is PROT_NONE.
3. Propagate the permissions from EADD to the VMAs of all mm structs
that have mapped some piece of the enclave, following the matching
rules from #2.
> > E.g. something like this:
> >
> > static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
> > struct sgx_encl *encl)
> > {
> > struct radix_tree_iter iter;
> > struct sgx_encl_page *entry;
> > unsigned long addr;
> > vm_flags_t flags;
> > void **slot;
> >
> > /*
> > * SGX special: if userspace is requesting PROT_NONE and pages have
> > * been added to the enclave, then propagate the flags snapshot from
> > * the enclave to the VMA. Do this if and only if all overlapped
> > * pages are defined and have identical permissions. Stuffing the
> > * VMA on PROT_NONE allows userspace to map EPC pages without being
> > * incorrectly rejected by LSMs due to insufficient permissions (the
> > * snapshottted flags have alaredy been vetted).
> > */
> > if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
> > return;
> >
> > flags = 0;
> >
> > for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
> > entry = radix_tree_lookup(>page_tree, addr >> PAGE_SHIFT);
> >
> > if (!entry && flags)
> > return;
> > if (!flags && entry) {
> > if (addr == vma->vm_start) {
> > flags = entry->vm_flags;
> > continue;
> >
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 07:35:17PM -0700, Sean Christopherson wrote:
> But actually, there's no need to disallow mmap() after ECREATE since the
> LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
> mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
> bypassed LSM checks? The real problem is that mmap()'ng an existing
> enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
> at square one.
I'm lost with the constraints we want to set.
We can still support fork() if we take a step back from v20 and require
the mmap(). Given the recent comments, I'd guess that is the best
compromise i.e. multiple processes can still share an enclave within
the limitations of ancestor hierarchy. Is this the constraint we agree
now upon? Some emails are a bit contradicting in this sense.
> Tracking permissions per enclave page isn't difficult, it's the new SGX
> specific LSM hooks and mprotect() interactions that I want to avoid.
>
> Jumping back to mmap(), AIUI the fundamental issue is that we want to
> allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
> otherwise FILE__WRITE and FILE__EXECUTE become meaningless. Assuming I'm
> not off in the weeds, that means we really just need to special case
> mmap() on enclaves so it can map enclave memory using the verified page
> permissions so as not to run afoul of LSM checks. All other behaviors,
> e.g. mprotect(), can reuse the existing LSM checks for shared mappings.
>
> So, what if we snapshot the permissions for each enclave page at EADD,
> and then special case mmap() to propagate flags from the snapshot to the
> VMA? More or less the same idea as doing mprotect_fixup() using the
> source VMA during EADD. We could define the EADD semantics to match
> this as well, e.g. only propagate the flags from the source VMA to the
> enclave VMA if the EADD range is fully mapped with PROT_NONE. This would
> allow the enclave builder concept, albeit with funky semantics, and
> wouldn't require new LSM hooks.
Dropped off here completely. What if the mmap() is done before any of
the EADD operations?
>
> E.g. something like this:
>
> static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
> struct sgx_encl *encl)
> {
> struct radix_tree_iter iter;
> struct sgx_encl_page *entry;
> unsigned long addr;
> vm_flags_t flags;
> void **slot;
>
> /*
>* SGX special: if userspace is requesting PROT_NONE and pages have
>* been added to the enclave, then propagate the flags snapshot from
>* the enclave to the VMA. Do this if and only if all overlapped
>* pages are defined and have identical permissions. Stuffing the
>* VMA on PROT_NONE allows userspace to map EPC pages without being
>* incorrectly rejected by LSMs due to insufficient permissions (the
>* snapshottted flags have alaredy been vetted).
>*/
> if (vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC))
> return;
>
> flags = 0;
>
> for (addr = vma->vm_start; addr < vma->vm_end; addr += PAGE_SIZE) {
> entry = radix_tree_lookup(>page_tree, addr >> PAGE_SHIFT);
>
> if (!entry && flags)
> return;
> if (!flags && entry) {
> if (addr == vma->vm_start) {
> flags = entry->vm_flags;
> continue;
> }
> return;
> }
> if (entry && flags && entry->vm_flags != flags)
> return;
>
> }
> vma->vm_flags |= flags;
> }
This looks flakky and error prone. You'd better have some "shadow VMAs"
and check that you have such matching size of the VMA you try to mmap()
and check flags from that.
Who would call this function anyhow and when?
Would be better to first agree on constraints. I have zero idea within
which kind of enviroment this snippet would live e.g.
- mmap() (before, after?)
- multi process constraint (only fork or full on versatility)
/Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 23, 2019 at 11:10:48AM +0300, Jarkko Sakkinen wrote: > On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote: > > As far as I know from this whole discussion, we still haven't come up > > with any credible way to avoid tracking, per enclave page, whether > > that page came from unmodified PROT_EXEC memory. > > So is this in the context that the enclave is read from another VMA > and not through a file descriptor? Is that locked in? No need to answer. Got in page from Sean's response. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote: > As far as I know from this whole discussion, we still haven't come up > with any credible way to avoid tracking, per enclave page, whether > that page came from unmodified PROT_EXEC memory. So is this in the context that the enclave is read from another VMA and not through a file descriptor? Is that locked in? /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 03:42:45PM -0700, Andy Lutomirski wrote:
> On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
> wrote:
> >
> > And that straight up doesn't work with the v20 driver because mmap() with
> > the enclave_fd will run through sgx_get_unmapped_area(), which also does
> > the natural alignment adjustments (the idea being that mmap() is mapping
> > the entire enclave). E.g. mmap() will map the wrong address if the offset
> > of a chunk is less than its size due to the driver adjusting the address.
>
> That presumably needs to change.
If we want to allow mmap() on a subset of the enclave, yes. I assume it's
a simple matter of respecting MAP_FIXED.
> Are we entirely missing an API to allocate a naturally aligned VA
> range? That's kind of annoying.
Yes?
> > Eliminating sgx_get_unmapped_area() means userspace is once again on the
> > hook for naturally aligning the enclave, which is less than desirable.
> >
> > Looking back at the original API discussions around a builder process[1],
> > we never fleshed out the end-to-end flow. While having a builder process
> > *sounds* reasonable, in practice it adds a lot of complexity without
> > providing much in the way of added security. E.g. in addition to the
> > above mmap() issues, since the order of EADDs affects the enclave
> > measurement, the enclave owner would need to communicate the exact steps
> > to build the enclave, or the builder would need a priori knowledge of the
> > enclave format.
> >
> > Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
> > daemon that requires additional credentials to obtain a new enclave_fd.
> > So AFAICT, the only benefit to having a dedicated builder is that it can
> > do its own whitelisting of enclaves, but since we're trending towards
> > supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
> > whitelisting in userspace purely in userspace also provides marginal value.
> >
> > TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
>
> This isn't necessarily a problem, but we pretty much have to use
> mprotect() then.
You lost me there. Who needs to mprotect() what?
> Maybe the semantics could just be that mmap() on the SGX device gives
> natural alignment, but that there is no actual constraint enforced by
> the driver as to whether mmap() happens before or after ECREATE.
> After all, it's *ugly* for user code to reserve its address range with
> an awkward giant mmap(), there's nothing fundamentally wrong with it.
>
> As far as I know from this whole discussion, we still haven't come up
> with any credible way to avoid tracking, per enclave page, whether
> that page came from unmodified PROT_EXEC memory.
Disallowing mmap() after ECREATE is credible, but apparently not
palatable. :-)
But actually, there's no need to disallow mmap() after ECREATE since the
LSM checks also apply to mmap(), e.g. FILE__EXECUTE would be needed to
mmap() any enclave pages PROT_EXEC. I guess my past self thought mmap()
bypassed LSM checks? The real problem is that mmap()'ng an existing
enclave would require FILE__WRITE and FILE__EXECUTE, which puts us back
at square one.
Tracking permissions per enclave page isn't difficult, it's the new SGX
specific LSM hooks and mprotect() interactions that I want to avoid.
Jumping back to mmap(), AIUI the fundamental issue is that we want to
allow building/running an enclave without FILE__WRITE and FILE__EXECUTE,
otherwise FILE__WRITE and FILE__EXECUTE become meaningless. Assuming I'm
not off in the weeds, that means we really just need to special case
mmap() on enclaves so it can map enclave memory using the verified page
permissions so as not to run afoul of LSM checks. All other behaviors,
e.g. mprotect(), can reuse the existing LSM checks for shared mappings.
So, what if we snapshot the permissions for each enclave page at EADD,
and then special case mmap() to propagate flags from the snapshot to the
VMA? More or less the same idea as doing mprotect_fixup() using the
source VMA during EADD. We could define the EADD semantics to match
this as well, e.g. only propagate the flags from the source VMA to the
enclave VMA if the EADD range is fully mapped with PROT_NONE. This would
allow the enclave builder concept, albeit with funky semantics, and
wouldn't require new LSM hooks.
E.g. something like this:
static inline void sgx_mmap_update_prot_flags(struct vm_area_struct *vma,
struct sgx_encl *encl)
{
struct radix_tree_iter iter;
struct sgx_encl_page *entry;
unsigned long addr;
vm_flags_t flags;
void **slot;
/*
* SGX special: if userspace is requesting PROT_NONE and pages have
* been added to the enclave, then propagate the flags snapshot from
* the enclave to the VMA. Do this if and only if all overlapped
* pages are defined and have identical permissions. Stuffing the
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 8:38 AM Sean Christopherson
wrote:
>
> On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:
> > On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:
> > >On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:
> > >>On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:
> > >>>Except that mmap() is more or less required to guarantee that ELRANGE
> > >>>established by ECREATE is available. And we want to disallow mmap() as
> > >>>soon as the first EADD is done so that userspace can't remap the
> > >>>enclave's
> > >>>VMAs via munmap()->mmap() and gain execute permissions to pages that were
> > >>>EADD'd as NX.
> > >>
> > >>We don't want to guarantee such thing and it is not guaranteed. It does
> > >>not fit at all to the multi process work done. Enclaves are detached
> > >>from any particular process addresse spaces. It is responsibility of
> > >>process to open windows to them.
> > >>
> > >>That would be completely against work that we've done lately.
> > >
> > >Example use case: you have a process that just constructs an enclave
> > >and sends it to another process or processes for use. The constructor
> > >process could have basically anything on that range. This was the key
> > >goal of the fd based enclave work.
> >
> > What exactly happens in the constructor versus the recipient processes?
> > Which process performs each of the necessary open(), mmap(), and ioctl()
> > calls for setting up the enclave? Can you provide a high level overview of
> > the sequence of userspace calls by the constructor and by the recipient
> > similar to what Sean showed earlier for just a single process?
>
> Hmm, what we had talked about was allowing the SGX ioctls to work without
> an associated VMA, with the end goal of letting userspace restrict access
> to /dev/sgx/enclave. Very roughly...
>
> Enclave Owner:
>
> connect(builder, ...);
> send(builder, "/home/sean/path/to/my/enclave");
>
> recv(builder, _fd);
>
> for_each_chunk {
> mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
> }
>
>
> Enclave Builder:
>
> recv(sock, _path);
>
> source_fd = open(enclave_path, O_RDONLY);
> for_each_chunk {
>memory>
> }
>
> enclave_fd = open("/dev/sgx/enclave", O_RDWR);
>
> ioctl(enclave_fd, ENCLAVE_CREATE, ...);
> for_each_chunk {
> struct sgx_enclave_add ioctlargs = {
> .offset = chunk.offset,
> .source = chunk.addr,
> .size = chunk.size,
> .type = chunk.type, /* SGX specific metadata */
> }
> ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
> }
> ioctl(enclave_fd, ENCLAVE_INIT, ...);
>
> write(sock, enclave_fd);
>
>
> But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
> takes the virtual address of the enclave, but in the above flow that's
> not established until "mmap(..., enclave_fd)". And because an enclave's
> virtual range needs to be naturally aligned (hardware requirements), the
> enclave owner would need to do something like:
>
> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> size =
>
> enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
> enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);
>
> connect(builder, ...);
> send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});
>
> recv(builder, _fd);
>
> munmap(enclave_range);
>
> for_each_chunk {
> addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED,
> enclave_fd, 0);
> if (addr != enclave_addr + c.offset)
>exit(1);
> }
>
> And that straight up doesn't work with the v20 driver because mmap() with
> the enclave_fd will run through sgx_get_unmapped_area(), which also does
> the natural alignment adjustments (the idea being that mmap() is mapping
> the entire enclave). E.g. mmap() will map the wrong address if the offset
> of a chunk is less than its size due to the driver adjusting the address.
That presumably needs to change.
Are we entirely missing an API to allocate a naturally aligned VA
range? That's kind of annoying.
>
> Eliminating sgx_get_unmapped_area() means userspace is once again on the
> hook for naturally aligning the enclave, which is less than desirable.
>
> Looking back at the original API discussions around a builder process[1],
> we never fleshed out the end-to-end flow. While having a builder process
> *sounds* reasonable, in practice it adds a lot of complexity without
> providing much in the way of added security. E.g. in addition to the
> above mmap() issues, since the order of EADDs affects the enclave
> measurement, the enclave owner would need to communicate the exact steps
> to build the enclave, or the builder would need a priori knowledge of the
> enclave format.
>
> Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
> daemon that requires additional credentials to obtain a new enclave_fd.
> So AFAICT, the only
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 09:56:30AM -0400, Stephen Smalley wrote:
> On 5/22/19 9:22 AM, Jarkko Sakkinen wrote:
> >On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote:
> >>On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote:
> >>>Except that mmap() is more or less required to guarantee that ELRANGE
> >>>established by ECREATE is available. And we want to disallow mmap() as
> >>>soon as the first EADD is done so that userspace can't remap the enclave's
> >>>VMAs via munmap()->mmap() and gain execute permissions to pages that were
> >>>EADD'd as NX.
> >>
> >>We don't want to guarantee such thing and it is not guaranteed. It does
> >>not fit at all to the multi process work done. Enclaves are detached
> >>from any particular process addresse spaces. It is responsibility of
> >>process to open windows to them.
> >>
> >>That would be completely against work that we've done lately.
> >
> >Example use case: you have a process that just constructs an enclave
> >and sends it to another process or processes for use. The constructor
> >process could have basically anything on that range. This was the key
> >goal of the fd based enclave work.
>
> What exactly happens in the constructor versus the recipient processes?
> Which process performs each of the necessary open(), mmap(), and ioctl()
> calls for setting up the enclave? Can you provide a high level overview of
> the sequence of userspace calls by the constructor and by the recipient
> similar to what Sean showed earlier for just a single process?
Hmm, what we had talked about was allowing the SGX ioctls to work without
an associated VMA, with the end goal of letting userspace restrict access
to /dev/sgx/enclave. Very roughly...
Enclave Owner:
connect(builder, ...);
send(builder, "/home/sean/path/to/my/enclave");
recv(builder, _fd);
for_each_chunk {
mmap(enclave_addr + offset, size, ..., MAP_SHARED, enclave_fd, 0);
}
Enclave Builder:
recv(sock, _path);
source_fd = open(enclave_path, O_RDONLY);
for_each_chunk {
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR);
ioctl(enclave_fd, ENCLAVE_CREATE, ...);
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
}
ioctl(enclave_fd, ENCLAVE_INIT, ...);
write(sock, enclave_fd);
But the above flow is flawed because there'a catch-22: ENCLAVE_ECREATE
takes the virtual address of the enclave, but in the above flow that's
not established until "mmap(..., enclave_fd)". And because an enclave's
virtual range needs to be naturally aligned (hardware requirements), the
enclave owner would need to do something like:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
size =
enclave_range = mmap(NULL, size*2, PROT_READ, ???, NULL, 0);
enclave_addr = (enclave_range + (size - 1)) & ~(size - 1);
connect(builder, ...);
send(builder, {"/home/sean/path/to/my/enclave", enclave_addr});
recv(builder, _fd);
munmap(enclave_range);
for_each_chunk {
addr = mmap(enclave_addr + c.offset, c.size, ..., MAP_SHARED, enclave_fd,
0);
if (addr != enclave_addr + c.offset)
exit(1);
}
And that straight up doesn't work with the v20 driver because mmap() with
the enclave_fd will run through sgx_get_unmapped_area(), which also does
the natural alignment adjustments (the idea being that mmap() is mapping
the entire enclave). E.g. mmap() will map the wrong address if the offset
of a chunk is less than its size due to the driver adjusting the address.
Eliminating sgx_get_unmapped_area() means userspace is once again on the
hook for naturally aligning the enclave, which is less than desirable.
Looking back at the original API discussions around a builder process[1],
we never fleshed out the end-to-end flow. While having a builder process
*sounds* reasonable, in practice it adds a lot of complexity without
providing much in the way of added security. E.g. in addition to the
above mmap() issues, since the order of EADDs affects the enclave
measurement, the enclave owner would need to communicate the exact steps
to build the enclave, or the builder would need a priori knowledge of the
enclave format.
Userspace can still restrict access to /dev/sgx/enclave, e.g. by having a
daemon that requires additional credentials to obtain a new enclave_fd.
So AFAICT, the only benefit to having a dedicated builder is that it can
do its own whitelisting of enclaves, but since we're trending towards
supporting whitelisting enclaves in the kernel, e.g. via sigstruct,
whitelisting in userspace purely in userspace also provides marginal value.
TL;DR: Requiring VMA backing to build an enclave seems reasonable and sane.
[1]
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/22/19 9:22 AM, Jarkko Sakkinen wrote: On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote: On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote: Except that mmap() is more or less required to guarantee that ELRANGE established by ECREATE is available. And we want to disallow mmap() as soon as the first EADD is done so that userspace can't remap the enclave's VMAs via munmap()->mmap() and gain execute permissions to pages that were EADD'd as NX. We don't want to guarantee such thing and it is not guaranteed. It does not fit at all to the multi process work done. Enclaves are detached from any particular process addresse spaces. It is responsibility of process to open windows to them. That would be completely against work that we've done lately. Example use case: you have a process that just constructs an enclave and sends it to another process or processes for use. The constructor process could have basically anything on that range. This was the key goal of the fd based enclave work. What exactly happens in the constructor versus the recipient processes? Which process performs each of the necessary open(), mmap(), and ioctl() calls for setting up the enclave? Can you provide a high level overview of the sequence of userspace calls by the constructor and by the recipient similar to what Sean showed earlier for just a single process?
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Wed, May 22, 2019 at 04:20:22PM +0300, Jarkko Sakkinen wrote: > On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote: > > Except that mmap() is more or less required to guarantee that ELRANGE > > established by ECREATE is available. And we want to disallow mmap() as > > soon as the first EADD is done so that userspace can't remap the enclave's > > VMAs via munmap()->mmap() and gain execute permissions to pages that were > > EADD'd as NX. > > We don't want to guarantee such thing and it is not guaranteed. It does > not fit at all to the multi process work done. Enclaves are detached > from any particular process addresse spaces. It is responsibility of > process to open windows to them. > > That would be completely against work that we've done lately. Example use case: you have a process that just constructs an enclave and sends it to another process or processes for use. The constructor process could have basically anything on that range. This was the key goal of the fd based enclave work. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Tue, May 21, 2019 at 08:51:40AM -0700, Sean Christopherson wrote: > Except that mmap() is more or less required to guarantee that ELRANGE > established by ECREATE is available. And we want to disallow mmap() as > soon as the first EADD is done so that userspace can't remap the enclave's > VMAs via munmap()->mmap() and gain execute permissions to pages that were > EADD'd as NX. We don't want to guarantee such thing and it is not guaranteed. It does not fit at all to the multi process work done. Enclaves are detached from any particular process addresse spaces. It is responsibility of process to open windows to them. That would be completely against work that we've done lately. > Actually, conceptually it's probably more intuitive to disallow mmap() at > ECREATE, i.e. the act of creating an enclave pins the associated virtual > address range until the enclave is destroyed. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Tue, May 21, 2019 at 03:24:18PM +, Jethro Beekman wrote: > On 2019-05-21 08:19, Jarkko Sakkinen wrote: > > We could even disallow mmap() before EINIT done. > This would be extremely annoying in software because now you have to save > the all the page permissions somewhere between EADD and mprotect. Actually you don't have to use mprotect anymore that much. You can just do multiple mmap's even with v20 after EINIT, one for each region (albeit it does not enforce above). /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Tue, May 21, 2019 at 06:19:37PM +0300, Jarkko Sakkinen wrote: > On Mon, May 20, 2019 at 02:41:05PM +0300, Jarkko Sakkinen wrote: > > On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote: > > > Is userspace actually requred to mmap() the enclave prior to EADDing > > > things? > > > > Nope, not since v20. Here is what I wrote about API to the kernel > > documentation: > > > > "The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this > > there is already a data structure inside kernel tracking the enclave > > that is initially uncreated. After this a set of ioctl's can be used to > > create, populate and initialize the enclave. > > > > You can close (if you want) the fd after you've mmap()'d. As long as the > > file is open the enclave stays alive so you might want to do that after > > you don't need it anymore. Even munmap() won't destruct the enclave if > > the file is open. Neither will closing the fd as long as you have > > mmap() done over the fd (even if it does not across the range defined in > > SECS)." > > > > Enclave can be created and initialized without doing a single mmap() > > call. > > We could even disallow mmap() before EINIT done. The way enclave > management internally works right now is quite robust and completely > detached from requiring process address space for anything. Except that mmap() is more or less required to guarantee that ELRANGE established by ECREATE is available. And we want to disallow mmap() as soon as the first EADD is done so that userspace can't remap the enclave's VMAs via munmap()->mmap() and gain execute permissions to pages that were EADD'd as NX. Actually, conceptually it's probably more intuitive to disallow mmap() at ECREATE, i.e. the act of creating an enclave pins the associated virtual address range until the enclave is destroyed.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 2019-05-21 08:19, Jarkko Sakkinen wrote: We could even disallow mmap() before EINIT done. This would be extremely annoying in software because now you have to save the all the page permissions somewhere between EADD and mprotect. -- Jethro Beekman | Fortanix smime.p7s Description: S/MIME Cryptographic Signature
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Mon, May 20, 2019 at 02:41:05PM +0300, Jarkko Sakkinen wrote: > On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote: > > Is userspace actually requred to mmap() the enclave prior to EADDing things? > > Nope, not since v20. Here is what I wrote about API to the kernel > documentation: > > "The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this > there is already a data structure inside kernel tracking the enclave > that is initially uncreated. After this a set of ioctl's can be used to > create, populate and initialize the enclave. > > You can close (if you want) the fd after you've mmap()'d. As long as the > file is open the enclave stays alive so you might want to do that after > you don't need it anymore. Even munmap() won't destruct the enclave if > the file is open. Neither will closing the fd as long as you have > mmap() done over the fd (even if it does not across the range defined in > SECS)." > > Enclave can be created and initialized without doing a single mmap() > call. We could even disallow mmap() before EINIT done. The way enclave management internally works right now is quite robust and completely detached from requiring process address space for anything. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 08:41:28AM -0700, Sean Christopherson wrote: > It was a requirement prior to the API rework in v20, i.e. unless someone > was really quick on the draw after the v20 update all existing userspace > implementations mmap() the enclave before ECREATE. Requiring a valid > enclave VMA for EADD shoudn't be too onerous. Still underlining: it is not required. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote: > Is userspace actually requred to mmap() the enclave prior to EADDing things? Nope, not since v20. Here is what I wrote about API to the kernel documentation: "The enclave life-cycle starts by opening `/dev/sgx/enclave`. After this there is already a data structure inside kernel tracking the enclave that is initially uncreated. After this a set of ioctl's can be used to create, populate and initialize the enclave. You can close (if you want) the fd after you've mmap()'d. As long as the file is open the enclave stays alive so you might want to do that after you don't need it anymore. Even munmap() won't destruct the enclave if the file is open. Neither will closing the fd as long as you have mmap() done over the fd (even if it does not across the range defined in SECS)." Enclave can be created and initialized without doing a single mmap() call. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 16, 2019 at 05:03:31PM -0700, Sean Christopherson wrote: > The SGX ioctl() would need to take mmap_sem for write, but we can mitigate > that issue by changing the ioctl() to take a range of memory instead of a > single page. That'd also provide "EADD batching" that folks have > requested. This should be easy enough to add as the EADD operations are already batched internally to a worker thread. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote: > That certainly *could* be done, and I guess the decision could be left > to the LSMs, but I'm not convinced this adds value. What security use > case does this cover that isn't already covered by requiring EXECUTE > (e.g. lib_t) on the enclave file and some new SIGSTRUCT right on the > .sigstruct? I guess you are right as SIGSTRUCT completely shields the memory layout and contents of an enclave. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 16, 2019 at 03:45:50PM -0700, Sean Christopherson wrote: > On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote: > > > On May 15, 2019, at 10:16 PM, Jarkko Sakkinen > > > wrote: > > > There is a problem here though. Usually the enclave itself is just a > > > loader that then loads the application from outside source and creates > > > the executable pages from the content. > > > > > > A great example of this is Graphene that bootstraps unmodified Linux > > > applications to an enclave: > > > > > > https://github.com/oscarlab/graphene > > > > > > > ISTM you should need EXECMEM or similar to run Graphene, then. > > Agreed, Graphene is effectively running arbitrary enclave code. I'm > guessing there is nothing that prevents extending/reworking Graphene to > allow generating the enclave ahead of time so as to avoid populating the > guts of the enclave at runtime, i.e. it's likely possible to run an > unmodified application in an enclave without EXECMEM if that's something > Graphene or its users really care about. I'd guess that also people adding SGX support to containers want somewhat similar framework to work on so that you can just wrap a container with an enclave. /Jarkko
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Thu, May 16, 2019 at 05:24:33PM +1000, James Morris wrote: Good morning, I hope everyone had a pleasant weekend. James, I believe the last time our paths crossed was at the Linux Security Summit in Seattle, I trust you have been well since then. > On Wed, 15 May 2019, Andy Lutomirski wrote: > > > On Wed, May 15, 2019 at 3:46 PM James Morris wrote: > > > > > > You could try user.sigstruct, which does not require any privs. > > > > > > > I don't think I understand your proposal. What file would this > > attribute be on? What would consume it? > It would be on the enclave file, so you keep the sigstruct bound to > it, rather than needing a separate file to manage. It would > simplify any LSM policy check. > > It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your > example, instead of having a 2nd fd. I've watched this discussion regarding LSM, sigstructs and file descriptors with some fascination, since all of this infrastructure already exists and should be well understood by anyone who has been active in SGX runtime development. There would thus seem to be a disconnect between SGX driver developers and the consumers of the services of the driver. The existing enclave format, codified by the silo within Intel that is responsible for the existing SDK/PSW, implements a notes section stored inside a standard ELF shared library image. The notes section contains a significant amount of metadata that is used to direct the instantiation of what will be the initialized enclave image. Said metadata includes a copy of the sigstruct that was generated when the enclave was signed, which is the event that triggers metadata generation. All of this means that any enclave that gets loaded effectively triggers both LSM and IMA checks. James, if you remember, the paper that we presented in Seattle described the initial implementation of an extension to the Linux IMA infrastructure that tracks whether or not processes can be 'trusted'. That work has gone on to include running the trust modeling and disciplining engine inside of a namespace specific SGX enclave. We would be happy to make available execution trajectory logs that clearly document IMA and LSM checks being conducted on enclaves. There is a strong probability that we will be maintaining and supporting a modified version of whatever driver that goes upstream. In support of this we are putting together a white paper discussing security architecture concerns inherent in an SGX driver. With the intent of avoiding LKML verbosity we will post a URL to the paper when it is available if there is interest. The issue of EDMM has already come up, suffice it to say that EDMM makes LSM inspection of enclave content, while desirable, largely irrelevant from a security perspective. > James Morris Best wishes for a productive week. Dr. Greg As always, Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC. 4206 N. 19th Ave. Specializing in information infra-structure Fargo, ND 58102development. PH: 701-281-1686EMAIL: g...@enjellic.com -- "If you plugged up your nose and mouth right before you sneezed, would the sneeze go out your ears or would your head explode? Either way I'm afraid to try." -- Nick Kean
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 04:09:22PM -0400, Stephen Smalley wrote:
> On 5/17/19 3:28 PM, Sean Christopherson wrote:
> >On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> >Yep, and that's by design in the overall proposal. The trick is that
> >ENCLAVE_ADD takes a source VMA and copies the contents *and* the
> >permissions from the source VMA. The source VMA points at regular memory
> >that was mapped and populated using existing mechanisms for loading DSOs.
> >
> >E.g. at a high level:
> >
> >source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
> >for_each_chunk {
> > > memory>
> >}
> >
> >enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
> >enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
> >
> >ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
> >for_each_chunk {
> > struct sgx_enclave_add ioctlargs = {
> > .offset = chunk.offset,
> > .source = chunk.addr,
> > .size = chunk.size,
> > .type = chunk.type, /* SGX specific metadata */
> > }
> > ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
> >}
> >ioctl(fd, ENCLAVE_INIT, ...);
> >
> >
> >Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
> >ensures userspace isn't bypassing LSM policies by virtue of copying the
> >permissions for EPC VMAs from regular VMAs that have already gone through
> >LSM checks.
>
> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you
> do anything other than ioctl() calls on it?
Hmm, in the current implementation, yes, O_RDWR is required. An enclave
and its associated EPC memory are represented and referenced by its fd,
which is backed by /dev/sgx/enclave. An enclave is not just code, e.g.
also has a heap, stack, variables, etc..., which need to be mapped
accordingly. In the current implementation, userspace directly does
mprotect() or mmap() on EPC VMAs, and so setting PROT_WRITE for the heap
and whatnot requires opening /dev/sgx/enclave with O_RDWR.
I *think* /dev/sgx/enclave could be opened O_RDONLY if ENCLAVE_ADD stuffed
the EPC VMA permissions, assuming the use case doesn't require changing
permissions after the enclave has been created.
The other reason userspace would need to open /dev/sgx/enclave O_RDWR
would be to debug an enclave, e.g. pwrite() works on the enclave fd due
to SGX restrictions on modifying EPC memory from outside the enclave.
But that's an obvious case where FILE__WRITE should be required.
> What's the advantage of allocating an anon inode in the above? At present
> anon inodes are exempted from inode-based checking, thereby losing the
> ability to perform SELinux ioctl whitelisting, unlike the file-backed
> /dev/sgx/enclave inode.
Purely to trigger the EXECMEM check on any PROT_EXEC mapping. However,
the motiviation for that was due to my bad assumption that FILE__WRITE
and FILE__EXECUTE are global and not per process. If we can do as you
suggest and allow creation of enclaves with O_RDONLY, then keeping a
file-backed inode is definitely better as it means most processes only
need FILE__READ and FILE__* in general has actual meaning.
Thanks a bunch for your help!
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 4:14 PM, Andy Lutomirski wrote:
On May 17, 2019, at 1:09 PM, Stephen Smalley wrote:
On 5/17/19 3:28 PM, Sean Christopherson wrote:
On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
How can that work? Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave. Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.
I'm not sure what the API is. Let's say they do something like this:
fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, );
ioctl(fd, ENCLAVE_ADD_PAGE, );
ioctl(fd, ENCLAVE_INIT, );
That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.
The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.
If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.
Yep, and that's by design in the overall proposal. The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA. The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.
Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you do
anything other than ioctl() calls on it?
What's the advantage of allocating an anon inode in the above? At present anon
inodes are exempted from inode-based checking, thereby losing the ability to
perform SELinux ioctl whitelisting, unlike the file-backed /dev/sgx/enclave
inode.
How would SELinux (or other security modules) restrict the authorized enclaves
that can be loaded via this interface? Would the sgx driver invoke a new LSM
hook with the regular/source VMAs as parameters and allow the security module
to reject the ENCLAVE_ADD operation? That could be just based on the vm_file
(e.g. whitelist what enclave files are permitted in general) or it could be
based on both the process and the vm_file (e.g. only allow specific enclaves to
be loaded into specific processes).
This is the idea behind the .sigstruct file. The driver could call a new hook
to approve or reject the .sigstruct. The sigstruct contains a hash of the whole
enclave and a signature by the author.
Ok, so same idea but moved to ENCLAVE_INIT and passing the vma or file
for the sigstruct instead of the enclave.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 17, 2019, at 1:09 PM, Stephen Smalley wrote:
>
>> On 5/17/19 3:28 PM, Sean Christopherson wrote:
>>> On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
How can that work? Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave. Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.
>>>
>>> I'm not sure what the API is. Let's say they do something like this:
>>>
>>> fd = open("/dev/sgx/enclave", O_RDONLY);
>>> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
>>> stuff addr into ioctl args
>>> ioctl(fd, ENCLAVE_CREATE, );
>>> ioctl(fd, ENCLAVE_ADD_PAGE, );
>>> ioctl(fd, ENCLAVE_INIT, );
>> That's rougly the flow, except that that all enclaves need to have RW and
>> X EPC pages.
>>> The important points are that they do not open /dev/sgx/enclave with write
>>> access (otherwise they will trigger FILE__WRITE at open time, and later
>>> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
>>> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
>>> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
>>> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
>>> policy.
>>>
>>> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
>>> will trigger an EXECMEM check, at least as currently implemented, as we have
>>> no useful backing inode information.
>> Yep, and that's by design in the overall proposal. The trick is that
>> ENCLAVE_ADD takes a source VMA and copies the contents *and* the
>> permissions from the source VMA. The source VMA points at regular memory
>> that was mapped and populated using existing mechanisms for loading DSOs.
>> E.g. at a high level:
>> source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
>> for_each_chunk {
>> > memory>
>> }
>> enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
>> enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
>> ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
>> for_each_chunk {
>> struct sgx_enclave_add ioctlargs = {
>> .offset = chunk.offset,
>> .source = chunk.addr,
>> .size = chunk.size,
>> .type = chunk.type, /* SGX specific metadata */
>> }
>> ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
>> }
>> ioctl(fd, ENCLAVE_INIT, ...);
>> Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
>> ensures userspace isn't bypassing LSM policies by virtue of copying the
>> permissions for EPC VMAs from regular VMAs that have already gone through
>> LSM checks.
>
> Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do you do
> anything other than ioctl() calls on it?
>
> What's the advantage of allocating an anon inode in the above? At present
> anon inodes are exempted from inode-based checking, thereby losing the
> ability to perform SELinux ioctl whitelisting, unlike the file-backed
> /dev/sgx/enclave inode.
>
> How would SELinux (or other security modules) restrict the authorized
> enclaves that can be loaded via this interface? Would the sgx driver invoke
> a new LSM hook with the regular/source VMAs as parameters and allow the
> security module to reject the ENCLAVE_ADD operation? That could be just
> based on the vm_file (e.g. whitelist what enclave files are permitted in
> general) or it could be based on both the process and the vm_file (e.g. only
> allow specific enclaves to be loaded into specific processes).
This is the idea behind the .sigstruct file. The driver could call a new hook
to approve or reject the .sigstruct. The sigstruct contains a hash of the whole
enclave and a signature by the author.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 3:28 PM, Sean Christopherson wrote:
On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
How can that work? Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave. Some of it will
be written only from already executable pages, and some privilege should be
needed to execute any enclave page that was not loaded like this.
I'm not sure what the API is. Let's say they do something like this:
fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, );
ioctl(fd, ENCLAVE_ADD_PAGE, );
ioctl(fd, ENCLAVE_INIT, );
That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.
The important points are that they do not open /dev/sgx/enclave with write
access (otherwise they will trigger FILE__WRITE at open time, and later
encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
policy.
If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
will trigger an EXECMEM check, at least as currently implemented, as we have
no useful backing inode information.
Yep, and that's by design in the overall proposal. The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA. The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.
Is O_RDWR required for /dev/sgx/enclave or would O_RDONLY suffice? Do
you do anything other than ioctl() calls on it?
What's the advantage of allocating an anon inode in the above? At
present anon inodes are exempted from inode-based checking, thereby
losing the ability to perform SELinux ioctl whitelisting, unlike the
file-backed /dev/sgx/enclave inode.
How would SELinux (or other security modules) restrict the authorized
enclaves that can be loaded via this interface? Would the sgx driver
invoke a new LSM hook with the regular/source VMAs as parameters and
allow the security module to reject the ENCLAVE_ADD operation? That
could be just based on the vm_file (e.g. whitelist what enclave files
are permitted in general) or it could be based on both the process and
the vm_file (e.g. only allow specific enclaves to be loaded into
specific processes).
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 02:05:39PM -0400, Stephen Smalley wrote:
> On 5/17/19 1:12 PM, Andy Lutomirski wrote:
> >
> >How can that work? Unless the API changes fairly radically, users
> >fundamentally need to both write and execute the enclave. Some of it will
> >be written only from already executable pages, and some privilege should be
> >needed to execute any enclave page that was not loaded like this.
>
> I'm not sure what the API is. Let's say they do something like this:
>
> fd = open("/dev/sgx/enclave", O_RDONLY);
> addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
> stuff addr into ioctl args
> ioctl(fd, ENCLAVE_CREATE, );
> ioctl(fd, ENCLAVE_ADD_PAGE, );
> ioctl(fd, ENCLAVE_INIT, );
That's rougly the flow, except that that all enclaves need to have RW and
X EPC pages.
> The important points are that they do not open /dev/sgx/enclave with write
> access (otherwise they will trigger FILE__WRITE at open time, and later
> encounter FILE__EXECUTE as well during mmap, thereby requiring both to be
> allowed to /dev/sgx/enclave), and that they do not request PROT_WRITE to the
> resulting mapping (otherwise they will trigger FILE__WRITE at mmap time).
> Then only FILE__READ and FILE__EXECUTE are required to /dev/sgx/enclave in
> policy.
>
> If they switch to an anon inode, then any mmap PROT_EXEC of the opened file
> will trigger an EXECMEM check, at least as currently implemented, as we have
> no useful backing inode information.
Yep, and that's by design in the overall proposal. The trick is that
ENCLAVE_ADD takes a source VMA and copies the contents *and* the
permissions from the source VMA. The source VMA points at regular memory
that was mapped and populated using existing mechanisms for loading DSOs.
E.g. at a high level:
source_fd = open("/home/sean/path/to/my/enclave", O_RDONLY);
for_each_chunk {
}
enclave_fd = open("/dev/sgx/enclave", O_RDWR); /* allocs anon inode */
enclave_addr = mmap(NULL, size, PROT_READ, MAP_SHARED, enclave_fd, 0);
ioctl(enclave_fd, ENCLAVE_CREATE, {enclave_addr});
for_each_chunk {
struct sgx_enclave_add ioctlargs = {
.offset = chunk.offset,
.source = chunk.addr,
.size = chunk.size,
.type = chunk.type, /* SGX specific metadata */
}
ioctl(fd, ENCLAVE_ADD, ); /* modifies enclave's VMAs */
}
ioctl(fd, ENCLAVE_INIT, ...);
Userspace never explicitly requests PROT_EXEC on enclave_fd, but SGX also
ensures userspace isn't bypassing LSM policies by virtue of copying the
permissions for EPC VMAs from regular VMAs that have already gone through
LSM checks.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 2:05 PM, Stephen Smalley wrote:
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
On May 17, 2019, at 9:37 AM, Stephen Smalley wrote:
On 5/17/19 12:20 PM, Stephen Smalley wrote:
On 5/17/19 11:09 AM, Sean Christopherson wrote:
On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by
them) but
I was probably wrong. It sounds like EXECMOD applies to the whole
process so
would allow all pages within a process's address space to be
modified then
executed, regardless the backing files. Am I correct this time?
No, you were correct the first time I think; EXECMOD is used to
control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special
case to
support text relocations without having to allow full EXECMEM
(i.e. execute
arbitrary memory).
SELinux checks relevant to W^X include:
- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping
(regardless of
PROT_WRITE, since we know the content has to have been written at
some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
(ignoring EXECSTACK and EXECHEAP here since they aren't really
relevant to
this discussion)
So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination
of both
FILE__WRITE and FILE__EXECUTE by the process to any file.
If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't
using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.
Yep, I was just typing this up in a different thread:
I think we may want to change the SGX API to alloc an anon inode
for each
enclave instead of hanging every enclave off of the
/dev/sgx/enclave inode.
Because /dev/sgx/enclave is NOT private, SELinux's
file_map_prot_check()
will only require FILE__WRITE and FILE__EXECUTE to mprotect()
enclave VMAs
to RWX. Backing each enclave with an anon inode will make SELinux
treat
EPC memory like anonymous mappings, which is what we want (I
think), e.g.
making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
64-bit only at this point, so SELinux will always have
default_noexec).
I don't think we want to require EXECMEM (or equivalently both
FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any
EPC page executable, only if the page is also writable or previously
modified. The intent is to prevent arbitrary code execution without
EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing
enclaves to be created without EXECMEM as long as the EPC page
mapping is only ever mapped RX and its initial contents came from an
unmodified file mapping that was PROT_EXEC (and hence already
checked via FILE__EXECUTE).
Also, just to be clear, there is nothing inherently better about
checking EXECMEM instead of checking both FILE__WRITE and
FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to
using anon inodes for that reason. Using anon inodes also
unfortunately disables SELinux inode-based checking since we no
longer have any useful inode information, so you'd lose out on
SELinux ioctl whitelisting on those enclave inodes if that matters.
How can that work? Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave. Some of it
will be written only from already executable pages, and some privilege
should be needed to execute any enclave page that was not loaded like
this.
I'm not sure what the API is. Let's say they do something like this:
fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, );
ioctl(fd, ENCLAVE_ADD_PAGE, );
ioctl(fd, ENCLAVE_INIT, );
The important points are that they do not open /dev/sgx/enclave with
write access (otherwise they will trigger FILE__WRITE at open time, and
later encounter FILE__EXECUTE as well during mmap, thereby requiring
both to be allowed to /dev/sgx/enclave), and that they do not request
PROT_WRITE to the resulting mapping (otherwise they will trigger
FILE__WRITE at mmap time). Then only FILE__READ and FILE__EXECUTE are
required to /dev/sgx/enclave in policy.
If they switch to an anon inode, then any mmap PROT_EXEC of the opened
file will trigger an EXECMEM check, at least as currently implemented,
as we have no useful backing inode information.
FWIW, looking at the selftest for SGX in the patch series, they open
/dev/sgx/enclave O_RDWR (probably not necessary?) and mmap the open file
RWX. If that is
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 17, 2019, at 11:21 AM, Sean Christopherson > wrote: > >> On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote: >> On Fri, May 17, 2019 at 10:55 AM Sean Christopherson >> wrote: >>> >>> In this snippet, IS_PRIVATE() is true for anon inodes, false for >>> /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never >>> check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave. >> >> Why _does_ the memory have to be shared? Shared mmap() is >> fundamentally less secure than private mmap, since by definition it >> means "oh, somebody else has access to it too and might modify it >> under us". >> >> Why does the SGX logic care about things like that? Normal executables >> are just private mappings of an underlying file, I'm not sure why the >> SGX interface has to have that shared thing, and why the interface has >> to have a device node in the first place when you have system calls >> for setup anyway. >> >> So why don't the system calls just work on perfectly normal anonymous >> mmap's? Why a device node, and why must it be shared to begin with? > > I agree that conceptually EPC is private memory, but because EPC is > managed as a separate memory pool, SGX tags it VM_PFNMAP and manually > inserts PFNs, i.e. EPC effectively it gets classified as IO memory. > > And vmf_insert_pfn_prot() doesn't like writable private IO mappings: > > BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags)); I don’t see how it could be anonymous even in principle. The kernel can’t *read* the memory — how could we possibly CoW it? And we can’t share an RO backing pages between two different enclaves because the CPU won’t let us — each EPC page belongs to a particular enclave. And fork()ing an enclave is right out. So I agree that MAP_ANONYMOUS would be nice conceptually, but I don’t see how it would work.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 11:33:30AM -0700, Linus Torvalds wrote: > On Fri, May 17, 2019 at 11:21 AM Sean Christopherson > wrote: > > > > I agree that conceptually EPC is private memory, but because EPC is > > managed as a separate memory pool, SGX tags it VM_PFNMAP and manually > > inserts PFNs, i.e. EPC effectively it gets classified as IO memory. > > > > And vmf_insert_pfn_prot() doesn't like writable private IO mappings: > > > >BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags)); > > Hmm. I haven't looked into why you want to do your own page insertion > and not just "use existing pages", but I'm sure there's some reason. Outside of the SGX subsystem, the kernel is unaware of EPC memory, e.g. BIOS enumerates it as reserved memory in the e820 tables, or not at all. On current hardware, EPC is backed by system memory, but it's protected by a range registers (and other stuff) and can't be accessed directly except when the CPU is in "enclave mode", i.e. executing an enclave in CPL3. To execute an enclave it must first be built, and because EPC memory can't be written outside of enclave mode, the only way to build the enclave is via dedicated CPL0 ISA, e.g. ENCLS[EADD]. > It looks like the "shared vs private" inode part is a red herring, > though. You might as well give each opener of the sgx node its own > inode - and you probably should. Then you can keep track of the pages > that have been added in the inode->i_mapping, and you could avoid the > whole PFN thing entirely. I still am not a huge fan of the device node > in the first place, but I guess it's just one more place where a > system admin can then give (or deny) access to a kernel feature from > users. I guess the kvm people do the same thing, for not necessarily > any better reasons. > > With the PFNMAP model I guess the SGX memory ends up being unswappable > - at least done the obvious way. EPC memory is swappable in it's own terms, e.g. pages can be swapped from EPC to system RAM and vice versa, but again moving pages in and out of the EPC can only be done through dedicated CPL0 ISA. And there are additional TLB flushing requirements, evicted pages need to be refcounted against the enclave, evicted pages need an anchor in the EPC to ensure freshness, etc... Long story short, we decided to manage EPC in the SGX subsystem as a separate memory pool rather than modify the kernel's MMU to teach it how to deal with EPC. > Again, the way I'd expect it to be done is as a shmem inode - that > would I think be a better model. But I think that's a largely internal > design decision, and the device node could just do that eventually > (and the mmap could just map the populated shmem information into > memory, no PFNMAP needed - the inode and the mapping could be > "read-only" as far as the _user_ is concerned, but the i_mapping then > gets populated by the ioctl's). > > I have not actually looked at any of the SGX patches, so maybe you're > already doing something like that (although the PFNMAP comment makes > me think not), and quite possibly there's some fundamental reason why > you can't just use the shmem approach. > > So my high-level reaction here may be just the rantings of somebody > who just isn't familiar with what you do. My "why not shmem and > regular mmap" questions come from a 3ft view without knowing any > of the details. > >Linus
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 11:21 AM Sean Christopherson wrote: > > I agree that conceptually EPC is private memory, but because EPC is > managed as a separate memory pool, SGX tags it VM_PFNMAP and manually > inserts PFNs, i.e. EPC effectively it gets classified as IO memory. > > And vmf_insert_pfn_prot() doesn't like writable private IO mappings: > >BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags)); Hmm. I haven't looked into why you want to do your own page insertion and not just "use existing pages", but I'm sure there's some reason. It looks like the "shared vs private" inode part is a red herring, though. You might as well give each opener of the sgx node its own inode - and you probably should. Then you can keep track of the pages that have been added in the inode->i_mapping, and you could avoid the whole PFN thing entirely. I still am not a huge fan of the device node in the first place, but I guess it's just one more place where a system admin can then give (or deny) access to a kernel feature from users. I guess the kvm people do the same thing, for not necessarily any better reasons. With the PFNMAP model I guess the SGX memory ends up being unswappable - at least done the obvious way. Again, the way I'd expect it to be done is as a shmem inode - that would I think be a better model. But I think that's a largely internal design decision, and the device node could just do that eventually (and the mmap could just map the populated shmem information into memory, no PFNMAP needed - the inode and the mapping could be "read-only" as far as the _user_ is concerned, but the i_mapping then gets populated by the ioctl's). I have not actually looked at any of the SGX patches, so maybe you're already doing something like that (although the PFNMAP comment makes me think not), and quite possibly there's some fundamental reason why you can't just use the shmem approach. So my high-level reaction here may be just the rantings of somebody who just isn't familiar with what you do. My "why not shmem and regular mmap" questions come from a 3ft view without knowing any of the details. Linus
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 11:04:22AM -0700, Linus Torvalds wrote: > On Fri, May 17, 2019 at 10:55 AM Sean Christopherson > wrote: > > > > In this snippet, IS_PRIVATE() is true for anon inodes, false for > > /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never > > check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave. > > Why _does_ the memory have to be shared? Shared mmap() is > fundamentally less secure than private mmap, since by definition it > means "oh, somebody else has access to it too and might modify it > under us". > > Why does the SGX logic care about things like that? Normal executables > are just private mappings of an underlying file, I'm not sure why the > SGX interface has to have that shared thing, and why the interface has > to have a device node in the first place when you have system calls > for setup anyway. > > So why don't the system calls just work on perfectly normal anonymous > mmap's? Why a device node, and why must it be shared to begin with? I agree that conceptually EPC is private memory, but because EPC is managed as a separate memory pool, SGX tags it VM_PFNMAP and manually inserts PFNs, i.e. EPC effectively it gets classified as IO memory. And vmf_insert_pfn_prot() doesn't like writable private IO mappings: BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 1:50 PM, Sean Christopherson wrote: On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote: On 5/17/19 1:29 PM, Sean Christopherson wrote: AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus PROCESS__EXECMEM achieves per-process granularity. No, FILE__WRITE and FILE__EXECUTE are a check between a process and a file, so you can ensure that only whitelisted processes are allowed both to /dev/sgx/enclave. Ah, so each process has its own FILE__* permissions for a specific set of files? That's correct. Does that allow differentiating between a process making an EPC page RWX and a process making two separate EPC pages RW and RX? Not if they are backed by the same inode, nor if they are all backed by anon inodes, at least not as currently implemented.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 10:55 AM Sean Christopherson wrote: > > In this snippet, IS_PRIVATE() is true for anon inodes, false for > /dev/sgx/enclave. Because EPC memory is always shared, SELinux will never > check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave. Why _does_ the memory have to be shared? Shared mmap() is fundamentally less secure than private mmap, since by definition it means "oh, somebody else has access to it too and might modify it under us". Why does the SGX logic care about things like that? Normal executables are just private mappings of an underlying file, I'm not sure why the SGX interface has to have that shared thing, and why the interface has to have a device node in the first place when you have system calls for setup anyway. So why don't the system calls just work on perfectly normal anonymous mmap's? Why a device node, and why must it be shared to begin with? Linus
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 1:12 PM, Andy Lutomirski wrote:
On May 17, 2019, at 9:37 AM, Stephen Smalley wrote:
On 5/17/19 12:20 PM, Stephen Smalley wrote:
On 5/17/19 11:09 AM, Sean Christopherson wrote:
On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
On 5/16/19 6:23 PM, Xing, Cedric wrote:
I thought EXECMOD applied to files (and memory mappings backed by them) but
I was probably wrong. It sounds like EXECMOD applies to the whole process so
would allow all pages within a process's address space to be modified then
executed, regardless the backing files. Am I correct this time?
No, you were correct the first time I think; EXECMOD is used to control
whether a process can make executable a private file mapping that has
previously been modified (e.g. text relocation); it is a special case to
support text relocations without having to allow full EXECMEM (i.e. execute
arbitrary memory).
SELinux checks relevant to W^X include:
- EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
PROT_WRITE, since we know the content has to have been written at some
point) or a private file mapping that is also PROT_WRITE.
- EXECMOD: mprotect PROT_EXEC a private file mapping that has been
previously modified, typically for text relocations,
- FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
- FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
(ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
this discussion)
So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
process, EXECMOD by the process to any file, and the combination of both
FILE__WRITE and FILE__EXECUTE by the process to any file.
If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
anonymous inode, then I would expect that only the FILE__WRITE and
FILE__EXECUTE checks are relevant.
Yep, I was just typing this up in a different thread:
I think we may want to change the SGX API to alloc an anon inode for each
enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
to RWX. Backing each enclave with an anon inode will make SELinux treat
EPC memory like anonymous mappings, which is what we want (I think), e.g.
making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
64-bit only at this point, so SELinux will always have default_noexec).
I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and
FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if
the page is also writable or previously modified. The intent is to prevent
arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while
still allowing enclaves to be created without EXECMEM as long as the EPC page
mapping is only ever mapped RX and its initial contents came from an unmodified
file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
Also, just to be clear, there is nothing inherently better about checking
EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
/dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
reason. Using anon inodes also unfortunately disables SELinux inode-based
checking since we no longer have any useful inode information, so you'd lose
out on SELinux ioctl whitelisting on those enclave inodes if that matters.
How can that work? Unless the API changes fairly radically, users
fundamentally need to both write and execute the enclave. Some of it will be
written only from already executable pages, and some privilege should be needed
to execute any enclave page that was not loaded like this.
I'm not sure what the API is. Let's say they do something like this:
fd = open("/dev/sgx/enclave", O_RDONLY);
addr = mmap(NULL, size, PROT_READ | PROT_EXEC, MAP_SHARED, fd, 0);
stuff addr into ioctl args
ioctl(fd, ENCLAVE_CREATE, );
ioctl(fd, ENCLAVE_ADD_PAGE, );
ioctl(fd, ENCLAVE_INIT, );
The important points are that they do not open /dev/sgx/enclave with
write access (otherwise they will trigger FILE__WRITE at open time, and
later encounter FILE__EXECUTE as well during mmap, thereby requiring
both to be allowed to /dev/sgx/enclave), and that they do not request
PROT_WRITE to the resulting mapping (otherwise they will trigger
FILE__WRITE at mmap time). Then only FILE__READ and FILE__EXECUTE are
required to /dev/sgx/enclave in policy.
If they switch to an anon inode, then any mmap PROT_EXEC of the opened
file will trigger an EXECMEM check, at least as currently implemented,
as we have no useful backing inode information.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 10:43:01AM -0700, Andy Lutomirski wrote:
>
> > On May 17, 2019, at 10:29 AM, Sean Christopherson
> > wrote:
> >
> > AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> > *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> > PROCESS__EXECMEM achieves per-process granularity.
>
> How does anon_inode make any difference? Anon_inode is not the same thing as
> anon_vma.
In this snippet, IS_PRIVATE() is true for anon inodes, false for
/dev/sgx/enclave. Because EPC memory is always shared, SELinux will never
check PROCESS__EXECMEM for mprotect() on/dev/sgx/enclave.
static int file_map_prot_check(struct file *file, unsigned long prot, int
shared)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int rc = 0;
if (default_noexec &&
(prot & PROT_EXEC) && (!file || IS_PRIVATE(file_inode(file)) ||
(!shared && (prot & PROT_WRITE {
/*
* We are making executable an anonymous mapping or a
* private file mapping that will also be writable.
* This has an additional check.
*/
rc = avc_has_perm(_state,
sid, sid, SECCLASS_PROCESS,
PROCESS__EXECMEM, NULL);
if (rc)
goto error;
}
...
}
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 01:42:50PM -0400, Stephen Smalley wrote: > On 5/17/19 1:29 PM, Sean Christopherson wrote: > >AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow > >*any* enclave/process to map EPC as RWX. Moving to anon inodes and thus > >PROCESS__EXECMEM achieves per-process granularity. > > > > No, FILE__WRITE and FILE__EXECUTE are a check between a process and a file, > so you can ensure that only whitelisted processes are allowed both to > /dev/sgx/enclave. Ah, so each process has its own FILE__* permissions for a specific set of files? Does that allow differentiating between a process making an EPC page RWX and a process making two separate EPC pages RW and RX?
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 17, 2019, at 10:29 AM, Sean Christopherson > wrote: > >> On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote: >>> On 5/17/19 12:20 PM, Stephen Smalley wrote: On 5/17/19 11:09 AM, Sean Christopherson wrote: I think we may want to change the SGX API to alloc an anon inode for each enclave instead of hanging every enclave off of the /dev/sgx/enclave inode. Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check() will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs to RWX. Backing each enclave with an anon inode will make SELinux treat EPC memory like anonymous mappings, which is what we want (I think), e.g. making *any* EPC page executable will require PROCESS__EXECMEM (SGX is 64-bit only at this point, so SELinux will always have default_noexec). >>> >>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE >>> and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, >>> only if the page is also writable or previously modified. The intent is >>> to prevent arbitrary code execution without EXECMEM (or >>> FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created >>> without EXECMEM as long as the EPC page mapping is only ever mapped RX and >>> its initial contents came from an unmodified file mapping that was >>> PROT_EXEC (and hence already checked via FILE__EXECUTE). > > The idea is that by providing an SGX ioctl() to propagate VMA permissions > from a source VMA, EXECMEM wouldn't be required to make an EPC page > executable. E.g. userspace establishes an enclave in non-EPC memory from > an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl() > to copy the contents and permissions into EPC memory. > >> Also, just to be clear, there is nothing inherently better about checking >> EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the >> /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that >> reason. Using anon inodes also unfortunately disables SELinux inode-based >> checking since we no longer have any useful inode information, so you'd lose >> out on SELinux ioctl whitelisting on those enclave inodes if that matters. > > The problem is that all enclaves are associated with a single inode, i.e. > /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to > provide ioctls() and to allow mmap()'ing EPC memory. In no way is it > associated with the content that actually gets loaded into EPC memory. > > The actual file that contains the enclave's contents (assuming the enclave > came from a file) is a separate regular file that the SGX subsystem never > sees. > > AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow > *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus > PROCESS__EXECMEM achieves per-process granularity. How does anon_inode make any difference? Anon_inode is not the same thing as anon_vma.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 1:29 PM, Sean Christopherson wrote: On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote: On 5/17/19 12:20 PM, Stephen Smalley wrote: On 5/17/19 11:09 AM, Sean Christopherson wrote: I think we may want to change the SGX API to alloc an anon inode for each enclave instead of hanging every enclave off of the /dev/sgx/enclave inode. Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check() will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs to RWX. Backing each enclave with an anon inode will make SELinux treat EPC memory like anonymous mappings, which is what we want (I think), e.g. making *any* EPC page executable will require PROCESS__EXECMEM (SGX is 64-bit only at this point, so SELinux will always have default_noexec). I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if the page is also writable or previously modified. The intent is to prevent arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created without EXECMEM as long as the EPC page mapping is only ever mapped RX and its initial contents came from an unmodified file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE). The idea is that by providing an SGX ioctl() to propagate VMA permissions from a source VMA, EXECMEM wouldn't be required to make an EPC page executable. E.g. userspace establishes an enclave in non-EPC memory from an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl() to copy the contents and permissions into EPC memory. Also, just to be clear, there is nothing inherently better about checking EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that reason. Using anon inodes also unfortunately disables SELinux inode-based checking since we no longer have any useful inode information, so you'd lose out on SELinux ioctl whitelisting on those enclave inodes if that matters. The problem is that all enclaves are associated with a single inode, i.e. /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to provide ioctls() and to allow mmap()'ing EPC memory. In no way is it associated with the content that actually gets loaded into EPC memory. The actual file that contains the enclave's contents (assuming the enclave came from a file) is a separate regular file that the SGX subsystem never sees. AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus PROCESS__EXECMEM achieves per-process granularity. No, FILE__WRITE and FILE__EXECUTE are a check between a process and a file, so you can ensure that only whitelisted processes are allowed both to /dev/sgx/enclave.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote: > On 5/17/19 12:20 PM, Stephen Smalley wrote: > >On 5/17/19 11:09 AM, Sean Christopherson wrote: > >>I think we may want to change the SGX API to alloc an anon inode for each > >>enclave instead of hanging every enclave off of the /dev/sgx/enclave > >>inode. > >>Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check() > >>will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave > >>VMAs > >>to RWX. Backing each enclave with an anon inode will make SELinux treat > >>EPC memory like anonymous mappings, which is what we want (I think), e.g. > >>making *any* EPC page executable will require PROCESS__EXECMEM (SGX is > >>64-bit only at this point, so SELinux will always have default_noexec). > > > >I don't think we want to require EXECMEM (or equivalently both FILE__WRITE > >and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, > >only if the page is also writable or previously modified. The intent is > >to prevent arbitrary code execution without EXECMEM (or > >FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created > >without EXECMEM as long as the EPC page mapping is only ever mapped RX and > >its initial contents came from an unmodified file mapping that was > >PROT_EXEC (and hence already checked via FILE__EXECUTE). The idea is that by providing an SGX ioctl() to propagate VMA permissions from a source VMA, EXECMEM wouldn't be required to make an EPC page executable. E.g. userspace establishes an enclave in non-EPC memory from an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl() to copy the contents and permissions into EPC memory. > Also, just to be clear, there is nothing inherently better about checking > EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the > /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that > reason. Using anon inodes also unfortunately disables SELinux inode-based > checking since we no longer have any useful inode information, so you'd lose > out on SELinux ioctl whitelisting on those enclave inodes if that matters. The problem is that all enclaves are associated with a single inode, i.e. /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to provide ioctls() and to allow mmap()'ing EPC memory. In no way is it associated with the content that actually gets loaded into EPC memory. The actual file that contains the enclave's contents (assuming the enclave came from a file) is a separate regular file that the SGX subsystem never sees. AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus PROCESS__EXECMEM achieves per-process granularity.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
> On May 17, 2019, at 9:37 AM, Stephen Smalley wrote: > >> On 5/17/19 12:20 PM, Stephen Smalley wrote: >>> On 5/17/19 11:09 AM, Sean Christopherson wrote: On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote: > On 5/16/19 6:23 PM, Xing, Cedric wrote: > I thought EXECMOD applied to files (and memory mappings backed by them) > but > I was probably wrong. It sounds like EXECMOD applies to the whole process > so > would allow all pages within a process's address space to be modified then > executed, regardless the backing files. Am I correct this time? No, you were correct the first time I think; EXECMOD is used to control whether a process can make executable a private file mapping that has previously been modified (e.g. text relocation); it is a special case to support text relocations without having to allow full EXECMEM (i.e. execute arbitrary memory). SELinux checks relevant to W^X include: - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of PROT_WRITE, since we know the content has to have been written at some point) or a private file mapping that is also PROT_WRITE. - EXECMOD: mprotect PROT_EXEC a private file mapping that has been previously modified, typically for text relocations, - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping, - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping. (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to this discussion) So if you want to ensure W^X, then you wouldn't allow EXECMEM for the process, EXECMOD by the process to any file, and the combination of both FILE__WRITE and FILE__EXECUTE by the process to any file. If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an anonymous inode, then I would expect that only the FILE__WRITE and FILE__EXECUTE checks are relevant. >>> >>> Yep, I was just typing this up in a different thread: >>> >>> I think we may want to change the SGX API to alloc an anon inode for each >>> enclave instead of hanging every enclave off of the /dev/sgx/enclave inode. >>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check() >>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs >>> to RWX. Backing each enclave with an anon inode will make SELinux treat >>> EPC memory like anonymous mappings, which is what we want (I think), e.g. >>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is >>> 64-bit only at this point, so SELinux will always have default_noexec). >> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE >> and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, >> only if the page is also writable or previously modified. The intent is to >> prevent arbitrary code execution without EXECMEM (or >> FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created >> without EXECMEM as long as the EPC page mapping is only ever mapped RX and >> its initial contents came from an unmodified file mapping that was PROT_EXEC >> (and hence already checked via FILE__EXECUTE). > > Also, just to be clear, there is nothing inherently better about checking > EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the > /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that > reason. Using anon inodes also unfortunately disables SELinux inode-based > checking since we no longer have any useful inode information, so you'd lose > out on SELinux ioctl whitelisting on those enclave inodes if that matters. How can that work? Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave. Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.
Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
On 5/17/19 12:20 PM, Stephen Smalley wrote: On 5/17/19 11:09 AM, Sean Christopherson wrote: On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote: On 5/16/19 6:23 PM, Xing, Cedric wrote: I thought EXECMOD applied to files (and memory mappings backed by them) but I was probably wrong. It sounds like EXECMOD applies to the whole process so would allow all pages within a process's address space to be modified then executed, regardless the backing files. Am I correct this time? No, you were correct the first time I think; EXECMOD is used to control whether a process can make executable a private file mapping that has previously been modified (e.g. text relocation); it is a special case to support text relocations without having to allow full EXECMEM (i.e. execute arbitrary memory). SELinux checks relevant to W^X include: - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of PROT_WRITE, since we know the content has to have been written at some point) or a private file mapping that is also PROT_WRITE. - EXECMOD: mprotect PROT_EXEC a private file mapping that has been previously modified, typically for text relocations, - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping, - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping. (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to this discussion) So if you want to ensure W^X, then you wouldn't allow EXECMEM for the process, EXECMOD by the process to any file, and the combination of both FILE__WRITE and FILE__EXECUTE by the process to any file. If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an anonymous inode, then I would expect that only the FILE__WRITE and FILE__EXECUTE checks are relevant. Yep, I was just typing this up in a different thread: I think we may want to change the SGX API to alloc an anon inode for each enclave instead of hanging every enclave off of the /dev/sgx/enclave inode. Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check() will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs to RWX. Backing each enclave with an anon inode will make SELinux treat EPC memory like anonymous mappings, which is what we want (I think), e.g. making *any* EPC page executable will require PROCESS__EXECMEM (SGX is 64-bit only at this point, so SELinux will always have default_noexec). I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if the page is also writable or previously modified. The intent is to prevent arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created without EXECMEM as long as the EPC page mapping is only ever mapped RX and its initial contents came from an unmodified file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE). Also, just to be clear, there is nothing inherently better about checking EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that reason. Using anon inodes also unfortunately disables SELinux inode-based checking since we no longer have any useful inode information, so you'd lose out on SELinux ioctl whitelisting on those enclave inodes if that matters.
