* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-23 19:58 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Stephen Smalley, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190523141752.GA12078@linux.intel.com>
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.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-23 23:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jarkko Sakkinen, Stephen Smalley, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux, Jethro Beekman,
Xing, Cedric, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrUzx3LPAKCLFf75P-XshAkRcr+JLET3LA_kHDs9MA11FA@mail.gmail.com>
On Thu, May 23, 2019 at 08:38:17AM -0700, Andy Lutomirski wrote:
> On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> <sean.j.christopherson@intel.com> 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 MAP_FIXED/mprotect().
Add SGX_IOC_MPROTECT (or SGX_IOC_MAP_REGION?) that works as follows:
1. Track VM_MAY{READ,WRITE,EXEC} flags for each enclave page.
2. SGX_IOC_ADD_REGION, i.e. EADD, initializes the VM_MAY* flags for each
enclave page based on the source VMA.
3. sgx_mmap() only works with PROT_NONE, skips alignment stuff if
MAP_FIXED, and clears VM_MAY{READ,WRITE,EXEC}.
4. mprotect() on /dev/sgx/enclave doesn't work because the VMA doesn't
have any VM_MAY{READ,WRITE,EXEC} capabilities.
5. Deny mremap() post-ECREATE as the address and size of the enclave
are fixed at ECREATE (in hardware).
6. SGX_IOC_MPROTECT works like normal mprotect(), except the VM_MAY*
flags are pulled from the enclave pages, and its call to
security_file_mprotect() is VM_READ|VM_EXEC by default. The LSM call
sets VM_WRITE iff the enclave page has both VM_MAYWRITE and
VM_MAYEXEC. The idea here is to require READ and EXECUTE to run an
enclave, and only require WRITE on /dev/sgx/enclave when the enclave
can execute modified memory.
To support SGX2 down the road, which will want to convert a page to
executable on the fly, we could add:
7. SGX_IOC_EXTEND_PERMISSIONS enables userspace to extend the VM_MAY*
flags for an enclave page, e.g. to make a page executable.
SGX_IOC_MPROTECT is still required to actually map the page.
Notably, adding a RW page to the enclave, e.g. to grow its heap,
doesn't require WRITE, whereas adding a RWX page, e.g. for dynamic
loading, would require WRITE. This can only extend! E.g. userspace
can't circumvent the WRITE requirement by clearing VM_MAYWRITE.
Note, FILE__WRITE on /dev/sgx/enclave is essentially equivalent to
FILE__EXECMOD. Using FILE__WRITE in this way means there are no changes
to SELinux (triggering FILE__EXECMOD would be awkward), and AppArmor also
picks up extra protections for enclaves.
> [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.
I don't see any reason to allow duplicate EADD as it serves no purpose,
e.g. doing so changes the enclave's measurement and that's it.
^ permalink raw reply
* Re: [PATCH v4 1/3] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Kees Cook @ 2019-05-24 0:04 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, kernel-hardening, linux-mm, linux-security-module,
Masahiro Yamada, Michal Hocko, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland
In-Reply-To: <20190523140844.132150-2-glider@google.com>
On Thu, May 23, 2019 at 04:08:42PM +0200, Alexander Potapenko wrote:
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 52e6fbb042cc..68fb6fa41cc1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1673,6 +1673,14 @@
>
> initrd= [BOOT] Specify the location of the initial ramdisk
>
> + init_on_alloc= [MM] Fill newly allocated pages and heap objects with
> + zeroes.
> + Format: 0 | 1
> + Default set by CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> + init_on_free= [MM] Fill freed pages and heap objects with zeroes.
> + Format: 0 | 1
> + Default set by CONFIG_INIT_ON_FREE_DEFAULT_ON.
> +
> init_pkru= [x86] Specify the default memory protection keys rights
> register contents for all processes. 0x55555554 by
> default (disallow access to all but pkey 0). Can
Nit: add a blank line between these new options' documentation to match
the others.
> diff --git a/security/Kconfig.hardening b/security/Kconfig.hardening
> index 0a1d4ca314f4..87883e3e3c2a 100644
> --- a/security/Kconfig.hardening
> +++ b/security/Kconfig.hardening
> @@ -159,6 +159,20 @@ config STACKLEAK_RUNTIME_DISABLE
> runtime to control kernel stack erasing for kernels built with
> CONFIG_GCC_PLUGIN_STACKLEAK.
>
> +config INIT_ON_ALLOC_DEFAULT_ON
> + bool "Set init_on_alloc=1 by default"
> + help
> + Enable init_on_alloc=1 by default, making the kernel initialize every
> + page and heap allocation with zeroes.
> + init_on_alloc can be overridden via command line.
> +
> +config INIT_ON_FREE_DEFAULT_ON
> + bool "Set init_on_free=1 by default"
> + help
> + Enable init_on_free=1 by default, making the kernel initialize freed
> + pages and slab memory with zeroes.
> + init_on_free can be overridden via command line.
> +
I think these could use a lot more detail. How about something like
these, with more details and performance notes:
config INIT_ON_ALLOC_DEFAULT_ON
bool "Enable heap memory zeroing on allocation by default"
help
This has the effect of setting "init_on_alloc=1" on the kernel
command line. This can be disabled with "init_on_alloc=0".
When "init_on_alloc" is enabled, all page allocator and slab
allocator memory will be zeroed when allocated, eliminating
many kinds of "uninitialized heap memory" flaws, especially
heap content exposures. The performance impact varies by
workload, but most cases see <1% impact. Some synthetic
workloads have measured as high as 7%.
config INIT_ON_FREE_DEFAULT_ON
bool "Enable heap memory zeroing on free by default"
help
This has the effect of setting "init_on_free=1" on the kernel
command line. This can be disabled with "init_on_free=0".
Similar to "init_on_alloc", when "init_on_free" is enabled,
all page allocator and slab allocator memory will be zeroed
when freed, eliminating many kinds of "uninitialized heap memory"
flaws, especially heap content exposures. The primary difference
with "init_on_free" is that data lifetime in memory is reduced,
as anything freed is wiped immediately, making live forensics or
cold boot memory attacks unable to recover freed memory contents.
The performance impact varies by workload, but is more expensive
than "init_on_alloc" due to the negative cache effects of
touching "cold" memory areas. Most cases see 3-5% impact. Some
synthetic workloads have measured as high as 8%.
--
Kees Cook
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 1:17 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Jarkko Sakkinen, Stephen Smalley, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190523234044.GC12078@linux.intel.com>
On Thu, May 23, 2019 at 4:40 PM Sean Christopherson
<sean.j.christopherson@intel.com> 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
> > <sean.j.christopherson@intel.com> 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 make the initial mapping
PROT_READ|PROT_WRITE|PROT_EXEC and have .fault actually instantiate
the PTEs with the intersection of the VMA permissions and MAXPERM. I
don't think I like this alternative, since it feels more magical than
needed and it will be harder to debug. I like the fact that
/proc/self/maps shows the actual permissions in all the other
variants.
All of the rest of the crud in my earlier email was just
implementation details. The point I was trying to make was that I
think it's possible to implement this without making too much of a
mess internally. I think I favor the mprotect() approach since it
makes the behavior fairly obvious.
I don't think any of this needs to change for SGX2. We'd have an
ioctl() that does EAUG and specifies MAXPERM. Trying to mprotect() a
page that hasn't been added yet with any permission other than
PROT_NONE would fail. I suppose we might end up needing a way to let
the EAUG operation *change* MAXPERM, and this operation would have to
do some more LSM checks and walk all the existing mappings to make
sure they're consistent with the new MAXPERM.
As an aside, I wonder if Linus et all would be okay with a new
MAP_FULLY_ALIGNED mmap() flag that allocated memory aligned to the
requested size. Then we could get rid of yet another bit of magic.
--Andy
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-24 7:24 UTC (permalink / raw)
To: Andy Lutomirski, Christopherson, Sean J
Cc: Jarkko Sakkinen, Stephen Smalley, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrV4DVEfW6EJ6DnQGGYDJAiA5M1QcuYJTiroumOM+D6Jjg@mail.gmail.com>
Hi Andy,
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Thursday, May 23, 2019 6:18 PM
>
> On Thu, May 23, 2019 at 4:40 PM Sean Christopherson <sean.j.christopherson@intel.com>
> 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
> > > <sean.j.christopherson@intel.com> 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 MAXPERM & R == R */
>
> But this has some annoying implications with regard to sgx_get_unmapped_area(). We could
> use an ioctl:
There's an easy fix. Just let sgx_get_unmapped_area() do the natural alignment only if MAP_FIXED is *not* set, otherwise, honor both address and len.
But mmap() is subject to LSM check (probably against /dev/sgx/enclave?). How to do mmap(RX) if FILE__EXECUTE is *not* granted for /dev/sgx/enclave, even if MAXPERM=RX?
>
> 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);
How to bypass LSM hooks in this mprotect()?
>
> Or, for the really evil option, we could use a bit of magic in .fault and do nothing here.
> Instead we'd make the initial mapping PROT_READ|PROT_WRITE|PROT_EXEC and have .fault
> actually instantiate the PTEs with the intersection of the VMA permissions and MAXPERM. I
> don't think I like this alternative, since it feels more magical than needed and it will
> be harder to debug. I like the fact that /proc/self/maps shows the actual permissions in
> all the other variants.
Agreed.
>
>
> All of the rest of the crud in my earlier email was just implementation details. The
> point I was trying to make was that I think it's possible to implement this without making
> too much of a mess internally. I think I favor the mprotect() approach since it makes the
> behavior fairly obvious.
>
> I don't think any of this needs to change for SGX2. We'd have an
> ioctl() that does EAUG and specifies MAXPERM. Trying to mprotect() a page that hasn't
> been added yet with any permission other than PROT_NONE would fail. I suppose we might
> end up needing a way to let the EAUG operation *change* MAXPERM, and this operation would
> have to do some more LSM checks and walk all the existing mappings to make sure they're
> consistent with the new MAXPERM.
EAUG ioctl could be a solution, but isn't optimal at least. What we've done is #PF based. Specifically, an SGX2 enclave will have its heap mapped as RW, but without any pages populated before EINIT. Then when the enclave needs a new page in its heap, it issues EACCEPT, which will cause a #PF and the driver will respond by EAUG a new EPC page. And then the enclave will be resumed and the faulted EACCEPT will be retried (and succeed).
>
> As an aside, I wonder if Linus et all would be okay with a new MAP_FULLY_ALIGNED mmap()
> flag that allocated memory aligned to the requested size. Then we could get rid of yet
> another bit of magic.
>
> --Andy
I've also got a chance to think more about it lately.
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
* + <None> - 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);
foreach phdr in loadable segments /* phdr->p_type == PT_LOAD */ {
/* <segment permission> below is subject to LSM checks */
loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, <segment permission>, image_fd, phdr->p_offset);
}
/* Step 2: Create enclave */
int enclave_fd = open("/dev/sgx/enclave", O_RDONLY /* or O_RDWR for SGX2 enclaves */);
void *enclave_base = mmap(NULL, <enclave size>, MAP_SHARED, PROT_READ, enclave_fd, 0); /* Only FILE__READ is required here */
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 source pages, and use that as *both* EPCM and VMA permissions).
* Given enclave_fd may have FILE__READ only, LSM has to be bypassed so the "mmap" part has to be done inside the driver.
* Initial EPC pages will be mapped only once, so no inode is needed to remember the initial permissions. mmap/mprotect afterwards are subject to FILE__* on /dev/sgx/enclave
* The key point here is: permissions of source pages govern initial permissions of EADD'ed pages, regardless FILE__* on /dev/sgx/enclave
*/
ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, s->base, s->size, s->perm...);
}
/* EADD other enclave components, e.g. TCS, stacks, heaps, etc. */
ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, tcs, 0x1000, RW | PT_TCS...);
ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, <zero page>, <stack size>, RW...);
...
/* Step 4 (SGX2 only): Reserve ranges for additional heaps, stacks, etc. */
/* FILE__WRITE required to allow expansion of data segments at runtime */
/* Key point here is: permissions, if needed to change at runtime, are subject to FILL__* on /dev/sgx/enclave */
mprotect(<heap address>, <heap size>, PROT_READ | PROT_WRITE);
/* Step 5: EINIT */
ioctl(IOC_EINIT, <sigstruct>...);
/* Step 6 (SGX2 only): Set RX for dynamically loaded code pages (e.g. Graphene, encrypted enclaves, etc.) as needed, at runtime */
/* FILE__EXECUTE required */
mprotect(<RX address>, <RX size>, PROT_READ | PROT_EXEC);
-Cedric
^ permalink raw reply
* Re: [PATCH V7 0/4] Add support for crypto agile logs
From: Jarkko Sakkinen @ 2019-05-24 10:38 UTC (permalink / raw)
To: James Morris
Cc: Matthew Garrett, linux-integrity, peterhuewe, jgg, roberto.sassu,
linux-efi, linux-security-module, linux-kernel, tweek, bsz
In-Reply-To: <alpine.LRH.2.21.1905240252440.31508@namei.org>
On Fri, May 24, 2019 at 02:54:20AM +1000, James Morris wrote:
> On Thu, 23 May 2019, Jarkko Sakkinen wrote:
>
> > On Thu, May 23, 2019 at 03:14:49PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, May 20, 2019 at 01:54:57PM -0700, Matthew Garrett wrote:
> > > > Identical to previous version except without the KSAN workaround - Ard
> > > > has a better solution for that.
> > >
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > Only applied to my master at this point becaues the patches from
> > my earlier PRs are not yet mirrored to security/next-general.
>
> Which PRs are these?
>
> btw, Linus wants security subsystem maintainers to submit PRs directly to
> him from now on.
>
> I'll only be carrying patches for the core LSM and new security mechanisms
> before they're merged and have a maintainer assigned.
I'm referring to these:
https://lore.kernel.org/linux-integrity/20190329115544.GA27351@linux.intel.com/
I got response from you that those were applied and there is another
response in that thread that they are being sent to Linus. That is why I
haven't done anything since. Most of them are critical fixes to v5.1
changes.
Should I now take the action to send a PR to Linus and tag them to
stable?
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-24 14:44 UTC (permalink / raw)
To: Andy Lutomirski, Sean Christopherson
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux, Jethro Beekman, Xing, Cedric,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrUzx3LPAKCLFf75P-XshAkRcr+JLET3LA_kHDs9MA11FA@mail.gmail.com>
On 5/23/19 11:38 AM, Andy Lutomirski wrote:
> On Thu, May 23, 2019 at 7:17 AM Sean Christopherson
> <sean.j.christopherson@intel.com> 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.
>
^ permalink raw reply
* Re: [PATCH v6 1/3] Add a new ima hook ima_kexec_cmdline to measure cmdline args
From: Mimi Zohar @ 2019-05-24 14:56 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: mjg59, roberto.sassu, vgoyal
In-Reply-To: <20190521000645.16227-2-prsriva02@gmail.com>
Hi Prakhar,
On Mon, 2019-05-20 at 17:06 -0700, Prakhar Srivastava wrote:
> Currently during kexec_file_load(soft reboot) the cmdline args
> passed are not measured and the PCR values are not reset.
This patch addresses not measuring the kexec boot cmdline. I don't
see a reason for mentioning anything about the PCR values not being
reset. Keep it simple.
> This results in the new kernel to assume a secure boot was
> followed. The boot args used to launch the new kernel need to be
> measured and carried over to the next kernel to be used for
> attestation. IMA supports only measuring files, no functionality
> exists to measure a buffer(kexec cmdline).
>
> This change adds a new functionality to measure buffers
> process_buffer_measurement which uses the hash of the buffer
> instead of file hash to add an entry in the ima log.
> A new ima hook ima_kexec_cmdline is also defined which calls
> into process_buffer_measurement to add the kexec_cmdline args
> to the log.
>
> A new policy KEXEC_CMDLINE is also defined to control measuring the
> kexec_cmdline buffer.
> This patch only adds IMA_MEASURE as a supported functionality.
>
> - A new ima hook ima_kexec_cmdline is defined to be called by the
> kexec code.
> - A new function process_buffer_measurement is defined to measure
> the buffer hash into the ima log.
> - A new func policy KEXEC_CMDLINE is defined to control the measurement.
Missing is how to verify the digest of the measurement list kexec boot
cmdline entry based on /proc/cmdline. Everything before the "root="
in the /proc entry needs to be removed before calculating the hash.
Please include a sample shell command.
Matthew's patch "IMA: Allow profiles to define the desired IMA
template" will require changes to this patch. Please rebase this
patch set on top of it, once Matthew addresses the comments and re-
posts.
(Reminder: the patch description should be 70 - 75 characters.)
>
> Signed-off-by: Prakhar Srivastava <prsriva02@gmail.com>
> ---
< snip >
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -576,6 +576,83 @@ int ima_load_data(enum kernel_load_data_id id)
> return 0;
> }
>
> +/*
> + * process_buffer_measurement - Measure the buffer to ima log.
> + * @buf: pointer to the buffer that needs to be added to the log.
> + * @size: size of buffer(in bytes).
> + * @eventname: event name to be used for the buffer entry.
> + * @cred: a pointer to a credentials structure for user validation.
> + * @secid: the secid of the task to be validated.
> + *
> + * Based on policy, the buffer is measured into the ima log.
> + */
> +static void process_buffer_measurement(const void *buf, int size,
> + const char *eventname, const struct cred *cred,
> + u32 secid)
> +{
> + int ret = 0;
> + struct ima_template_entry *entry = NULL;
> + struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> + struct ima_event_data event_data = {iint, NULL, NULL,
> + NULL, 0, NULL};
Thiago's clean up patch initializes only specific variables, as
needed. Please initialize event_data like:
struct ima_event_data event_data = {.iint = iint};
> + struct {
> + struct ima_digest_data hdr;
> + char digest[IMA_MAX_DIGEST_SIZE];
> + } hash;
> + int violation = 0;
> + int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> + int action = 0;
> +
> + action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr);
> + if (!(action & IMA_MEASURE))
> + goto out;
> +
> + memset(iint, 0, sizeof(*iint));
> + memset(&hash, 0, sizeof(hash));
> +
> + event_data.filename = eventname;
> +
> + iint->ima_hash = &hash.hdr;
> + iint->ima_hash->algo = ima_hash_algo;
> + iint->ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> + ret = ima_calc_buffer_hash(buf, size, iint->ima_hash);
> + if (ret < 0)
> + goto out;
> +
> + ret = ima_alloc_init_template(&event_data, &entry);
> + if (ret < 0)
> + goto out;
> +
> + if (action & IMA_MEASURE)
> + ret = ima_store_template(entry, violation, NULL, buf, pcr);
> +
> + if (ret < 0) {
> + ima_free_template_entry(entry);
> + }
Remove brackets.
Mimi
> +
> +out:
> + return;
> +}
^ permalink raw reply
* Re: [PATCH v6 2/3] add a new ima template field buf
From: Mimi Zohar @ 2019-05-24 15:12 UTC (permalink / raw)
To: Prakhar Srivastava, linux-integrity, linux-security-module,
linux-kernel
Cc: mjg59, roberto.sassu, vgoyal
In-Reply-To: <20190521000645.16227-3-prsriva02@gmail.com>
On Mon, 2019-05-20 at 17:06 -0700, Prakhar Srivastava wrote:
> A buffer(cmdline args) measured into ima cannot be appraised
> without already being aware of the buffer contents.Since we
> don't know what cmdline args will be passed (or need to validate
> what was passed) it is not possible to appraise it.
>
> Since hashs are non reversible the raw buffer is needed to
> recompute the hash.
> To regenrate the hash of the buffer and appraise the same
> the contents of the buffer need to be available.
>
> A new template field buf is added to the existing ima template
> fields, which can be used to store/read the buffer itself.
> Two new fields are added to the ima_event_data to carry the
> buf and buf_len whenever necessary.
>
> Updated the process_buffer_measurement call to add the buf
> to the ima_event_data.
> process_buffer_measurement added in "Add a new ima hook
> ima_kexec_cmdline to measure cmdline args"
>
> - Add a new template field 'buf' to be used to store/read
> the buffer data.
> - Added two new fields to ima_event_data to hold the buf and
> buf_len [Suggested by Roberto]
> -Updated process_buffer_meaurement to add the buffer to
> ima_event_data
This patch description can be written more concisely.
Patch 1/3 in this series introduces measuring the kexec boot command
line. This patch defines a new template field for storing the kexec
boot command line in the measurement list in order for a remote
attestation server to verify.
As mentioned, the first patch description should include a shell
command for verifying the digest in the kexec boot command line
measurement list record against /proc/cmdline. This patch description
should include a shell command showing how to verify the digest based
on the new field. Should the new field in the ascii measurement list
be displayed as a string, not hex?
Mimi
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-24 15:41 UTC (permalink / raw)
To: Xing, Cedric, Andy Lutomirski, Christopherson, Sean J
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8956@ORSMSX116.amr.corp.intel.com>
On 5/24/19 3:24 AM, Xing, Cedric wrote:
> Hi Andy,
>
>> From: Andy Lutomirski [mailto:luto@kernel.org]
>> Sent: Thursday, May 23, 2019 6:18 PM
>>
>> On Thu, May 23, 2019 at 4:40 PM Sean Christopherson <sean.j.christopherson@intel.com>
>> 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
>>>> <sean.j.christopherson@intel.com> 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 MAXPERM & R == R */
>>
>> But this has some annoying implications with regard to sgx_get_unmapped_area(). We could
>> use an ioctl:
>
> There's an easy fix. Just let sgx_get_unmapped_area() do the natural alignment only if MAP_FIXED is *not* set, otherwise, honor both address and len.
>
> But mmap() is subject to LSM check (probably against /dev/sgx/enclave?). How to do mmap(RX) if FILE__EXECUTE is *not* granted for /dev/sgx/enclave, even if MAXPERM=RX?
>
>>
>> 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);
>
> How to bypass LSM hooks in this mprotect()?
>
>>
>> Or, for the really evil option, we could use a bit of magic in .fault and do nothing here.
>> Instead we'd make the initial mapping PROT_READ|PROT_WRITE|PROT_EXEC and have .fault
>> actually instantiate the PTEs with the intersection of the VMA permissions and MAXPERM. I
>> don't think I like this alternative, since it feels more magical than needed and it will
>> be harder to debug. I like the fact that /proc/self/maps shows the actual permissions in
>> all the other variants.
>
> Agreed.
>
>>
>>
>> All of the rest of the crud in my earlier email was just implementation details. The
>> point I was trying to make was that I think it's possible to implement this without making
>> too much of a mess internally. I think I favor the mprotect() approach since it makes the
>> behavior fairly obvious.
>>
>> I don't think any of this needs to change for SGX2. We'd have an
>> ioctl() that does EAUG and specifies MAXPERM. Trying to mprotect() a page that hasn't
>> been added yet with any permission other than PROT_NONE would fail. I suppose we might
>> end up needing a way to let the EAUG operation *change* MAXPERM, and this operation would
>> have to do some more LSM checks and walk all the existing mappings to make sure they're
>> consistent with the new MAXPERM.
>
> EAUG ioctl could be a solution, but isn't optimal at least. What we've done is #PF based. Specifically, an SGX2 enclave will have its heap mapped as RW, but without any pages populated before EINIT. Then when the enclave needs a new page in its heap, it issues EACCEPT, which will cause a #PF and the driver will respond by EAUG a new EPC page. And then the enclave will be resumed and the faulted EACCEPT will be retried (and succeed).
>
>>
>> As an aside, I wonder if Linus et all would be okay with a new MAP_FULLY_ALIGNED mmap()
>> flag that allocated memory aligned to the requested size. Then we could get rid of yet
>> another bit of magic.
>>
>> --Andy
>
> I've also got a chance to think more about it lately.
>
> 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.
> * + <None> - 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 */ {
> /* <segment permission> below is subject to LSM checks */
> loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, <segment 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).
> }
>
> /* 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.
> void *enclave_base = mmap(NULL, <enclave size>, MAP_SHARED, PROT_READ, enclave_fd, 0); /* Only FILE__READ is required here */
FILE__READ revalidated to /dev/sgx/enclave upon mmap().
> 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 source pages, and use that as *both* EPCM and VMA permissions).
> * Given enclave_fd may have FILE__READ only, LSM has to be bypassed so the "mmap" part has to be done inside the driver.
> * Initial EPC pages will be mapped only once, so no inode is needed to remember the initial permissions. mmap/mprotect afterwards are subject to FILE__* on /dev/sgx/enclave
> * The key point here is: permissions of source pages govern initial permissions of EADD'ed pages, regardless FILE__* on /dev/sgx/enclave
> */
> ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, s->base, s->size, s->perm...);
> }
> /* EADD other enclave components, e.g. TCS, stacks, heaps, etc. */
> ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, tcs, 0x1000, RW | PT_TCS...);
> ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, <zero page>, <stack size>, RW...);
> ...
>
> /* Step 4 (SGX2 only): Reserve ranges for additional heaps, stacks, etc. */
> /* FILE__WRITE required to allow expansion of data segments at runtime */
> /* Key point here is: permissions, if needed to change at runtime, are subject to FILL__* on /dev/sgx/enclave */
> mprotect(<heap address>, <heap size>, PROT_READ | PROT_WRITE);
FILE__READ and FILE__WRITE revalidated to /dev/sgx/enclave upon mprotect().
>
> /* Step 5: EINIT */
> ioctl(IOC_EINIT, <sigstruct>...);
>
> /* Step 6 (SGX2 only): Set RX for dynamically loaded code pages (e.g. Graphene, encrypted enclaves, etc.) as needed, at runtime */
> /* FILE__EXECUTE required */
> mprotect(<RX address>, <RX size>, PROT_READ | PROT_EXEC);
FILE__READ revalidated and FILE__EXECUTE checked to /dev/sgx/enclave
upon mprotect(). Cumulative set of checks at this point is
FILE__READ|FILE__WRITE|FILE__EXECUTE.
What would the step be for a SGX1 RW->RX change? How would that trigger
EXECMOD? Do we really need to distinguish it from the SGX2 dynamically
loaded code case?
>
> -Cedric
>
^ permalink raw reply
* Re:
From: Roberto Sassu @ 2019-05-24 15:42 UTC (permalink / raw)
To: Mimi Zohar, Prakhar Srivastava, linux-integrity,
linux-security-module, linux-kernel
Cc: mjg59, vgoyal
In-Reply-To: <1558710722.3977.68.camel@linux.ibm.com>
On 5/24/2019 5:12 PM, Mimi Zohar wrote:
> On Mon, 2019-05-20 at 17:06 -0700, Prakhar Srivastava wrote:
>> A buffer(cmdline args) measured into ima cannot be appraised
>> without already being aware of the buffer contents.Since we
>> don't know what cmdline args will be passed (or need to validate
>> what was passed) it is not possible to appraise it.
>>
>> Since hashs are non reversible the raw buffer is needed to
>> recompute the hash.
>> To regenrate the hash of the buffer and appraise the same
>> the contents of the buffer need to be available.
>>
>> A new template field buf is added to the existing ima template
>> fields, which can be used to store/read the buffer itself.
>> Two new fields are added to the ima_event_data to carry the
>> buf and buf_len whenever necessary.
>>
>> Updated the process_buffer_measurement call to add the buf
>> to the ima_event_data.
>> process_buffer_measurement added in "Add a new ima hook
>> ima_kexec_cmdline to measure cmdline args"
>>
>> - Add a new template field 'buf' to be used to store/read
>> the buffer data.
>> - Added two new fields to ima_event_data to hold the buf and
>> buf_len [Suggested by Roberto]
>> -Updated process_buffer_meaurement to add the buffer to
>> ima_event_data
>
> This patch description can be written more concisely.
>
> Patch 1/3 in this series introduces measuring the kexec boot command
> line. This patch defines a new template field for storing the kexec
> boot command line in the measurement list in order for a remote
> attestation server to verify.
>
> As mentioned, the first patch description should include a shell
> command for verifying the digest in the kexec boot command line
> measurement list record against /proc/cmdline. This patch description
> should include a shell command showing how to verify the digest based
> on the new field. Should the new field in the ascii measurement list
> be displayed as a string, not hex?
We should define a new type. If the type is DATA_FMT_STRING, spaces are
replaced with '_'.
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re:
From: Roberto Sassu @ 2019-05-24 15:47 UTC (permalink / raw)
To: Mimi Zohar, Prakhar Srivastava, linux-integrity,
linux-security-module, linux-kernel
Cc: mjg59, vgoyal
In-Reply-To: <a7acac28-156e-80d1-b759-cb0c59f73169@huawei.com>
On 5/24/2019 5:42 PM, Roberto Sassu wrote:
> On 5/24/2019 5:12 PM, Mimi Zohar wrote:
>> On Mon, 2019-05-20 at 17:06 -0700, Prakhar Srivastava wrote:
>>> A buffer(cmdline args) measured into ima cannot be appraised
>>> without already being aware of the buffer contents.Since we
>>> don't know what cmdline args will be passed (or need to validate
>>> what was passed) it is not possible to appraise it.
>>>
>>> Since hashs are non reversible the raw buffer is needed to
>>> recompute the hash.
>>> To regenrate the hash of the buffer and appraise the same
>>> the contents of the buffer need to be available.
>>>
>>> A new template field buf is added to the existing ima template
>>> fields, which can be used to store/read the buffer itself.
>>> Two new fields are added to the ima_event_data to carry the
>>> buf and buf_len whenever necessary.
>>>
>>> Updated the process_buffer_measurement call to add the buf
>>> to the ima_event_data.
>>> process_buffer_measurement added in "Add a new ima hook
>>> ima_kexec_cmdline to measure cmdline args"
>>>
>>> - Add a new template field 'buf' to be used to store/read
>>> the buffer data.
>>> - Added two new fields to ima_event_data to hold the buf and
>>> buf_len [Suggested by Roberto]
>>> -Updated process_buffer_meaurement to add the buffer to
>>> ima_event_data
>>
>> This patch description can be written more concisely.
>>
>> Patch 1/3 in this series introduces measuring the kexec boot command
>> line. This patch defines a new template field for storing the kexec
>> boot command line in the measurement list in order for a remote
>> attestation server to verify.
>>
>> As mentioned, the first patch description should include a shell
>> command for verifying the digest in the kexec boot command line
>> measurement list record against /proc/cmdline. This patch description
>> should include a shell command showing how to verify the digest based
>> on the new field. Should the new field in the ascii measurement list
>> be displayed as a string, not hex?
>
> We should define a new type. If the type is DATA_FMT_STRING, spaces are
> replaced with '_'.
Or better. Leave it as hex, otherwise there would be a parsing problem
if there are spaces in the data for a field.
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 16:43 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, Christopherson, Sean J, Jarkko Sakkinen,
Stephen Smalley, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8956@ORSMSX116.amr.corp.intel.com>
On Fri, May 24, 2019 at 12:24 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Thursday, May 23, 2019 6:18 PM
> >
> > On Thu, May 23, 2019 at 4:40 PM Sean Christopherson <sean.j.christopherson@intel.com>
> > 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
> > > > <sean.j.christopherson@intel.com> 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 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).
That's exactly what I intended here. If you specify MAXPERM=RX, then
the kernel can validate that the source address is executable.
>
> >
> > 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:
>
> There's an easy fix. Just let sgx_get_unmapped_area() do the natural alignment only if MAP_FIXED is *not* set, otherwise, honor both address and len.
>
> But mmap() is subject to LSM check (probably against /dev/sgx/enclave?). How to do mmap(RX) if FILE__EXECUTE is *not* granted for /dev/sgx/enclave, even if MAXPERM=RX?
I think we just let /dev/sgx/enclave be FILE__EXECUTE. We don't
*have* to make it so that FILE__WRITE and FILE__EXECUTE on
/dev/sgx/enclave means you can create RWX enclave mappings.
>
> >
> > 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);
>
> How to bypass LSM hooks in this mprotect()?
Hmm. I guess we either use FILE__WRITE and FILE__EXECUTE or we use ioctl().
>
> >
> > Or, for the really evil option, we could use a bit of magic in .fault and do nothing here.
> > Instead we'd make the initial mapping PROT_READ|PROT_WRITE|PROT_EXEC and have .fault
> > actually instantiate the PTEs with the intersection of the VMA permissions and MAXPERM. I
> > don't think I like this alternative, since it feels more magical than needed and it will
> > be harder to debug. I like the fact that /proc/self/maps shows the actual permissions in
> > all the other variants.
>
> Agreed.
>
> >
> >
> > All of the rest of the crud in my earlier email was just implementation details. The
> > point I was trying to make was that I think it's possible to implement this without making
> > too much of a mess internally. I think I favor the mprotect() approach since it makes the
> > behavior fairly obvious.
> >
> > I don't think any of this needs to change for SGX2. We'd have an
> > ioctl() that does EAUG and specifies MAXPERM. Trying to mprotect() a page that hasn't
> > been added yet with any permission other than PROT_NONE would fail. I suppose we might
> > end up needing a way to let the EAUG operation *change* MAXPERM, and this operation would
> > have to do some more LSM checks and walk all the existing mappings to make sure they're
> > consistent with the new MAXPERM.
>
> EAUG ioctl could be a solution, but isn't optimal at least. What we've done is #PF based. Specifically, an SGX2 enclave will have its heap mapped as RW, but without any pages populated before EINIT. Then when the enclave needs a new page in its heap, it issues EACCEPT, which will cause a #PF and the driver will respond by EAUG a new EPC page. And then the enclave will be resumed and the faulted EACCEPT will be retried (and succeed).
>
If the driver works like that, then whatever call sets up this lazily
allocated heap could do the MAXPERM part.
That being said, is the performance advantage from putting this logic
in the kernel instead of in the untrusted part of the SDK really
worthwhile?
> >
> > As an aside, I wonder if Linus et all would be okay with a new MAP_FULLY_ALIGNED mmap()
> > flag that allocated memory aligned to the requested size. Then we could get rid of yet
> > another bit of magic.
> >
> > --Andy
>
> I've also got a chance to think more about it lately.
>
> 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.)
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.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-24 16:57 UTC (permalink / raw)
To: Stephen Smalley, Andy Lutomirski, Christopherson, Sean J
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <dda0912b-cb15-3c07-d368-345159e995f7@tycho.nsa.gov>
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.
>
> > * + <None> - 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 */ {
> > /* <segment permission> below is subject to LSM checks */
> > loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE,
> > <segment 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).
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, <enclave size>, 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 source pages, and use that as *both* EPCM and VMA
> permissions).
> > * Given enclave_fd may have FILE__READ only, LSM has to be
> bypassed so the "mmap" part has to be done inside the driver.
> > * Initial EPC pages will be mapped only once, so no inode is
> needed to remember the initial permissions. mmap/mprotect afterwards are
> subject to FILE__* on /dev/sgx/enclave
> > * The key point here is: permissions of source pages govern
> initial permissions of EADD'ed pages, regardless FILE__* on
> /dev/sgx/enclave
> > */
> > ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, s->base, s->size,
> > s->perm...); }
> > /* EADD other enclave components, e.g. TCS, stacks, heaps, etc. */
> > ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, tcs, 0x1000, RW |
> > PT_TCS...); ioctl(enclave_fd, IOC_EADD_AND_MAP_SEGMENT, <zero page>,
> > <stack size>, RW...); ...
> >
> > /* Step 4 (SGX2 only): Reserve ranges for additional heaps, stacks,
> > etc. */
> > /* FILE__WRITE required to allow expansion of data segments at runtime
> > */
> > /* Key point here is: permissions, if needed to change at runtime, are
> > subject to FILL__* on /dev/sgx/enclave */ mprotect(<heap address>,
> > <heap size>, PROT_READ | PROT_WRITE);
>
> FILE__READ and FILE__WRITE revalidated to /dev/sgx/enclave upon
> mprotect().
Yes. The intention here is to limit "runtime" permissions by accesses granted to the calling process to /dev/sgx/enclave. The "initial" permissions are set by ioctl to bypass LSM, because they are derived/determined by the enclave file.
Alternatively, the driver can remember "initial" permissions for each EPC page at IOC_EADD, to be committed at IOC_EINIT. Then this new IOC_EADD_AND_MAP will not be needed.
>
> >
> > /* Step 5: EINIT */
> > ioctl(IOC_EINIT, <sigstruct>...);
> >
> > /* Step 6 (SGX2 only): Set RX for dynamically loaded code pages (e.g.
> > Graphene, encrypted enclaves, etc.) as needed, at runtime */
> > /* FILE__EXECUTE required */
> > mprotect(<RX address>, <RX size>, PROT_READ | PROT_EXEC);
>
> FILE__READ revalidated and FILE__EXECUTE checked to /dev/sgx/enclave
> upon mprotect(). Cumulative set of checks at this point is
> FILE__READ|FILE__WRITE|FILE__EXECUTE.
>
> What would the step be for a SGX1 RW->RX change? How would that trigger
> EXECMOD? Do we really need to distinguish it from the SGX2 dynamically
> loaded code case?
Per your earlier comments, FILE__EXECMOD is also needed I think to allow RW->RX changes.
FILE__WRITE controls EAUG. I'm not judging its necessity, but just saying they are both valid combinations. To minimize impact to LSM, I don't want to special-case /dev/sgx/enclave. And the current semantics of FILE__* distinguish those two naturally.
BTW, there are usages, such as encrypted enclaves (https://github.com/intel/linux-sgx-pcl), requiring RW->RX but not EAUG. Graphene could also run on SGX1, provided that pages needed by shared objects are all pre-allocated before EINIT. All those could run without FILE__WRITE.
>
> >
> > -Cedric
> >
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 17:07 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, Jarkko Sakkinen, Stephen Smalley, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrX0WqouSWgdM+LNxMzypa0ZHZXTmJ+nNkuPuL8UOF_f2w@mail.gmail.com>
On Fri, May 24, 2019 at 09:43:27AM -0700, Andy Lutomirski wrote:
> On Fri, May 24, 2019 at 12:24 AM Xing, Cedric <cedric.xing@intel.com> 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?
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 17:42 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Andy Lutomirski, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <dda0912b-cb15-3c07-d368-345159e995f7@tycho.nsa.gov>
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.
>
> > * + <None> - 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 */ {
> > /* <segment permission> below is subject to LSM checks */
> > loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, <segment 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.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 17:51 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Xing, Cedric, Jarkko Sakkinen, Stephen Smalley,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524170704.GA3401@linux.intel.com>
> On May 24, 2019, at 10:07 AM, Sean Christopherson <sean.j.christopherson@intel.com> 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 <cedric.xing@intel.com> 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.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-24 17:54 UTC (permalink / raw)
To: Sean Christopherson
Cc: Stephen Smalley, Xing, Cedric, Andy Lutomirski, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524174243.GA365@linux.intel.com>
> On May 24, 2019, at 10:42 AM, Sean Christopherson <sean.j.christopherson@intel.com> 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.
>>
>>> * + <None> - 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 */ {
>>> /* <segment permission> below is subject to LSM checks */
>>> loadable_segments[i] = mmap(NULL, phdr->p_memsz, MAP_PRIATE, <segment 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.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 17:54 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Andy Lutomirski, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190524174243.GA365@linux.intel.com>
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.
>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 17:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Xing, Cedric, Andy Lutomirski, Jarkko Sakkinen,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <56EA6C7C-F69E-42EB-9CFB-CD0300549298@amacapital.net>
On Fri, May 24, 2019 at 10:54:34AM -0700, Andy Lutomirski wrote:
>
> > On May 24, 2019, at 10:42 AM, Sean Christopherson <sean.j.christopherson@intel.com> 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.
^ permalink raw reply
* Re: Re:
From: Mimi Zohar @ 2019-05-24 18:09 UTC (permalink / raw)
To: Roberto Sassu, Prakhar Srivastava, linux-integrity,
linux-security-module, linux-kernel
Cc: mjg59, vgoyal
In-Reply-To: <cb0eb785-9050-738e-c1bf-8e769fe096fa@huawei.com>
> >> As mentioned, the first patch description should include a shell
> >> command for verifying the digest in the kexec boot command line
> >> measurement list record against /proc/cmdline. This patch description
> >> should include a shell command showing how to verify the digest based
> >> on the new field. Should the new field in the ascii measurement list
> >> be displayed as a string, not hex?
> >
> > We should define a new type. If the type is DATA_FMT_STRING, spaces are
> > replaced with '_'.
>
> Or better. Leave it as hex, otherwise there would be a parsing problem
> if there are spaces in the data for a field.
After making a few changes, the measurement list contains the
following kexec-cmdline data:
10 edc32d1e3a5ba7272280a395b6fb56a5ef7c78c3 ima-buf
sha256:4f43b7db850e
88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b45275
kexec-cmdline
726f6f
743d2f6465762f7364613420726f2072642e6c756b732e757569643d6c756b73
2d6637
3633643737632d653236622d343431642d613734652d62363633636334643832
656120
696d615f706f6c6963793d7463627c61707072616973655f746362
There's probably a better shell command, but the following works to
verify the digest locally against the /proc/cmdline:
$ echo -n -e `cat /proc/cmdline | sed 's/^.*root=/root=/'` | sha256sum
4f43b7db850e88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b4527f65 -
If we leave the "buf" field as ascii-hex, what would the shell command
look like when verifying the digest based on the "buf" field?
Mimi
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-24 18:34 UTC (permalink / raw)
To: Christopherson, Sean J, Stephen Smalley
Cc: Andy Lutomirski, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190524175458.GB365@linux.intel.com>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@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
^ permalink raw reply
* Re: Re:
From: prakhar srivastava @ 2019-05-24 19:00 UTC (permalink / raw)
To: Mimi Zohar
Cc: Roberto Sassu, linux-integrity, linux-security-module,
linux-kernel, Matthew Garrett, vgoyal
In-Reply-To: <1558721385.3977.84.camel@linux.ibm.com>
On Fri, May 24, 2019 at 11:09 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> > >> As mentioned, the first patch description should include a shell
> > >> command for verifying the digest in the kexec boot command line
> > >> measurement list record against /proc/cmdline. This patch description
> > >> should include a shell command showing how to verify the digest based
> > >> on the new field. Should the new field in the ascii measurement list
> > >> be displayed as a string, not hex?
> > >
> > > We should define a new type. If the type is DATA_FMT_STRING, spaces are
> > > replaced with '_'.
> >
> > Or better. Leave it as hex, otherwise there would be a parsing problem
> > if there are spaces in the data for a field.
>
> After making a few changes, the measurement list contains the
> following kexec-cmdline data:
>
> 10 edc32d1e3a5ba7272280a395b6fb56a5ef7c78c3 ima-buf
> sha256:4f43b7db850e
> 88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b45275
> kexec-cmdline
> 726f6f
> 743d2f6465762f7364613420726f2072642e6c756b732e757569643d6c756b73
> 2d6637
> 3633643737632d653236622d343431642d613734652d62363633636334643832
> 656120
> 696d615f706f6c6963793d7463627c61707072616973655f746362
>
> There's probably a better shell command, but the following works to
> verify the digest locally against the /proc/cmdline:
>
> $ echo -n -e `cat /proc/cmdline | sed 's/^.*root=/root=/'` | sha256sum
> 4f43b7db850e88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b4527f65 -
>
> If we leave the "buf" field as ascii-hex, what would the shell command
> look like when verifying the digest based on the "buf" field?
>
> Mimi
>
To quickly test the sha256 i used the my /proc/cmdline
ro quiet splash vt.handoff=1 ima_policy=tcb ima_appraise=fix
ima_template_fmt=n-ng|d-ng|sig|buf ima_hash=sha256
export $VAL=
726f2071756965742073706c6173682076742e68616e646f66663d3120
696d615f706f6c6963793d74636220696d615f61707072616973653d666
97820696d615f74656d706c6174655f666d743d6e2d6e677c642d6e677c
7369677c62756620696d615f686173683d736861323536
echo -n -e $VAL | xxd -r -p | sha256sum
0d0b891bb730120d9593799cba1a7b3febf68f2bb81fb1304b0c963f95f6bc58 -
I will run it through the code as well, but the shell command should work.
Thanks,
Prakhar Srivastava
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-24 19:13 UTC (permalink / raw)
To: Xing, Cedric
Cc: Stephen Smalley, Andy Lutomirski, Jarkko Sakkinen, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654E8E1D@ORSMSX116.amr.corp.intel.com>
On Fri, May 24, 2019 at 11:34:32AM -0700, Xing, Cedric wrote:
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@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 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.
^ permalink raw reply
* Re: Re:
From: Mimi Zohar @ 2019-05-24 19:15 UTC (permalink / raw)
To: prakhar srivastava
Cc: Roberto Sassu, linux-integrity, linux-security-module,
linux-kernel, Matthew Garrett, vgoyal
In-Reply-To: <CAEFn8q+8QUs8iCP+Q3VujTkTU+XYYPGY8O8+KT1=6vvc-khJFg@mail.gmail.com>
On Fri, 2019-05-24 at 12:00 -0700, prakhar srivastava wrote:
> On Fri, May 24, 2019 at 11:09 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > > >> As mentioned, the first patch description should include a shell
> > > >> command for verifying the digest in the kexec boot command line
> > > >> measurement list record against /proc/cmdline. This patch description
> > > >> should include a shell command showing how to verify the digest based
> > > >> on the new field. Should the new field in the ascii measurement list
> > > >> be displayed as a string, not hex?
> > > >
> > > > We should define a new type. If the type is DATA_FMT_STRING, spaces are
> > > > replaced with '_'.
> > >
> > > Or better. Leave it as hex, otherwise there would be a parsing problem
> > > if there are spaces in the data for a field.
> >
> > After making a few changes, the measurement list contains the
> > following kexec-cmdline data:
> >
> > 10 edc32d1e3a5ba7272280a395b6fb56a5ef7c78c3 ima-buf
> > sha256:4f43b7db850e
> > 88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b45275
> > kexec-cmdline
> > 726f6f
> > 743d2f6465762f7364613420726f2072642e6c756b732e757569643d6c756b73
> > 2d6637
> > 3633643737632d653236622d343431642d613734652d62363633636334643832
> > 656120
> > 696d615f706f6c6963793d7463627c61707072616973655f746362
> >
> > There's probably a better shell command, but the following works to
> > verify the digest locally against the /proc/cmdline:
> >
> > $ echo -n -e `cat /proc/cmdline | sed 's/^.*root=/root=/'` | sha256sum
> > 4f43b7db850e88c49dfeffd4b1eb4f021d78033dfb05b07e45eec8d0b4527f65 -
> >
> > If we leave the "buf" field as ascii-hex, what would the shell command
> > look like when verifying the digest based on the "buf" field?
> >
> > Mimi
> >
> To quickly test the sha256 i used the my /proc/cmdline
> ro quiet splash vt.handoff=1 ima_policy=tcb ima_appraise=fix
> ima_template_fmt=n-ng|d-ng|sig|buf ima_hash=sha256
>
> export $VAL=
> 726f2071756965742073706c6173682076742e68616e646f66663d3120
> 696d615f706f6c6963793d74636220696d615f61707072616973653d666
> 97820696d615f74656d706c6174655f666d743d6e2d6e677c642d6e677c
> 7369677c62756620696d615f686173683d736861323536
>
> echo -n -e $VAL | xxd -r -p | sha256sum
> 0d0b891bb730120d9593799cba1a7b3febf68f2bb81fb1304b0c963f95f6bc58 -
>
> I will run it through the code as well, but the shell command should work.
Yes, that works.
sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements
| grep kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox