*Re: [RFC PATCH v4 06/12] mm: Introduce vm_ops->may_mprotect()
2019-06-19 22:23 ` [RFC PATCH v4 06/12] mm: Introduce vm_ops->may_mprotect() Sean Christopherson
@ 2019-06-21 1:35 ` Jarkko Sakkinen0 siblings, 0 replies; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-06-21 1:35 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
On Wed, Jun 19, 2019 at 03:23:55PM -0700, Sean Christopherson wrote:
> SGX will use ->may_mprotect() to invoke an SGX variant of the existing
> file_mprotect() and mmap_file() LSM hooks.
>
> The name may_mprotect() is intended to reflect the hook's purpose as a
> way to restrict mprotect() as opposed to a wholesale replacement.
>
> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
> MAP_SHARED. Furthermore, all enclaves need read, write and execute
> VMAs. As a result, applying W^X restrictions on /dev/sgx/enclave using
> existing LSM hooks is for all intents and purposes impossible, e.g.
> denying either W or X would deny access to *any* enclave.
>
> By hooking mprotect(), SGX can invoke an SGX specific LSM hook, which in
> turn allows LSMs to enforce W^X policies.
>
> Alternatively, SGX could provide a helper to identify enclaves given a
> vma or file. LSMs could then check if a mapping is for enclave and take
> action according.
>
> A second alternative would be to have SGX implement its own LSM hooks
> for file_mprotect() and mmap_file(), using them to "forward" the call to
> the SGX specific hook.
>
> The major con to both alternatives is that they provide zero flexibility
> for the SGX specific LSM hook. The "is_sgx_enclave()" helper doesn't
> allow SGX can't supply any additional information whatsoever, and the
> mmap_file() hook is called before the final address is known, e.g. SGX
> can't provide any information about the specific enclave being mapped.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Absolutely nothing to say about this one. We can take it as part of the
main patch set as it is. Not going to apply it though before the things
have been sorted out.
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
2019-06-19 22:23 ` [RFC PATCH v4 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX Sean Christopherson
@ 2019-06-21 2:28 ` Jarkko Sakkinen
2019-06-21 16:54 ` Xing, Cedric1 sibling, 0 replies; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-06-21 2:28 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
On Wed, Jun 19, 2019 at 03:23:56PM -0700, Sean Christopherson wrote:
> enclave_map() is an SGX specific variant of file_mprotect() and
> mmap_file(), and is provided so that LSMs can apply W^X restrictions to
> enclaves.
>
> Due to the nature of SGX and its Enclave Page Cache (EPC), all enclave
> VMAs are backed by a single file, i.e. /dev/sgx/enclave, that must be
> MAP_SHARED. Furthermore, all enclaves need read, write and execute
> VMAs. As a result, applying W^X restrictions on /dev/sgx/enclave using
> existing LSM hooks is for all intents and purposes impossible, e.g.
> denying either W or X would deny access to any enclave.
>
> Note, extensive discussion yielded no sane alternative to some form of
> SGX specific LSM hook[1].
>
> [1] https://lkml.kernel.org/r/CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
All the non-LSM changes are almost cleared from my part.
I would suggest that we scrape v21 together as soon as you return from
your vacation discluding the LSM hooks.
There is no any particular reason to get the LSM changes to the mainline
before the SGX foundations so now is the right time close things
underlying them.
I'm now in the same boat with your changes to the ioctl API, which means
that we are ready to go. I feel a tiny bit bad that it took me so long
time with [1] but I'm a simple minded person so what I can do :-)
Once you can come back please deal with the suggestions that I made
and provide a "pure" SRCU patch (apologies for repeating myself). I
will the squash them to the existing patch set.
After that is fully done we can make v21 scope decision when it comes
to the enclave life-cycle.
Even if the LSM changes would not be upstreamed as part of the
foundations I can start holding versions of them in my tree but only
after v21 is out.
Can you cope with this plan?
[1] https://patchwork.kernel.org/patch/11005431/
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 08/12] security/selinux: Require SGX_MAPWX to map enclave page WX
2019-06-25 20:19 ` Stephen Smalley@ 2019-06-26 12:49 ` Dr. Greg0 siblings, 0 replies; 156+ messages in thread
From: Dr. Greg @ 2019-06-26 12:49 UTC (permalink / raw)
To: Stephen Smalley
Cc: Sean Christopherson, Jarkko Sakkinen, linux-sgx,
linux-security-module, selinux, Bill Roberts, Casey Schaufler,
James Morris, Dave Hansen, Cedric Xing, Andy Lutomirski,
Jethro Beekman
On Tue, Jun 25, 2019 at 04:19:35PM -0400, Stephen Smalley wrote:
Good morning, I hope the week is going well for everyone.
> On 6/19/19 6:23 PM, Sean Christopherson wrote:
> >Hook enclave_map() to require a new per-process capability, SGX_MAPWX,
> >when mapping an enclave as simultaneously writable and executable.
> >Note, @prot contains the actual protection bits that will be set by the
> >kernel, not the maximal protection bits specified by userspace when the
> >page was first loaded into the enclave.
> >
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >---
> > security/selinux/hooks.c | 21 +++++++++++++++++++++
> > security/selinux/include/classmap.h | 3 ++-
> > 2 files changed, 23 insertions(+), 1 deletion(-)
> >
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index 3ec702cf46ca..fc239e541b62 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -6726,6 +6726,23 @@ static void selinux_bpf_prog_free(struct
> >bpf_prog_aux *aux)
> > }
> > #endif
> >
> >+#ifdef CONFIG_INTEL_SGX
> >+static int selinux_enclave_map(unsigned long prot)
> >+{
> >+ const struct cred *cred = current_cred();
> >+ u32 sid = cred_sid(cred);
> >+
> >+ /* SGX is supported only in 64-bit kernels. */
> >+ WARN_ON_ONCE(!default_noexec);
> >+
> >+ if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> >+ return avc_has_perm(&selinux_state, sid, sid,
> >+ SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX,
> >+ NULL);
> Possibly we should use a slightly more general name for the
> permission to allow reusing it in the future if/when another
> architecture introduces a similar construct under a different
> branding? ENCLAVE_* seems slightly more generic than SGX_*.
Perhaps TEE_*, since it is generic and expresses the notion of
privileges specific to an alternate execution environment.
> I was interested in testing this code but sadly the driver reports
> the following on my development workstation:
>
> [ 1.644191] sgx: The launch control MSRs are not writable
> [ 1.695477] sgx: EPC section 0x70200000-0x75f7ffff
> [ 1.771760] sgx: The public key MSRs are not writable
>
> I guess I'm out of luck until/unless I get a NUC or server class
> hardware that supports flexible launch control? Seems developer
> unfriendly.
Indeed.
Most importantly, it is decidedly unfriendly to the future of the
technology on Linux.
More problematically, from a development perspective, the driver is
incompatible with the current Intel runtime, which makes testing at a
level beyond the one page test harness that is included with the
patchset impossible.
As I noted previously, before the LSM discussion, we have a patch that
addresses the compatibility, security and launch control issues the
original version of the driver had. If you missed the thread, it is
available from the following URL:
ftp://ftp.idfusion.net/pub/idfusion/jarkko-master-SFLC.patch
It will be a bit dated by now and doesn't address the API change
needed to set page permissions. It is a pretty solid starting point
if you want to use the existing runtime to do more then trivial
testing.
We have an extension to the existing driver that we will be releasing,
so users of our SRDE will be able to use both the out-of-tree and
in-tree drivers. It also re-establishes launch control and provides a
very simplistic interface to implement ring-0 security for launch
control on flexible launch control platforms.
I'm in Israel right now but we should have a GIT tree against the
current development branches by the weekend. We will be testing the
driver with our SRDE against real world enclaves as we advance the
driver forward.
Have a good day.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC
4206 N. 19th Ave. Implementing measured information privacy
Fargo, ND 58102 and integrity architectures.
PH: 701-281-1686
FAX: 701-281-3949 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"This place is so screwed up. It's just like the Titanic, only
we don't even have a band playing.
-- Terrance George Wieland
Resurrection.
^permalinkrawreply [flat|nested] 156+ messages in thread

*[RFC PATCH v2 0/3] security/x86/sgx: SGX specific LSM hooks
2019-06-19 22:23 [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Sean Christopherson
` (12 preceding siblings ...)
2019-06-21 1:32 ` [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Jarkko Sakkinen
@ 2019-06-27 18:56 ` Cedric Xing
2019-07-03 23:16 ` Jarkko Sakkinen
` (5 more replies)
2019-06-27 18:56 ` [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks Cedric Xing
` (3 subsequent siblings)17 siblings, 6 replies; 156+ messages in thread
From: Cedric Xing @ 2019-06-27 18:56 UTC (permalink / raw)
To: linux-sgx, linux-security-module, selinux
Cc: Cedric Xing, casey.schaufler, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, sean.j.christopherson
This series intends to make the new SGX subsystem to work with the existing LSM
architecture smoothly so that, say, SGX cannot be abused to work around
restrictions set forth by LSM modules/policies.
This patch is based on and could be applied cleanly on top of Jarkko Sakkinen’s
SGX patch series v20 (https://patchwork.kernel.org/cover/10905141/).
For those who haven’t followed closely, the whole discussion started from the
primary question of how to prevent creating an executable enclave page from a
regular memory page that is NOT executable as prohibited by LSM
modules/policies. And that can be translated into 2 relating questions in
practice, i.e. 1) how to determine the allowed initial protections of enclave
pages when they are being loaded and 2) how to determine the allowed
protections of enclave pages at runtime. Those who are familiar with LSM may
notice that, for regular files, #1 is determined by security_mmap_file() while
#2 is covered by security_file_mprotect(). Those 2 hooks however are
insufficient for enclaves due to the distinct composition and lifespan of
enclave pages. Specifically, security_mmap_file() only passes in the file but
is not specific on which portion of the file being mmap()’ed, with the
assumption that all pages of the same file shall have the same set of
allowed/disallowed protections. But that assumption is no longer true for
enclaves for 2 reasons: a) pages of an enclave may be loaded from different
image files with different attributes and b) enclave pages retain contents
across munmap()/mmap(), therefore, say, if a policy prohibits execution of
modified pages, then pages flagged modified have to stay modified across
munmap()/mmap() so that the policy cannot be circumvented by remapping (i.e.
munmap() followed by mmap() on the same range). But the lack of range
information in security_mmap_file()’s arguments simply blocks LSM modules from
tracking enclave pages properly.
A rational solution would always involve tracking the correspondence between
enclave pages and their origin (e.g. files from which they were loaded), which
is similar to tracking regular memory pages and their origin via vm_file of
struct vm_area_struct. But given the longer lifespan of enclave pages (than
VMAs they are mapped into), such correspondence has to be stored in a separate
data structure outside of VMAs. In theory, the correspondence could be stored
either in LSM or in the SGX subsystem. This series has picked the former
because firstly, such information is useful only within LSM so it makes more
sense to keep it as “LSM internal” and secondly, keeping the data structure
inside LSM would allow additional information to be cached in LSM modules
without affecting the rest of the kernel, while lastly, those data structures
would be gone when LSM is disabled hence would not impose any unnecessary
overhead. As you can see in the SELinux implementation of those new hooks
enclosed in this series, options are offered (via kernel command line
parameter) to system administrators between accurate auditing (decisions made
at the time of request) and low memory/performance overhead (decisions
made/cached ahead of time at page instantiation). And that’s one of the
benefits of keeping everything inside LSM.
Those who are familiar with this topic and related discussions may also notice
that, Sean Christopherson has sent out an RFC patch recently to address the
same problem as this series. He adopted the other approach of tracking
page/origin correspondence inside the SGX subsystem. However, to reduce memory
overhead in practice, he cached the FSM (Finite State Machine) instead of
page/origin correspondences. By “FSM”, I mean policy FSM defined as sets of
states and events that may trigger state transitions. Generally speaking, any
LSM module has its own definition of FSM and usually uses attributes attached
to files to argument the FSM, then it advances the FSM as events are observed
and gives out decision based on the current FSM state. Sean’s implementation
attempts to move the FSM into the SGX subsystem, and by caching the arguments
returned by LSM it tries to monitor events and reach the same decisions by
itself. So from architecture perspective, that model has to face tough
challenges in reality, such as how to support multiple LSM modules that employ
different FSMs to govern page protection transitions. Implementation wise, his
model also imposes unwanted restrictions specifically to SGX2, such as:
- Complicated/Restricted UAPI – Enclave loaders are required to provide
“maximal protection” at page load time, but such information is NOT always
available. For example, Graphene containers may run different applications
comprised of different set of executables and/or shared objects. Some of
them may contain self-modifying code (or text relocation) while others
don’t. The generic enclave loader usually doesn’t have such information so
wouldn’t be able to provide it ahead of time.
- Inefficient Auditing – Audit logs are supposed to help system
administrators to determine the set of minimally needed permissions and to
detect abnormal behaviors. But consider the “maximal protection” model, if
“maximal protection” is set to be too permissive, then audit log wouldn’t
be able to detect anomalies; or if “maximal protection” is too restrictive,
then audit log cannot identify the file violating the policy. In either
case the audit log cannot fulfill its purposes.
- Inability to support #PF driven EPC allocation in SGX2 – For those
unfamiliar with SGX2 software flows, an SGX2 enclave requests a page by
issuing EACCEPT on the address that a new page is wanted, and the resulted
#PF is expected to be handled by the kernel by EAUG’ing an EPC page at the
fault address, and then the enclave would be resumed and the faulting
EACCEPT retried, and succeed. The key requirement is to allow mmap()’ing
non-existing enclave pages so that the SGX module/subsystem could respond
to #PFs by EAUG’ing new pages. Sean’s implementation doesn’t allow
mmap()’ing non-existing pages for variety of reasons and therefore blocks
this major SGX2 usage.
History:
- This is version 2 of this patch series, with the following changes per
comments/requests from the community:
+ A new data structure – EMA (Enclave Memory Area) is introduced to track
range/origin correspondences for enclaves. EMAs are maintained by the LSM
framework to be shared among all LSM modules. EMAs are allocated for
enclave files only so will not impose overhead to regular
applications/files.
+ Improved auditing – A new kernel command line option
“lsm.ema.cache_decisions” is introduced, if on, would cause LSM modules
to make/cache decisions at page instantiation (i.e. enclave_load() hook)
instead of at time of request (i.e. file_mprotect() hook), in order to
save memory by NOT keeping enclave source files open. System
administrators are expected to run LSM in permissive mode along with this
option off to figure out the minimal permissions necessary, then turn it
back on in enforcing mode to minimize memory/performance overheads.
+ In the SELinux implementation of the new hooks, FILE__EXECUTE on the file
containing SIGSTRUCT is interpreted as approval for launch, while
FILE__EXECMOD is interpreted as allowing anonymous pages (i.e. pages
EAUG’ed, or EADD’ed from an anonymous source page) to be executable.
Allowed protections for other pages loaded from files are dictated by the
source files’ FILE__EXECUTE/FILE__EXECMOD. This series intentionally
avoids defining new permissions so that user mode tools could continue to
work by treating enclave files the same way as regular executables and/or
shared objects.
- v1 – https://patchwork.kernel.org/cover/10984127/
Cedric Xing (3):
x86/sgx: Add SGX specific LSM hooks
x86/sgx: Call LSM hooks from SGX subsystem/module
x86/sgx: Implement SGX specific hooks in SELinux
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 80 ++++++++-
arch/x86/kernel/cpu/sgx/driver/main.c | 16 +-
include/linux/lsm_ema.h | 171 ++++++++++++++++++
include/linux/lsm_hooks.h | 29 ++++
include/linux/security.h | 23 +++
security/Makefile | 1 +
security/lsm_ema.c | 132 ++++++++++++++
security/security.c | 47 ++++-
security/selinux/hooks.c | 229 ++++++++++++++++++++++++-
security/selinux/include/objsec.h | 24 +++
10 files changed, 732 insertions(+), 20 deletions(-)
create mode 100644 include/linux/lsm_ema.h
create mode 100644 security/lsm_ema.c
--
2.17.1
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-27 23:37 ` Casey Schaufler@ 2019-06-28 0:47 ` Xing, Cedric
2019-06-28 17:22 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-06-28 0:47 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
Cc: Schaufler, Casey, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, Christopherson, Sean J
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Thursday, June 27, 2019 4:37 PM
> >>
> >> This code should not be mixed in with the LSM infrastructure.
> >> It should all be contained in its own module, under security/enclave.
> > lsm_ema is *intended* to be part of the LSM infrastructure.
>
> That's not going to fly, not for a minute.
Why not, if there's a need for it?
And what's the concern here if it becomes part of the LSM infrastructure.
>
> > It is going to be shared among all LSMs that would like to track
> enclave pages and their origins.
>
> That's true for InfiniBand, tun and sctp as well. Look at their
> implementations.
As far as I can tell, InfiniBand, tun and sctp, all of them seemed used inside SELinux only.
If you had a chance to look at v1 of my series, I started by burying everything inside SELinux too. But Stephen pointed out such tracking would be needed by all LSMs so code duplication might be a concern. Thus I responded by moving it into LSM infrastructure.
>
> > And they could be extended to store more information as deemed
> appropriate by the LSM module.
>
> Which is what blobs are for, but that does not appear to be how
> you're using either the file blob or your new ema blob.
A lsm_ema_map pointer is stored in file->f_security. Each lsm_ema_map contains a list of lsm_ema structures. In my last patch, SELinux stores a ema_security_struct with every ema, by setting selinux_blob_sizes.lbs_ema_data to sizeof(ema_security_struct).
ema_security_struct is initialized in selinux_enclave_load(), and checked in enclave_mprotect(), which is a subroutine of selinux_file_mprotect(). BTW, it is alloced/freed automatically by LSM infrastructure in security_enclave_load()/security_file_free().
>
> > The last patch of this series shows how to extend EMA inside SELinux.
>
> I don't see (but I admit the code doesn't make a lot of sense to me)
> anything you couldn't do in the SELinux code by adding data to the
> file blob. The data you're adding to the LSM infrastructure doesn't
> belong there, and it doesn't need to be there.
You are correct. My v1 did it inside SELinux.
The key question I think is whether only SELinux needs it, or all LSMs need it. Stephen thought it was the latter (and I agree with him) so I moved it into the LSM infrastructure to be shared, just like the auditing code.
> >> Not acceptable for the LSM infrastructure. They
> >> are inconsistent with the way data is used there.
> > I'm not sure I understand this comment.
>
> It means that your definition and use of the lsm_ema_blob
> does not match the way other blobs are managed and used.
> The LSM infrastructure uses these entries in a very particular
> way, and you're trying to use it differently. Your might be
> able to change the rest of the enclave system to use it
> correctly, or you might be able to find a different place
> for it.
I'm still not sure why you think this (lbs_ema_data) is inconsistent with other blobs.
Same as all other blobs, an LSM requests it by storing the needed size in it, and is assigned an offset, and the buffer is allocated/freed by the infrastructure. Am I missing anything?
> >
> > As I stated in the cover letter, the primary question is how to
> prevent SGX from being abused as a backdoor to make executable pages
> that would otherwise not be executable without SGX. Any LSM module
> unaware of that would leave that "hole" open. So tracking enclave pages
> will become a common task for all LSMs that care page protections, and
> that's why I place it inside LSM infrastructure.
>
> Page protections are an important part of many security features,
> but that's beside the point. The LSM system provides mechanism for
> providing additional restrictions to existing security mechanisms.
> First, you create the security mechanism (e.g. enclaves) then you
> add LSM hooks so that security modules (e.g. SELinux) can apply
> their own policies in addition. In support of this, the LSM blob
> mechanism allows security modules to maintain their own information
> about the system components (e.g. file, inode, cred, task) they
> care about. The LSM infrastructure does not itself provide or
> support security data or policy. That's strictly for the modules
> to do.
Agreed!
EMA doesn't dictate policies for sure. Is it considered "security data"? I'm not sure the definition of "security data" here. It does store some "data", something that multiple LSM modules would need to duplicate if not pulled into a common place. It is meant to be a "helper" data structure, just like the auditing code.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-06-27 20:19 ` Xing, Cedric@ 2019-06-28 16:16 ` Stephen Smalley
2019-06-28 21:20 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-06-28 16:16 UTC (permalink / raw)
To: Xing, Cedric, Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein
On 6/27/19 4:19 PM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
>> owner@vger.kernel.org] On Behalf Of Stephen Smalley
>> Sent: Tuesday, June 25, 2019 2:10 PM
>>
>> On 6/21/19 5:22 PM, Xing, Cedric wrote:
>>>> From: Christopherson, Sean J
>>>> Sent: Wednesday, June 19, 2019 3:24 PM
>>>>
>>>> Intended use of each permission:
>>>>
>>>> - SGX_EXECDIRTY: dynamically load code within the enclave itself
>>>> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
>>>> Graphene
>>>
>>> Why does it matter whether a code page is measured or not?
>>
>> It won't be incorporated into an attestation?
>
> Yes, it will. And because of that, I don't think LSM should care.
>
>>
>>>
>>>> - SGX_EXECANON: load code from anonymous memory (likely Graphene)
>>>
>>> Graphene doesn't load code from anonymous memory. It loads code
>> dynamically though, as in SGX_EXECDIRTY case.
>>
>> So do we expect EXECANON to never be triggered at all?
>
> I don't think so. And from security perspective, the decision I think shall base on whether the source pages are (allowed to be made) executable.
>
>>
>>>
>>>> - SGX_EXECUTE: load an enclave from a file, i.e. normal behavior
>>>
>>> Why is SGX_EXECUTE needed from security perspective? Or why isn't
>> FILE__EXECUTE sufficient?
>>
>> Splitting the SGX permissions from the regular ones allows distinctions
>> to be made between what can be executed in the host process and what can
>> be executed in the enclave. The host process may be allowed
>> FILE__EXECUTE to numerous files that do not contain any code ever
>> intended to be executed within the enclave.
>
> Given an enclave and its host process, any executable contents could be allowed in
> 1) Neither the enclave nor the host
> 2) Enclave only
> 3) Host only
> 4) Both the enclave and the host
>
> Given the fact that enclave can access host's memory, if a piece of code is NOT allowed in the host, then it shouldn't be allowed in enclave either. So #2 shall never happen.
>
> An enclave dictates/enforces its own contents cryptographically, so it's unnecessary to enforce #3 by LSM IMO.
>
> Then #1 and #4 are the only 2 cases to be supported - a single FILE__EXECUTE is sufficient.
>
> I'm not objecting to new permissions to make things more explicit, but that'd require updates to user mode tools. I think it just easier to reuse existing permissions.
FWIW, adding new permissions only requires updating policy
configuration, not userspace code/tools. But in any event, we can reuse
the execute-related permissions if it makes sense but still consider
introducing additional, new permissions, possibly in a separate
"enclave" security class, if we want explicit control over enclave
loading, e.g. ENCLAVE__LOAD, ENCLAVE__INIT, etc.
One residual concern I have with the reuse of FILE__EXECUTE is using it
for the sigstruct file as the fallback case. If the sigstruct is always
part of the same file as the code, then it probably doesn't matter. But
otherwise, it is somewhat odd to have to allow the host process to
execute from the sigstruct file if it is only data (the signature).
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-28 0:47 ` Xing, Cedric@ 2019-06-28 17:22 ` Casey Schaufler
2019-06-28 22:29 ` Xing, Cedric
2019-06-29 1:37 ` Stephen Smalley0 siblings, 2 replies; 156+ messages in thread
From: Casey Schaufler @ 2019-06-28 17:22 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
Cc: Schaufler, Casey, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, Christopherson, Sean J
On 6/27/2019 5:47 PM, Xing, Cedric wrote:
>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>> Sent: Thursday, June 27, 2019 4:37 PM
>>>> This code should not be mixed in with the LSM infrastructure.
>>>> It should all be contained in its own module, under security/enclave.
>>> lsm_ema is *intended* to be part of the LSM infrastructure.
>> That's not going to fly, not for a minute.
> Why not, if there's a need for it?
>
> And what's the concern here if it becomes part of the LSM infrastructure.
The LSM infrastructure provides a framework for hooks
and allocation of blobs. That's it. It's a layer for
connecting system features like VFS, IPC and the IP stack
to the security modules. It does not implement any policy
of it's own. We are not going to implement SGX or any other
mechanism within the LSM infrastructure.
>>> It is going to be shared among all LSMs that would like to track
>> enclave pages and their origins.
>>
>> That's true for InfiniBand, tun and sctp as well. Look at their
>> implementations.
> As far as I can tell, InfiniBand, tun and sctp, all of them seemed used inside SELinux only.
So?
> If you had a chance to look at v1 of my series, I started by burying everything inside SELinux too. But Stephen pointed out such tracking would be needed by all LSMs so code duplication might be a concern. Thus I responded by moving it into LSM infrastructure.
What you need to do is move all the lsm_ema code into its own
place (which could be security/enclave). Manage your internal
data as you like. LSMs (e.g. SELinux) can call your APIs if
needed. If the LSMs need to store SGX information with the file
structure they need to include that in the space they ask for in
the file blob.
>>> And they could be extended to store more information as deemed
>> appropriate by the LSM module.
>>
>> Which is what blobs are for, but that does not appear to be how
>> you're using either the file blob or your new ema blob.
> A lsm_ema_map pointer is stored in file->f_security.
That's up to the individual security module to decide.
> Each lsm_ema_map contains a list of lsm_ema structures. In my last patch, SELinux stores a ema_security_struct with every ema, by setting selinux_blob_sizes.lbs_ema_data to sizeof(ema_security_struct).
You are managing the ema map lists. You don't need the LSM
infrastructure to do that.
> ema_security_struct is initialized in selinux_enclave_load(), and checked in enclave_mprotect(), which is a subroutine of selinux_file_mprotect(). BTW, it is alloced/freed automatically by LSM infrastructure in security_enclave_load()/security_file_free().
Do you mean security_enclave_load()/security_enclave_free() ?
There is no way you can possibly have sane behavior if you're
allocation and free aren't tied to the same blob.
>>> The last patch of this series shows how to extend EMA inside SELinux.
>> I don't see (but I admit the code doesn't make a lot of sense to me)
>> anything you couldn't do in the SELinux code by adding data to the
>> file blob. The data you're adding to the LSM infrastructure doesn't
>> belong there, and it doesn't need to be there.
> You are correct. My v1 did it inside SELinux.
>
> The key question I think is whether only SELinux needs it, or all LSMs need it. Stephen thought it was the latter (and I agree with him) so I moved it into the LSM infrastructure to be shared, just like the auditing code.
You are both right that it doesn't belong in the SELinux code.
It also doesn't belong as part of the LSM infrastructure.
>>>> Not acceptable for the LSM infrastructure. They
>>>> are inconsistent with the way data is used there.
>>> I'm not sure I understand this comment.
>> It means that your definition and use of the lsm_ema_blob
>> does not match the way other blobs are managed and used.
>> The LSM infrastructure uses these entries in a very particular
>> way, and you're trying to use it differently. Your might be
>> able to change the rest of the enclave system to use it
>> correctly, or you might be able to find a different place
>> for it.
> I'm still not sure why you think this (lbs_ema_data) is inconsistent with other blobs.
>
> Same as all other blobs, an LSM requests it by storing the needed size in it, and is assigned an offset, and the buffer is allocated/freed by the infrastructure. Am I missing anything?
Yes. Aside from allocation and deletion the infrastructure does
nothing with the blobs. The blobs are used only by the security
modules. All other data is maintained and used elsewhere. SGX
specific data needs to me maintained and managed elsewhere.
>>> As I stated in the cover letter, the primary question is how to
>> prevent SGX from being abused as a backdoor to make executable pages
>> that would otherwise not be executable without SGX. Any LSM module
>> unaware of that would leave that "hole" open. So tracking enclave pages
>> will become a common task for all LSMs that care page protections, and
>> that's why I place it inside LSM infrastructure.
>>
>> Page protections are an important part of many security features,
>> but that's beside the point. The LSM system provides mechanism for
>> providing additional restrictions to existing security mechanisms.
>> First, you create the security mechanism (e.g. enclaves) then you
>> add LSM hooks so that security modules (e.g. SELinux) can apply
>> their own policies in addition. In support of this, the LSM blob
>> mechanism allows security modules to maintain their own information
>> about the system components (e.g. file, inode, cred, task) they
>> care about. The LSM infrastructure does not itself provide or
>> support security data or policy. That's strictly for the modules
>> to do.
> Agreed!
>
> EMA doesn't dictate policies for sure. Is it considered "security data"? I'm not sure the definition of "security data" here. It does store some "data", something that multiple LSM modules would need to duplicate if not pulled into a common place. It is meant to be a "helper" data structure, just like the auditing code.
Good example. You'll see that there is no audit code in the
LSM infrastructure. None. No audit data, either. It's all taken
care of in the audit system.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-06-28 16:16 ` Stephen Smalley@ 2019-06-28 21:20 ` Xing, Cedric
2019-06-29 1:15 ` Stephen Smalley0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-06-28 21:20 UTC (permalink / raw)
To: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Friday, June 28, 2019 9:17 AM
>
> FWIW, adding new permissions only requires updating policy configuration,
> not userspace code/tools. But in any event, we can reuse the execute-
> related permissions if it makes sense but still consider introducing
> additional, new permissions, possibly in a separate "enclave" security
> class, if we want explicit control over enclave loading, e.g.
> ENCLAVE__LOAD, ENCLAVE__INIT, etc.
I'm not so familiar with SELinux tools so my apology in advance if I end up mixing up things.
I'm not only talking about the new permissions, but also how to apply them to enclave files. Intel SGX SDK packages enclaves as .so files, and I guess that's the most straight forward way that most others would do. So if different permissions are defined, then user mode tools would have to distinguish enclaves from regular .so files in order to grant them different permissions. Would that be something extra to existing tools?
>
> One residual concern I have with the reuse of FILE__EXECUTE is using it
> for the sigstruct file as the fallback case. If the sigstruct is always
> part of the same file as the code, then it probably doesn't matter. But
> otherwise, it is somewhat odd to have to allow the host process to
> execute from the sigstruct file if it is only data (the signature).
I agree with you. But do you think it a practical problem today? As far as I know, no one is deploying sigstructs in dedicated files. I'm just trying to touch as few things as possible until there's definitely a need to do so.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-28 16:37 ` Stephen Smalley@ 2019-06-28 21:53 ` Xing, Cedric
2019-06-29 1:22 ` Stephen Smalley0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-06-28 21:53 UTC (permalink / raw)
To: Stephen Smalley, linux-sgx, linux-security-module, selinux
Cc: Schaufler, Casey, jmorris, luto, jethro, greg, jarkko.sakkinen,
Christopherson, Sean J
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Friday, June 28, 2019 9:37 AM
>
> > lsm.ema.cache_decisions is on by default and
> > could be turned off by appending “lsm.ema.cache_decisions=0” or
> > “lsm.ema.cache_decisions=off” to the kernel command line.
>
> This seems problematic on a few fronts:
>
> - Specifying it as a boot parameter requires teaching admins / policy
> developers to do something in addition to what they are already doing
> when they want to create policy,
>
> - Limiting it to a boot parameter requires a reboot to change the mode
> of operation, whereas SELinux offers runtime toggling of permissive mode
> and even per-process (domain) permissive mode (and so does AppArmor),
How about making a variable tunable via sysctl?
>
> - In the cache_decisions=1 case, do we get any auditing at all? If not,
> that's a problem. We want auditing not only when we are
> generating/learning policy but also in operation.
Currently it doesn't generate audit log, but I could add it, except it couldn't point to the exact file. But I can use the sigstruct file instead so administrators can at least tell which enclave violates the policy. Do you think it acceptable?
>
> - There is the potential for inconsistencies to arise between the
> enforcement applied with different cache_decisions values.
The enforcement will be consistent. The difference only lies in the logs.
>
> I would suggest that we just never cache the decision and accept the
> cost if we are going to take this approach.
This will also be a viable option. I don't think any enclaves would be comprised of a large number of files anyway. When SGX2 comes up, I think most enclaves will be instantiated from only one file and defer loading libraries at runtime. So in practice we are looking to keeping only one file open per enclave, which seems totally acceptable.
Stephen (and everyone having an opinion on this), which way do you prefer? sysctl variable? Or never cache decisions?
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-28 17:22 ` Casey Schaufler@ 2019-06-28 22:29 ` Xing, Cedric
2019-06-29 1:37 ` Stephen Smalley1 sibling, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-06-28 22:29 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
Cc: Schaufler, Casey, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, Christopherson, Sean J
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Friday, June 28, 2019 10:22 AM
> >
> > And what's the concern here if it becomes part of the LSM
> infrastructure.
>
> The LSM infrastructure provides a framework for hooks
> and allocation of blobs. That's it. It's a layer for
> connecting system features like VFS, IPC and the IP stack
> to the security modules. It does not implement any policy
> of it's own. We are not going to implement SGX or any other
> mechanism within the LSM infrastructure.
EMA doesn't force/implement any policy either. It just supplements VMA.
>
> >>> It is going to be shared among all LSMs that would like to track
> >> enclave pages and their origins.
> >>
> >> That's true for InfiniBand, tun and sctp as well. Look at their
> >> implementations.
> > As far as I can tell, InfiniBand, tun and sctp, all of them seemed
> used inside SELinux only.
>
> So?
So they are NOT shared among LSMs, which are different than EMA.
>
> > If you had a chance to look at v1 of my series, I started by burying
> everything inside SELinux too. But Stephen pointed out such tracking
> would be needed by all LSMs so code duplication might be a concern. Thus
> I responded by moving it into LSM infrastructure.
>
> What you need to do is move all the lsm_ema code into its own
> place (which could be security/enclave). Manage your internal
> data as you like. LSMs (e.g. SELinux) can call your APIs if
> needed. If the LSMs need to store SGX information with the file
> structure they need to include that in the space they ask for in
> the file blob.
I thought subdirectories were for LSM modules. EMA is more like auditing code, which has a header in include/linux/ and an implementation in security/. Is that right?
>
>
> >>> And they could be extended to store more information as deemed
> >> appropriate by the LSM module.
> >>
> >> Which is what blobs are for, but that does not appear to be how
> >> you're using either the file blob or your new ema blob.
> > A lsm_ema_map pointer is stored in file->f_security.
>
> That's up to the individual security module to decide.
That's doable. The drawback is, if there are N LSM modules active, then the same information will be duplicated N times. Will that be a problem?
>
> > Each lsm_ema_map contains a list of lsm_ema structures. In my last
> patch, SELinux stores a ema_security_struct with every ema, by setting
> selinux_blob_sizes.lbs_ema_data to sizeof(ema_security_struct).
>
> You are managing the ema map lists. You don't need the LSM
> infrastructure to do that.
>
> > ema_security_struct is initialized in selinux_enclave_load(), and
> checked in enclave_mprotect(), which is a subroutine of
> selinux_file_mprotect(). BTW, it is alloced/freed automatically by LSM
> infrastructure in security_enclave_load()/security_file_free().
>
> Do you mean security_enclave_load()/security_enclave_free() ?
> There is no way you can possibly have sane behavior if you're
> allocation and free aren't tied to the same blob.
There's no security_*enclave*_free(). lsm_ema_map is allocated only for enclaves. But LSM doesn't know which file is an enclave, so the allocation is deferred until the first security_enclave_load(). security_file_free() frees the map if it isn't NULL.
>
> >>> The last patch of this series shows how to extend EMA inside
> SELinux.
> >> I don't see (but I admit the code doesn't make a lot of sense to me)
> >> anything you couldn't do in the SELinux code by adding data to the
> >> file blob. The data you're adding to the LSM infrastructure doesn't
> >> belong there, and it doesn't need to be there.
> > You are correct. My v1 did it inside SELinux.
> >
> > The key question I think is whether only SELinux needs it, or all LSMs
> need it. Stephen thought it was the latter (and I agree with him) so I
> moved it into the LSM infrastructure to be shared, just like the
> auditing code.
>
> You are both right that it doesn't belong in the SELinux code.
> It also doesn't belong as part of the LSM infrastructure.
Then what is your suggestion?
Is the code in security_enclave_load()/security_file_free() that bothers you? Because you think they shouldn't do anything more than just a single line of call_int/void_hooks()?
>
> >>>> Not acceptable for the LSM infrastructure. They
> >>>> are inconsistent with the way data is used there.
> >>> I'm not sure I understand this comment.
> >> It means that your definition and use of the lsm_ema_blob
> >> does not match the way other blobs are managed and used.
> >> The LSM infrastructure uses these entries in a very particular
> >> way, and you're trying to use it differently. Your might be
> >> able to change the rest of the enclave system to use it
> >> correctly, or you might be able to find a different place
> >> for it.
> > I'm still not sure why you think this (lbs_ema_data) is inconsistent
> with other blobs.
> >
> > Same as all other blobs, an LSM requests it by storing the needed size
> in it, and is assigned an offset, and the buffer is allocated/freed by
> the infrastructure. Am I missing anything?
>
> Yes. Aside from allocation and deletion the infrastructure does
> nothing with the blobs. The blobs are used only by the security
> modules. All other data is maintained and used elsewhere. SGX
> specific data needs to me maintained and managed elsewhere.
>
> >>> As I stated in the cover letter, the primary question is how to
> >> prevent SGX from being abused as a backdoor to make executable pages
> >> that would otherwise not be executable without SGX. Any LSM module
> >> unaware of that would leave that "hole" open. So tracking enclave
> pages
> >> will become a common task for all LSMs that care page protections,
> and
> >> that's why I place it inside LSM infrastructure.
> >>
> >> Page protections are an important part of many security features,
> >> but that's beside the point. The LSM system provides mechanism for
> >> providing additional restrictions to existing security mechanisms.
> >> First, you create the security mechanism (e.g. enclaves) then you
> >> add LSM hooks so that security modules (e.g. SELinux) can apply
> >> their own policies in addition. In support of this, the LSM blob
> >> mechanism allows security modules to maintain their own information
> >> about the system components (e.g. file, inode, cred, task) they
> >> care about. The LSM infrastructure does not itself provide or
> >> support security data or policy. That's strictly for the modules
> >> to do.
> > Agreed!
> >
> > EMA doesn't dictate policies for sure. Is it considered "security
> data"? I'm not sure the definition of "security data" here. It does
> store some "data", something that multiple LSM modules would need to
> duplicate if not pulled into a common place. It is meant to be a
> "helper" data structure, just like the auditing code.
>
> Good example. You'll see that there is no audit code in the
> LSM infrastructure. None. No audit data, either. It's all taken
> care of in the audit system.
Did you mean security/security.c didn't call into any audit functions? lsm_audit.c is located in security/ and its header in include/linux/ but you don't have a problem with them. Am I right?
IIUC, you want me to pack whatever inside security_enclave_load()/security_file_free() into some APIs to be called by individual LSM modules. But if you can pay a bit more attention to the code, an EMA can be inserted to the map only after *all* LSM modules have approved it. So if it is spread into individual LSMs and if there are multiple active LSMs, there could be inconsistence among LSMs if they each maintains its own map and makes different decisions on the same EMA at enclave_load(). I'm not saying that's unresolvable but definitely more error prone, besides wasting memory. Or do you have any practical suggestions?
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-06-28 21:20 ` Xing, Cedric@ 2019-06-29 1:15 ` Stephen Smalley
2019-07-01 18:14 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-06-29 1:15 UTC (permalink / raw)
To: Xing, Cedric
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein
On Fri, Jun 28, 2019 at 5:20 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > Sent: Friday, June 28, 2019 9:17 AM
> >
> > FWIW, adding new permissions only requires updating policy configuration,
> > not userspace code/tools. But in any event, we can reuse the execute-
> > related permissions if it makes sense but still consider introducing
> > additional, new permissions, possibly in a separate "enclave" security
> > class, if we want explicit control over enclave loading, e.g.
> > ENCLAVE__LOAD, ENCLAVE__INIT, etc.
>
> I'm not so familiar with SELinux tools so my apology in advance if I end up mixing up things.
>
> I'm not only talking about the new permissions, but also how to apply them to enclave files. Intel SGX SDK packages enclaves as .so files, and I guess that's the most straight forward way that most others would do. So if different permissions are defined, then user mode tools would have to distinguish enclaves from regular .so files in order to grant them different permissions. Would that be something extra to existing tools?
It doesn't require any userspace code changes. It is just a matter of
defining some configuration data in the policy for the new
permissions, one or more security labels (tags) for the SGX .so files,
and rules allowing access where desired, and then setting those
security labels on the SGX .so files (via the security.selinux
extended attribute on the files). Even the last part is generally
handled by updating a configuration specifying how files should be
labeled and then rpm automatically labels the files when created, or
you can manually restorecon them. If the new permissions are defined
in their own security class rather than reusing existing ones, then
they can even be defined entirely via a local or third party policy
module separate from the distro policy if desired/needed.
>
> >
> > One residual concern I have with the reuse of FILE__EXECUTE is using it
> > for the sigstruct file as the fallback case. If the sigstruct is always
> > part of the same file as the code, then it probably doesn't matter. But
> > otherwise, it is somewhat odd to have to allow the host process to
> > execute from the sigstruct file if it is only data (the signature).
>
> I agree with you. But do you think it a practical problem today? As far as I know, no one is deploying sigstructs in dedicated files. I'm just trying to touch as few things as possible until there's definitely a need to do so.
I don't know, and it wasn't clear to me from the earlier discussions.
If not and if it is acceptable to require them to be in files in the
first place, then perhaps it isn't necessary.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-28 21:53 ` Xing, Cedric@ 2019-06-29 1:22 ` Stephen Smalley
2019-07-01 18:02 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-06-29 1:22 UTC (permalink / raw)
To: Xing, Cedric
Cc: Stephen Smalley, linux-sgx, linux-security-module, selinux,
Schaufler, Casey, jmorris, luto, jethro, greg, jarkko.sakkinen,
Christopherson, Sean J
On Fri, Jun 28, 2019 at 5:54 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > Sent: Friday, June 28, 2019 9:37 AM
> >
> > > lsm.ema.cache_decisions is on by default and
> > > could be turned off by appending “lsm.ema.cache_decisions=0” or
> > > “lsm.ema.cache_decisions=off” to the kernel command line.
> >
> > This seems problematic on a few fronts:
> >
> > - Specifying it as a boot parameter requires teaching admins / policy
> > developers to do something in addition to what they are already doing
> > when they want to create policy,
> >
> > - Limiting it to a boot parameter requires a reboot to change the mode
> > of operation, whereas SELinux offers runtime toggling of permissive mode
> > and even per-process (domain) permissive mode (and so does AppArmor),
>
> How about making a variable tunable via sysctl?
Better than a boot parameter but still not amenable to per-domain
permissive and still requires admins
to remember and perform an extra step before collecting denials.
>
> >
> > - In the cache_decisions=1 case, do we get any auditing at all? If not,
> > that's a problem. We want auditing not only when we are
> > generating/learning policy but also in operation.
>
> Currently it doesn't generate audit log, but I could add it, except it couldn't point to the exact file. But I can use the sigstruct file instead so administrators can at least tell which enclave violates the policy. Do you think it acceptable?
Seems prone to user confusion and lacks precision in why the denial occurred.
>
> >
> > - There is the potential for inconsistencies to arise between the
> > enforcement applied with different cache_decisions values.
>
> The enforcement will be consistent. The difference only lies in the logs.
>
> >
> > I would suggest that we just never cache the decision and accept the
> > cost if we are going to take this approach.
>
> This will also be a viable option. I don't think any enclaves would be comprised of a large number of files anyway. When SGX2 comes up, I think most enclaves will be instantiated from only one file and defer loading libraries at runtime. So in practice we are looking to keeping only one file open per enclave, which seems totally acceptable.
>
> Stephen (and everyone having an opinion on this), which way do you prefer? sysctl variable? Or never cache decisions?
I'd favor never caching decisions.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-28 17:22 ` Casey Schaufler
2019-06-28 22:29 ` Xing, Cedric@ 2019-06-29 1:37 ` Stephen Smalley
2019-06-29 21:35 ` Casey Schaufler1 sibling, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-06-29 1:37 UTC (permalink / raw)
To: Casey Schaufler
Cc: Xing, Cedric, linux-sgx, linux-security-module, selinux,
Schaufler, Casey, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, Christopherson, Sean J
On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
> >> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> >> Sent: Thursday, June 27, 2019 4:37 PM
> >>>> This code should not be mixed in with the LSM infrastructure.
> >>>> It should all be contained in its own module, under security/enclave.
> >>> lsm_ema is *intended* to be part of the LSM infrastructure.
> >> That's not going to fly, not for a minute.
> > Why not, if there's a need for it?
> >
> > And what's the concern here if it becomes part of the LSM infrastructure.
>
> The LSM infrastructure provides a framework for hooks
> and allocation of blobs. That's it. It's a layer for
> connecting system features like VFS, IPC and the IP stack
> to the security modules. It does not implement any policy
> of it's own. We are not going to implement SGX or any other
> mechanism within the LSM infrastructure.
I don't think you understand the purpose of this code. It isn't
implementing SGX, nor is it needed by SGX.
It is providing shared infrastructure for security modules, similar to
lsm_audit.c, so that security modules can enforce W^X or similar
memory protection guarantees for SGX enclave memory, which has unique
properties that render the existing mmap and mprotect hooks
insufficient. They can certainly implement it only for SELinux, but
then any other security module that wants to provide such guarantees
will have to replicate that code.
>
> >>> It is going to be shared among all LSMs that would like to track
> >> enclave pages and their origins.
> >>
> >> That's true for InfiniBand, tun and sctp as well. Look at their
> >> implementations.
> > As far as I can tell, InfiniBand, tun and sctp, all of them seemed used inside SELinux only.
>
> So?
>
> > If you had a chance to look at v1 of my series, I started by burying everything inside SELinux too. But Stephen pointed out such tracking would be needed by all LSMs so code duplication might be a concern. Thus I responded by moving it into LSM infrastructure.
>
> What you need to do is move all the lsm_ema code into its own
> place (which could be security/enclave). Manage your internal
> data as you like. LSMs (e.g. SELinux) can call your APIs if
> needed. If the LSMs need to store SGX information with the file
> structure they need to include that in the space they ask for in
> the file blob.
>
>
> >>> And they could be extended to store more information as deemed
> >> appropriate by the LSM module.
> >>
> >> Which is what blobs are for, but that does not appear to be how
> >> you're using either the file blob or your new ema blob.
> > A lsm_ema_map pointer is stored in file->f_security.
>
> That's up to the individual security module to decide.
>
> > Each lsm_ema_map contains a list of lsm_ema structures. In my last patch, SELinux stores a ema_security_struct with every ema, by setting selinux_blob_sizes.lbs_ema_data to sizeof(ema_security_struct).
>
> You are managing the ema map lists. You don't need the LSM
> infrastructure to do that.
>
> > ema_security_struct is initialized in selinux_enclave_load(), and checked in enclave_mprotect(), which is a subroutine of selinux_file_mprotect(). BTW, it is alloced/freed automatically by LSM infrastructure in security_enclave_load()/security_file_free().
>
> Do you mean security_enclave_load()/security_enclave_free() ?
> There is no way you can possibly have sane behavior if you're
> allocation and free aren't tied to the same blob.
>
> >>> The last patch of this series shows how to extend EMA inside SELinux.
> >> I don't see (but I admit the code doesn't make a lot of sense to me)
> >> anything you couldn't do in the SELinux code by adding data to the
> >> file blob. The data you're adding to the LSM infrastructure doesn't
> >> belong there, and it doesn't need to be there.
> > You are correct. My v1 did it inside SELinux.
> >
> > The key question I think is whether only SELinux needs it, or all LSMs need it. Stephen thought it was the latter (and I agree with him) so I moved it into the LSM infrastructure to be shared, just like the auditing code.
>
> You are both right that it doesn't belong in the SELinux code.
> It also doesn't belong as part of the LSM infrastructure.
>
> >>>> Not acceptable for the LSM infrastructure. They
> >>>> are inconsistent with the way data is used there.
> >>> I'm not sure I understand this comment.
> >> It means that your definition and use of the lsm_ema_blob
> >> does not match the way other blobs are managed and used.
> >> The LSM infrastructure uses these entries in a very particular
> >> way, and you're trying to use it differently. Your might be
> >> able to change the rest of the enclave system to use it
> >> correctly, or you might be able to find a different place
> >> for it.
> > I'm still not sure why you think this (lbs_ema_data) is inconsistent with other blobs.
> >
> > Same as all other blobs, an LSM requests it by storing the needed size in it, and is assigned an offset, and the buffer is allocated/freed by the infrastructure. Am I missing anything?
>
> Yes. Aside from allocation and deletion the infrastructure does
> nothing with the blobs. The blobs are used only by the security
> modules. All other data is maintained and used elsewhere. SGX
> specific data needs to me maintained and managed elsewhere.
>
> >>> As I stated in the cover letter, the primary question is how to
> >> prevent SGX from being abused as a backdoor to make executable pages
> >> that would otherwise not be executable without SGX. Any LSM module
> >> unaware of that would leave that "hole" open. So tracking enclave pages
> >> will become a common task for all LSMs that care page protections, and
> >> that's why I place it inside LSM infrastructure.
> >>
> >> Page protections are an important part of many security features,
> >> but that's beside the point. The LSM system provides mechanism for
> >> providing additional restrictions to existing security mechanisms.
> >> First, you create the security mechanism (e.g. enclaves) then you
> >> add LSM hooks so that security modules (e.g. SELinux) can apply
> >> their own policies in addition. In support of this, the LSM blob
> >> mechanism allows security modules to maintain their own information
> >> about the system components (e.g. file, inode, cred, task) they
> >> care about. The LSM infrastructure does not itself provide or
> >> support security data or policy. That's strictly for the modules
> >> to do.
> > Agreed!
> >
> > EMA doesn't dictate policies for sure. Is it considered "security data"? I'm not sure the definition of "security data" here. It does store some "data", something that multiple LSM modules would need to duplicate if not pulled into a common place. It is meant to be a "helper" data structure, just like the auditing code.
>
> Good example. You'll see that there is no audit code in the
> LSM infrastructure. None. No audit data, either. It's all taken
> care of in the audit system.
>
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-29 1:37 ` Stephen Smalley@ 2019-06-29 21:35 ` Casey Schaufler
2019-07-01 17:57 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-06-29 21:35 UTC (permalink / raw)
To: Stephen Smalley, casey
Cc: Xing, Cedric, linux-sgx, linux-security-module, selinux,
Schaufler, Casey, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, Christopherson, Sean J
On 6/28/2019 6:37 PM, Stephen Smalley wrote:
> On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
>>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>>>> Sent: Thursday, June 27, 2019 4:37 PM
>>>>>> This code should not be mixed in with the LSM infrastructure.
>>>>>> It should all be contained in its own module, under security/enclave.
>>>>> lsm_ema is *intended* to be part of the LSM infrastructure.
>>>> That's not going to fly, not for a minute.
>>> Why not, if there's a need for it?
>>>
>>> And what's the concern here if it becomes part of the LSM infrastructure.
>> The LSM infrastructure provides a framework for hooks
>> and allocation of blobs. That's it. It's a layer for
>> connecting system features like VFS, IPC and the IP stack
>> to the security modules. It does not implement any policy
>> of it's own. We are not going to implement SGX or any other
>> mechanism within the LSM infrastructure.
> I don't think you understand the purpose of this code. It isn't
> implementing SGX, nor is it needed by SGX.
> It is providing shared infrastructure for security modules, similar to
> lsm_audit.c, so that security modules can enforce W^X or similar
> memory protection guarantees for SGX enclave memory, which has unique
> properties that render the existing mmap and mprotect hooks
> insufficient. They can certainly implement it only for SELinux, but
> then any other security module that wants to provide such guarantees
> will have to replicate that code.
I am not objecting to the purpose of the code.
I *am* objecting to calling it part of the LSM infrastructure.
It needs to be it's own thing, off somewhere else.
It must not use the lsm_ prefix. That's namespace pollution.
The code must not be embedded in the LSM infrastructure code,
that breaks with how everything else works.
... and the notion that you allocate data for one blob
that gets freed relative to another breaks the data management
model.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-27 18:56 ` [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks Cedric Xing
2019-06-27 22:06 ` Casey Schaufler
2019-06-28 16:37 ` Stephen Smalley@ 2019-06-29 23:46 ` Andy Lutomirski
2019-07-01 17:11 ` Xing, Cedric2 siblings, 1 reply; 156+ messages in thread
From: Andy Lutomirski @ 2019-06-29 23:46 UTC (permalink / raw)
To: Cedric Xing
Cc: linux-sgx, LSM List, selinux, Schaufler, Casey, James Morris,
Andrew Lutomirski, Jethro Beekman, Dr. Greg Wettstein,
Stephen Smalley, Jarkko Sakkinen, Christopherson, Sean J
On Thu, Jun 27, 2019 at 11:56 AM Cedric Xing <cedric.xing@intel.com> wrote:
>
> SGX enclaves are loaded from pages in regular memory. Given the ability to
> create executable pages, the newly added SGX subsystem may present a backdoor
> for adversaries to circumvent LSM policies, such as creating an executable
> enclave page from a modified regular page that would otherwise not be made
> executable as prohibited by LSM. Therefore arises the primary question of
> whether an enclave page should be allowed to be created from a given source
> page in regular memory.
>
> A related question is whether to grant/deny a mprotect() request on a given
> enclave page/range. mprotect() is traditionally covered by
> security_file_mprotect() hook, however, enclave pages have a different lifespan
> than either MAP_PRIVATE or MAP_SHARED. Particularly, MAP_PRIVATE pages have the
> same lifespan as the VMA while MAP_SHARED pages have the same lifespan as the
> backing file (on disk), but enclave pages have the lifespan of the enclave’s
> file descriptor. For example, enclave pages could be munmap()’ed then mmap()’ed
> again without losing contents (like MAP_SHARED), but all enclave pages will be
> lost once its file descriptor has been closed (like MAP_PRIVATE). That said,
> LSM modules need some new data structure for tracking protections of enclave
> pages/ranges so that they can make proper decisions at mmap()/mprotect()
> syscalls.
>
> The last question, which is orthogonal to the 2 above, is whether or not to
> allow a given enclave to launch/run. Enclave pages are not visible to the rest
> of the system, so to some extent offer a better place for malicious software to
> hide. Thus, it is sometimes desirable to whitelist/blacklist enclaves by their
> measurements, signing public keys, or image files.
>
> To address the questions above, 2 new LSM hooks are added for enclaves.
> - security_enclave_load() – This hook allows LSM to decide whether or not to
> allow instantiation of a range of enclave pages using the specified VMA. It
> is invoked when a range of enclave pages is about to be loaded. It serves 3
> purposes: 1) indicate to LSM that the file struct in subject is an enclave;
> 2) allow LSM to decide whether or not to instantiate those pages and 3)
> allow LSM to initialize internal data structures for tracking
> origins/protections of those pages.
> - security_enclave_init() – This hook allows whitelisting/blacklisting or
> performing whatever checks deemed appropriate before an enclave is allowed
> to run. An LSM module may opt to use the file backing the SIGSTRUCT as a
> proxy to dictate allowed protections for anonymous pages.
>
> mprotect() of enclave pages continue to be governed by
> security_file_mprotect(), with the expectation that LSM is able to distinguish
> between regular and enclave pages inside the hook. For mmap(), the SGX
> subsystem is expected to invoke security_file_mprotect() explicitly to check
> protections against the requested protections for existing enclave pages. As
> stated earlier, enclave pages have different lifespan than the existing
> MAP_PRIVATE and MAP_SHARED pages, so would require a new data structure outside
> of VMA to track their protections and/or origins. Enclave Memory Area (or EMA
> for short) has been introduced to address the need. EMAs are maintained by the
> LSM framework for all LSM modules to share. EMAs will be instantiated for
> enclaves only so will not impose memory/performance overheads for regular
> applications/files. Please see include/linux/lsm_ema.h and security/lsm_ema.c
> for details.
>
> A new setup parameter – lsm.ema.cache_decisions has been introduced to offer
> the choice between memory consumption and accuracy of audit logs. Enabling
> lsm.ema.cache_decisions causes LSM framework NOT to keep backing files open for
> EMAs. While that saves memory, it requires LSM modules to make and cache
> decisions ahead of time, and makes it difficult for LSM modules to generate
> accurate audit logs. System administrators are expected to run LSM in
> permissive mode with lsm.ema.cache_decisions off to determine the minimal
> permissions needed, and then turn it back on in enforcing mode for optimal
> performance and memory usage. lsm.ema.cache_decisions is on by default and
> could be turned off by appending “lsm.ema.cache_decisions=0” or
> “lsm.ema.cache_decisions=off” to the kernel command line.
Just on a very cursory review, this seems like it's creating a bunch
of complexity (a whole new library and data structure), and I'm not
convinced the result is any better than Sean's version.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-29 23:46 ` Andy Lutomirski@ 2019-07-01 17:11 ` Xing, Cedric
2019-07-01 17:58 ` Andy Lutomirski0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 17:11 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx, LSM List, selinux, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
Hi Andy,
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Saturday, June 29, 2019 4:47 PM
>
> Just on a very cursory review, this seems like it's creating a bunch of
> complexity (a whole new library and data structure), and I'm not
> convinced the result is any better than Sean's version.
The new EMA data structure is to track enclave pages by range. Yes, Sean avoided that by storing similar information in the existing encl_page structure inside SGX subsystem. But as I pointed out, his code has to iterate through *every* page in range so mprotect() will be very slow if the range is large. So he would end up introducing something similar to achieve the same performance.
And that's not the most important point. The major problem in his patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or he'd have to amend his code accordingly, which will add complexity and tip your scale).
Other weird things, such as mmap()'ing page by page vs. mmap()'ing the whole range will impact subsequent mprotect()'s as you have noticed, don't exist in my series.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-06-29 23:41 ` Andy Lutomirski@ 2019-07-01 17:46 ` Xing, Cedric
2019-07-01 17:53 ` Andy Lutomirski0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 17:46 UTC (permalink / raw)
To: Andy Lutomirski, Stephen Smalley
Cc: Christopherson, Sean J, Jarkko Sakkinen, linux-sgx,
linux-security-module, selinux, Roberts, William C, Schaufler,
Casey, James Morris, Hansen, Dave, Jethro Beekman,
Dr . Greg Wettstein
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Saturday, June 29, 2019 4:42 PM
>
> On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> >
> > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > >> From: Christopherson, Sean J
> > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > >>
> > >> Intended use of each permission:
> > >>
> > >> - SGX_EXECDIRTY: dynamically load code within the enclave itself
> > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > >> Graphene
> > >
> > > Why does it matter whether a code page is measured or not?
> >
> > It won't be incorporated into an attestation?
> >
>
> Also, if there is, in parallel, a policy that limits the set of enclave
> SIGSTRUCTs that are accepted, requiring all code be measured makes it
> harder to subvert by writing incompetent or maliciously incompetent
> enclaves.
As analyzed in my reply to one of Stephen's comments, no executable page shall be "enclave only" as enclaves have access to host's memory, so if an executable page in regular memory is considered posting a threat to the process, it should be considered posting the same threat inside an enclave as well.
That said, every executable enclave page should have an executable source page (doesn’t have to executable, as long as mprotect(X) would succeed on it, as shown in my patch), hence any exploits mountable on the enclave page shall also be mountable using the source page. Given only the weakest link matters in security, I argue that SGX_EXECUNMR is unnecessary from the process's perspective.
SGX_EXECUNMR does impact security from the enclave's perspective, thus it is reflected in enclave's measurement, which is part of SGX ISA. It's the enclave vendor's responsibility to ensure code pages are properly measured and that's largely automated by tools. It's highly unlikely an ISV would "forget" to measure a page so I don't think SGX_EXECUNMR has much value for ISVs.
So the only case left is the enclave author left a page unmeasured with a malicious intent. As that's part of the enclave measurement, it would get caught at EINIT because of an untrusted/blacklisted signing key, or it doesn't because of the lack of whitelisting/blacklisting mechanism. But in the latter case, the adversary could just measure the malicious page as the final measurement or signing key doesn't matter anyway. Sean's series doesn't have an enclave_init() hook so it will always be the latter case, where the final measurement doesn't matter. Therefore, SGX_EXECUNMR doesn't have any value as adversaries could always measure all code pages to satisfy the policy without worrying about final measurements.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-29 21:35 ` Casey Schaufler@ 2019-07-01 17:57 ` Xing, Cedric
2019-07-01 19:53 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 17:57 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>
> On 6/28/2019 6:37 PM, Stephen Smalley wrote:
> > On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-
> ca.com> wrote:
> >> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
> >>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> >>>> Sent: Thursday, June 27, 2019 4:37 PM
> >>>>>> This code should not be mixed in with the LSM infrastructure.
> >>>>>> It should all be contained in its own module, under
> security/enclave.
> >>>>> lsm_ema is *intended* to be part of the LSM infrastructure.
> >>>> That's not going to fly, not for a minute.
> >>> Why not, if there's a need for it?
> >>>
> >>> And what's the concern here if it becomes part of the LSM
> infrastructure.
> >> The LSM infrastructure provides a framework for hooks and allocation
> >> of blobs. That's it. It's a layer for connecting system features like
> >> VFS, IPC and the IP stack to the security modules. It does not
> >> implement any policy of it's own. We are not going to implement SGX
> >> or any other mechanism within the LSM infrastructure.
> > I don't think you understand the purpose of this code. It isn't
> > implementing SGX, nor is it needed by SGX.
> > It is providing shared infrastructure for security modules, similar to
> > lsm_audit.c, so that security modules can enforce W^X or similar
> > memory protection guarantees for SGX enclave memory, which has unique
> > properties that render the existing mmap and mprotect hooks
> > insufficient. They can certainly implement it only for SELinux, but
> > then any other security module that wants to provide such guarantees
> > will have to replicate that code.
>
> I am not objecting to the purpose of the code.
> I *am* objecting to calling it part of the LSM infrastructure.
> It needs to be it's own thing, off somewhere else.
> It must not use the lsm_ prefix. That's namespace pollution.
> The code must not be embedded in the LSM infrastructure code, that
> breaks with how everything else works.
If you understand the purpose, then why are you objecting the lsm_ prefix as they are APIs to be used by all LSM modules? Or what should be the prefix in your mind? Or what kind of APIs do you think can qualify the lsm_ prefix?
And I'd like to clarify that it doesn't break anything, but is just a bit different, in that security_enclave_load() and security_file_free() call into those APIs. But there's a need for them because otherwise code/data would have to be duplicated among LSMs and the logic would be harder to comprehend. So that's a trade-off. Then what's the practical drawback of doing that? If no, why would we want to pay for the cost for not doing that?
>
> ... and the notion that you allocate data for one blob that gets freed
> relative to another breaks the data management model.
What do you mean here?
EMA blobs are allocated/freed *not* relative to any other blobs.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 17:11 ` Xing, Cedric@ 2019-07-01 17:58 ` Andy Lutomirski
2019-07-01 18:31 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Andy Lutomirski @ 2019-07-01 17:58 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, linux-sgx, LSM List, selinux, Schaufler, Casey,
James Morris, Jethro Beekman, Dr. Greg Wettstein,
Stephen Smalley, Jarkko Sakkinen, Christopherson, Sean J
On Mon, Jul 1, 2019 at 10:11 AM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Saturday, June 29, 2019 4:47 PM
> >
> > Just on a very cursory review, this seems like it's creating a bunch of
> > complexity (a whole new library and data structure), and I'm not
> > convinced the result is any better than Sean's version.
>
> The new EMA data structure is to track enclave pages by range. Yes, Sean avoided that by storing similar information in the existing encl_page structure inside SGX subsystem. But as I pointed out, his code has to iterate through *every* page in range so mprotect() will be very slow if the range is large. So he would end up introducing something similar to achieve the same performance.
It seems odd to stick it in security/ if it only has one user, though.
Also, if it wasn't in security/, then the security folks would stop
complaining :)
>
> And that's not the most important point. The major problem in his patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or he'd have to amend his code accordingly, which will add complexity and tip your scale).
>
Why can't it be?
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-06-29 1:22 ` Stephen Smalley@ 2019-07-01 18:02 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 18:02 UTC (permalink / raw)
To: Stephen Smalley
Cc: Stephen Smalley, linux-sgx, linux-security-module, selinux,
Schaufler, Casey, jmorris, luto, jethro, greg, jarkko.sakkinen,
Christopherson, Sean J
> From: Stephen Smalley [mailto:stephen.smalley@gmail.com]
> Sent: Friday, June 28, 2019 6:22 PM
>
> On Fri, Jun 28, 2019 at 5:54 PM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > > Sent: Friday, June 28, 2019 9:37 AM
> > >
> > > > lsm.ema.cache_decisions is on by default and could be turned off
> > > > by appending “lsm.ema.cache_decisions=0” or
> > > > “lsm.ema.cache_decisions=off” to the kernel command line.
> > >
> > > This seems problematic on a few fronts:
> > >
> > > - Specifying it as a boot parameter requires teaching admins /
> > > policy developers to do something in addition to what they are
> > > already doing when they want to create policy,
> > >
> > > - Limiting it to a boot parameter requires a reboot to change the
> > > mode of operation, whereas SELinux offers runtime toggling of
> > > permissive mode and even per-process (domain) permissive mode (and
> > > so does AppArmor),
> >
> > How about making a variable tunable via sysctl?
>
> Better than a boot parameter but still not amenable to per-domain
> permissive and still requires admins to remember and perform an extra
> step before collecting denials.
>
> >
> > >
> > > - In the cache_decisions=1 case, do we get any auditing at all? If
> > > not, that's a problem. We want auditing not only when we are
> > > generating/learning policy but also in operation.
> >
> > Currently it doesn't generate audit log, but I could add it, except it
> couldn't point to the exact file. But I can use the sigstruct file
> instead so administrators can at least tell which enclave violates the
> policy. Do you think it acceptable?
>
> Seems prone to user confusion and lacks precision in why the denial
> occurred.
>
> >
> > >
> > > - There is the potential for inconsistencies to arise between the
> > > enforcement applied with different cache_decisions values.
> >
> > The enforcement will be consistent. The difference only lies in the
> logs.
> >
> > >
> > > I would suggest that we just never cache the decision and accept the
> > > cost if we are going to take this approach.
> >
> > This will also be a viable option. I don't think any enclaves would be
> comprised of a large number of files anyway. When SGX2 comes up, I think
> most enclaves will be instantiated from only one file and defer loading
> libraries at runtime. So in practice we are looking to keeping only one
> file open per enclave, which seems totally acceptable.
> >
> > Stephen (and everyone having an opinion on this), which way do you
> prefer? sysctl variable? Or never cache decisions?
>
> I'd favor never caching decisions.
Alright, I'll remove the boot parameter and never cache decisions.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-06-29 1:15 ` Stephen Smalley@ 2019-07-01 18:14 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 18:14 UTC (permalink / raw)
To: Stephen Smalley
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein
> From: Stephen Smalley [mailto:stephen.smalley@gmail.com]
> Sent: Friday, June 28, 2019 6:16 PM
>
> On Fri, Jun 28, 2019 at 5:20 PM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> > > owner@vger.kernel.org] On Behalf Of Stephen Smalley
> > > Sent: Friday, June 28, 2019 9:17 AM
> > >
> > > FWIW, adding new permissions only requires updating policy
> > > configuration, not userspace code/tools. But in any event, we can
> > > reuse the execute- related permissions if it makes sense but still
> > > consider introducing additional, new permissions, possibly in a
> > > separate "enclave" security class, if we want explicit control over
> enclave loading, e.g.
> > > ENCLAVE__LOAD, ENCLAVE__INIT, etc.
> >
> > I'm not so familiar with SELinux tools so my apology in advance if I
> end up mixing up things.
> >
> > I'm not only talking about the new permissions, but also how to apply
> them to enclave files. Intel SGX SDK packages enclaves as .so files, and
> I guess that's the most straight forward way that most others would do.
> So if different permissions are defined, then user mode tools would have
> to distinguish enclaves from regular .so files in order to grant them
> different permissions. Would that be something extra to existing tools?
>
> It doesn't require any userspace code changes. It is just a matter of
> defining some configuration data in the policy for the new permissions,
> one or more security labels (tags) for the SGX .so files, and rules
> allowing access where desired, and then setting those security labels on
> the SGX .so files (via the security.selinux extended attribute on the
> files). Even the last part is generally handled by updating a
> configuration specifying how files should be labeled and then rpm
> automatically labels the files when created, or you can manually
> restorecon them. If the new permissions are defined in their own
> security class rather than reusing existing ones, then they can even be
> defined entirely via a local or third party policy module separate from
> the distro policy if desired/needed.
I'm not objecting to what you proposed but just trying to understand more.
SGX enclaves don't look any different than regular shared objects except the meta data section, which is implementation dependent (all enclaves built by Intel's SDK have .note.sgxmeta sections but others could do something completely different and may not even use ELF sections). Then how does rpm tell whether a .so file is a regular shared object or an SGX enclave? My understanding is, rpm has to be able to distinguish those two in order to label them correctly (differently). Am I correct?
>
> >
> > >
> > > One residual concern I have with the reuse of FILE__EXECUTE is using
> > > it for the sigstruct file as the fallback case. If the sigstruct is
> > > always part of the same file as the code, then it probably doesn't
> > > matter. But otherwise, it is somewhat odd to have to allow the host
> > > process to execute from the sigstruct file if it is only data (the
> signature).
> >
> > I agree with you. But do you think it a practical problem today? As
> far as I know, no one is deploying sigstructs in dedicated files. I'm
> just trying to touch as few things as possible until there's definitely
> a need to do so.
>
> I don't know, and it wasn't clear to me from the earlier discussions.
> If not and if it is acceptable to require them to be in files in the
> first place, then perhaps it isn't necessary.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 17:58 ` Andy Lutomirski@ 2019-07-01 18:31 ` Xing, Cedric
2019-07-01 19:36 ` Andy Lutomirski0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 18:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx, LSM List, selinux, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 10:58 AM
>
> On Mon, Jul 1, 2019 at 10:11 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > Hi Andy,
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Saturday, June 29, 2019 4:47 PM
> > >
> > > Just on a very cursory review, this seems like it's creating a bunch
> > > of complexity (a whole new library and data structure), and I'm not
> > > convinced the result is any better than Sean's version.
> >
> > The new EMA data structure is to track enclave pages by range. Yes,
> Sean avoided that by storing similar information in the existing
> encl_page structure inside SGX subsystem. But as I pointed out, his code
> has to iterate through *every* page in range so mprotect() will be very
> slow if the range is large. So he would end up introducing something
> similar to achieve the same performance.
>
> It seems odd to stick it in security/ if it only has one user, though.
> Also, if it wasn't in security/, then the security folks would stop
> complaining :)
That's where I started. EMA (though named differently in my v1) was buried inside and used only by SELinux. But Stephen thought it useful for other LSMs as well, as it could be expected that other LSMs would also need to track enclave pages and end up duplicating what's done inside SELinux.
I'm ok either way, though I do agree with Stephen's assessment.
>
>
> >
> > And that's not the most important point. The major problem in his
> patch lies in SGX2 support, as #PF driven EAUG cannot be supported (or
> he'd have to amend his code accordingly, which will add complexity and
> tip your scale).
> >
>
> Why can't it be?
Let me take it back. It's important as it is where LSM folks are divided.
I intended to say the major reason I objected Sean's approach was its inability to support SGX2 smoothly - as #PF driven EAUG requires non-existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be dispatched so EAUG couldn't be issued in response to #PF.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-07-01 17:53 ` Andy Lutomirski@ 2019-07-01 18:54 ` Xing, Cedric
2019-07-01 19:03 ` Xing, Cedric
2019-07-01 19:32 ` Andy Lutomirski0 siblings, 2 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 18:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Jethro Beekman,
Dr . Greg Wettstein
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 10:54 AM
>
> On Mon, Jul 1, 2019 at 10:46 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Saturday, June 29, 2019 4:42 PM
> > >
> > > On Tue, Jun 25, 2019 at 2:09 PM Stephen Smalley <sds@tycho.nsa.gov>
> > > wrote:
> > > >
> > > > On 6/21/19 5:22 PM, Xing, Cedric wrote:
> > > > >> From: Christopherson, Sean J
> > > > >> Sent: Wednesday, June 19, 2019 3:24 PM
> > > > >>
> > > > >> Intended use of each permission:
> > > > >>
> > > > >> - SGX_EXECDIRTY: dynamically load code within the enclave
> itself
> > > > >> - SGX_EXECUNMR: load unmeasured code into the enclave, e.g.
> > > > >> Graphene
> > > > >
> > > > > Why does it matter whether a code page is measured or not?
> > > >
> > > > It won't be incorporated into an attestation?
> > > >
> > >
> > > Also, if there is, in parallel, a policy that limits the set of
> > > enclave SIGSTRUCTs that are accepted, requiring all code be measured
> > > makes it harder to subvert by writing incompetent or maliciously
> > > incompetent enclaves.
> >
> > As analyzed in my reply to one of Stephen's comments, no executable
> page shall be "enclave only" as enclaves have access to host's memory,
> so if an executable page in regular memory is considered posting a
> threat to the process, it should be considered posting the same threat
> inside an enclave as well.
What I was trying to say was, an executable page, if considered a threat to the enclosing process, should always be considered a threat no matter it is in that process's memory or inside an enclave enclosed in that same process's address space.
Therefore, for a page in regular memory, if it is denied executable, it is because it is considered a potential security threat to the enclosing process, so it shall not be used as the source for an executable enclave page, as the same threat exists regardless it is in regular memory or EPC. Does that make more sense?
>
> Huh? The SDM (37.3 in whateve version I'm reading) says "Code fetches
> from inside an enclave to a linear address outside that enclave result
> in a #GP(0) exception." Enclaves execute enclave code only.
>
> In any event, I believe we're discussing taking readable memory from
> outside the enclave and copying it to an executable code inside the
> enclave.
You are correct. SGX ISA doesn't care the source page as it only takes care of the security the enclave itself.
But LSM on the other hand also takes care of the enclosing process. That said, a page, if denied executable because it is considered a potential threat to the process by LSM, should also be denied (by LSM) as the source for an executable enclave page because the same threat would exist even if it resides inside an enclave, for enclaves have access to all of the enclosing process's memory.
>
> >
> > That said, every executable enclave page should have an executable
> > source page (doesn’t have to executable, as long as mprotect(X) would
> > succeed on it, as shown in my patch)
>
> Does Sean's series require this? I think that, if we can get away with
> it, it's a lot nicer to *not* require user code to map the source pages
> PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some
> other attribute of the VMA, but actually requiring PROT_EXEC seems like
> we're weakening existing hardening measures to enforce a policy, which
> is a mistake.
My patch doesn't require X on source pages either. I said "would", meaning X *would* be granted but doesn't have to be granted. You can see this in selinux_enclave_load() calling selinux_file_mprotect() in my code. The purpose is to determine if X *would* be granted to the source pages without actually granting X.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v4 10/12] security/selinux: Add enclave_load() implementation
2019-07-01 18:54 ` Xing, Cedric@ 2019-07-01 19:03 ` Xing, Cedric
2019-07-01 19:32 ` Andy Lutomirski1 sibling, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 19:03 UTC (permalink / raw)
To: Xing, Cedric, Andy Lutomirski
Cc: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen,
linux-sgx, linux-security-module, selinux, Roberts, William C,
Schaufler, Casey, James Morris, Hansen, Dave, Jethro Beekman,
Dr . Greg Wettstein
Hi Andy,
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Xing, Cedric
> Sent: Monday, July 01, 2019 11:54 AM
> > >
> > > That said, every executable enclave page should have an executable
> > > source page (doesn’t have to executable, as long as mprotect(X)
> would
> > > succeed on it, as shown in my patch)
> >
> > Does Sean's series require this? I think that, if we can get away
> with
> > it, it's a lot nicer to *not* require user code to map the source
> pages
> > PROT_EXEC. Some policy may check that it's VM_MAYEXEC or check some
> > other attribute of the VMA, but actually requiring PROT_EXEC seems
> like
> > we're weakening existing hardening measures to enforce a policy, which
> > is a mistake.
>
> My patch doesn't require X on source pages either. I said "would",
> meaning X *would* be granted but doesn't have to be granted. You can see
> this in selinux_enclave_load() calling selinux_file_mprotect() in my
> code. The purpose is to determine if X *would* be granted to the source
> pages without actually granting X.
Forgot to conclude that we are on the same page for the requirement on the source pages.
And given that requirement (enclave page cannot be X unless source would also be allowed X), measuring enclave code pages or not doesn't make any difference from the enclosing process's perspective in terms of security. So it only makes a difference for the enclave, which however has been covered cryptographically by its measurement already. So SGX_EXECUNMR doesn't have any practical use, thus I don't think it should be added as a new permission.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 17:57 ` Xing, Cedric@ 2019-07-01 19:53 ` Casey Schaufler
2019-07-01 21:45 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-01 19:53 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
On 7/1/2019 10:57 AM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
>> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>>
>> On 6/28/2019 6:37 PM, Stephen Smalley wrote:
>>> On Fri, Jun 28, 2019 at 1:22 PM Casey Schaufler <casey@schaufler-
>> ca.com> wrote:
>>>> On 6/27/2019 5:47 PM, Xing, Cedric wrote:
>>>>>> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
>>>>>> Sent: Thursday, June 27, 2019 4:37 PM
>>>>>>>> This code should not be mixed in with the LSM infrastructure.
>>>>>>>> It should all be contained in its own module, under
>> security/enclave.
>>>>>>> lsm_ema is *intended* to be part of the LSM infrastructure.
>>>>>> That's not going to fly, not for a minute.
>>>>> Why not, if there's a need for it?
>>>>>
>>>>> And what's the concern here if it becomes part of the LSM
>> infrastructure.
>>>> The LSM infrastructure provides a framework for hooks and allocation
>>>> of blobs. That's it. It's a layer for connecting system features like
>>>> VFS, IPC and the IP stack to the security modules. It does not
>>>> implement any policy of it's own. We are not going to implement SGX
>>>> or any other mechanism within the LSM infrastructure.
>>> I don't think you understand the purpose of this code. It isn't
>>> implementing SGX, nor is it needed by SGX.
>>> It is providing shared infrastructure for security modules, similar to
>>> lsm_audit.c, so that security modules can enforce W^X or similar
>>> memory protection guarantees for SGX enclave memory, which has unique
>>> properties that render the existing mmap and mprotect hooks
>>> insufficient. They can certainly implement it only for SELinux, but
>>> then any other security module that wants to provide such guarantees
>>> will have to replicate that code.
>> I am not objecting to the purpose of the code.
>> I *am* objecting to calling it part of the LSM infrastructure.
>> It needs to be it's own thing, off somewhere else.
>> It must not use the lsm_ prefix. That's namespace pollution.
>> The code must not be embedded in the LSM infrastructure code, that
>> breaks with how everything else works.
> If you understand the purpose,
The purpose is to support the SGX hardware, is it not?
If you don't have SGX hardware (e.g. MIPS, ARM, s390) you
don't need this code.
> then why are you objecting the lsm_ prefix as they are APIs to be used by all LSM modules?
We name interfaces based on what they provide, not who consumes them.
As your code provides enclave services, that is how they should be named.
> Or what should be the prefix in your mind?
I'm pretty sure that I've consistently suggested "enclave".
> Or what kind of APIs do you think can qualify the lsm_ prefix?
Code that implements the LSM infrastructure. There is one LSM
blob allocation interface, lsm_inode_alloc(), that is used in
early set-up that is exported. As I've mentioned more than once,
enclave/page management is not an LSM infrastructure function,
it's a memory management function.
> And I'd like to clarify that it doesn't break anything, but is just a bit different, in that security_enclave_load() and security_file_free() call into those APIs.
There should be nothing in security_enclave_load() except calls to the enclave_load()
hooks (e.g. selinux_enclave_load()). There should be nothing in security_file_free()
except file blob management calls to the file_free() hooks (e.g. apparmor_file_free()).
> But there's a need for them because otherwise code/data would have to be duplicated among LSMs
There's plenty of code duplication among the LSMs, because a lot
of what they do is the same thing. Someday there may be an effort
to address some of that, but I don't think it's on anybody's radar.
As for data duplication, there's a reason we use lots of pointers.
> and the logic would be harder to comprehend.
Keeping the layering clean is critical to comprehension.
There's a lot of VFS code that could have been implemented
within the LSM infrastructure, but I don't think that anyone
would argue that it should have been.
> So that's a trade-off.
I remain completely unconvinced that your proposal
represents a good way to implement you scheme.
> Then what's the practical drawback of doing that?
Name space pollution.
Layering violation.
Architecture specific implementation detail in a
general infrastructure.
> If no, why would we want to pay for the cost for not doing that?
Modularity and maintainability come directly to mind.
>> ... and the notion that you allocate data for one blob that gets freed
>> relative to another breaks the data management model.
> What do you mean here?
You're freeing the EMA data from security_file_free().
If selinux wants to free EMA data it has allocated in
selinux_enclave_load() in selinux_file_free() that's fine,
but the LSM infrastructure has no need to know about it.
EMA needs to manage its own data, just like VFS does.
The LSM infrastructure provides blob management so that
the security modules can extend data if they want to.
> EMA blobs are allocated/freed *not* relative to any other blobs.
In the code you proposed they are freed in security_file_free().
That is for file blob management.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 19:36 ` Andy Lutomirski@ 2019-07-01 19:56 ` Xing, Cedric
2019-07-02 2:29 ` Andy Lutomirski0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 19:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx, LSM List, selinux, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 01, 2019 12:36 PM
>
> On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> > I intended to say the major reason I objected Sean's approach was its
> inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> dispatched so EAUG couldn't be issued in response to #PF.
>
> I still think that, if the kernel wants to support #PF-driven EAUG, it
> should be an opt-in thing. It would be something like
> SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> the driver needs to learn how to track ranges of pages efficiently,
> which is another reason to consider leaving all the fancy page / page
> range tracking in the driver.
>
> I don't think it's a good idea for a page fault on any non-EADDed page
> in ELRANGE to automatically populate the page.
I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF. What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages can be mapped before coming into existence.
For "page tracking", what information to track is LSM dependent, so it may run into problems if different LSMs want to track different things. And that's the major reason I think it should be done inside LSM.
Besides, the current page tracking structure in the driver is page oriented and its sole purpose is to serve #PFs. Page protection is better tracked using range oriented structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel by moving it into SGX driver.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 19:53 ` Casey Schaufler@ 2019-07-01 21:45 ` Xing, Cedric
2019-07-01 23:11 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-01 21:45 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Casey Schaufler
> Sent: Monday, July 01, 2019 12:54 PM
> > If you understand the purpose,
>
> The purpose is to support the SGX hardware, is it not?
> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
> this code.
No, it is NOT to support SGX - i.e. SGX doesn't require this piece of code to work.
And as Dr. Greg pointed out, it can be used for other TEEs than SGX. It doesn't contain SGX h/w specifics. It is compiled out because there's no module calling it on other architectures today. But it doesn't conflict with any h/w and may be useful (for other TEEs) on other architectures in future.
>
> > then why are you objecting the lsm_ prefix as they are APIs to be used
> by all LSM modules?
>
> We name interfaces based on what they provide, not who consumes them.
> As your code provides enclave services, that is how they should be named.
>
> > Or what should be the prefix in your mind?
>
> I'm pretty sure that I've consistently suggested "enclave".
>
> > Or what kind of APIs do you think can qualify the lsm_ prefix?
>
> Code that implements the LSM infrastructure. There is one LSM blob
> allocation interface, lsm_inode_alloc(), that is used in early set-up
> that is exported. As I've mentioned more than once, enclave/page
> management is not an LSM infrastructure function, it's a memory
> management function.
It doesn't manage anything. The reason it appears in the infrastructure is because the decision of inserting an EMA depends on the decisions from *all* active LSMs. That is NOT new either, as you can see it in security_file_permission() and security_vm_enough_memory_mm(), both do something after all LSM modules make their decisions.
Would you please explain why you don't see those as problems but calling EMA functions in security_enclave_load() is a problem?
>
> > And I'd like to clarify that it doesn't break anything, but is just a
> bit different, in that security_enclave_load() and security_file_free()
> call into those APIs.
>
> There should be nothing in security_enclave_load() except calls to the
> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
> nothing in security_file_free() except file blob management calls to the
> file_free() hooks (e.g. apparmor_file_free()).
As above, there are examples in security/security.c where the hook does more than just calling registered hooks from LSMs.
>
> > But there's a need for them because otherwise code/data would have to
> > be duplicated among LSMs
>
> There's plenty of code duplication among the LSMs, because a lot of what
> they do is the same thing. Someday there may be an effort to address
> some of that, but I don't think it's on anybody's radar.
> As for data duplication, there's a reason we use lots of pointers.
As stated above, security_enclave_load() needs to do something extra after all LSMs make their decisions. How can pointers help here?
>
> > and the logic would be harder to comprehend.
>
> Keeping the layering clean is critical to comprehension.
> There's a lot of VFS code that could have been implemented within the
> LSM infrastructure, but I don't think that anyone would argue that it
> should have been.
>
> > So that's a trade-off.
>
> I remain completely unconvinced that your proposal represents a good way
> to implement you scheme.
>
> > Then what's the practical drawback of doing that?
>
> Name space pollution.
Alright, I can fix the names.
> Layering violation.
Not sure what you are referring to.
If you are referring to buffers allocated in one layer and freed in elsewhere, you have got the code wrong. Buffers allocated in security_enclave_load() is freed in security_file_free(). Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-enclave files, otherwise it could be done in file_alloc() to be more "paired" with file_free(). But I don't see it necessary.
> Architecture specific implementation detail in a general infrastructure.
Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction. It could be left on all the time or controlled by a different config macro. It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only so far) TEE that needs attention from LSM, but there could be more in future.
>
> > If no, why would we want to pay for the cost for not doing that?
>
> Modularity and maintainability come directly to mind.
Putting it elsewhere will incur more maintenance cost.
>
> >> ... and the notion that you allocate data for one blob that gets
> >> freed relative to another breaks the data management model.
> > What do you mean here?
>
> You're freeing the EMA data from security_file_free().
> If selinux wants to free EMA data it has allocated in
> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
> infrastructure has no need to know about it.
> EMA needs to manage its own data, just like VFS does.
> The LSM infrastructure provides blob management so that the security
> modules can extend data if they want to.
You've got the code wrong. selinux_enclave_load() doesn't allocate any memory. selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
The LSM infrastructure doesn't know anything about what LSM modules do, nor does it manage any buffers allocated by any LSM modules.
EMA is currently managing its own data. What's needed is the trigger - to let EMA know when to update its states. The trigger could be placed in LSM infrastructure or inside individual LSMs. The reason to put it in the infrastructure, is that it depends on the decision of *all* LSMs whether to insert a new EMA. That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made by the infrastructure but not individual LSMs.
>
> > EMA blobs are allocated/freed *not* relative to any other blobs.
>
> In the code you proposed they are freed in security_file_free().
> That is for file blob management.
Yes. EMA contributes to the file blob. But it only frees memory allocated by the infrastructure itself, not anything from any LSM modules.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 21:45 ` Xing, Cedric@ 2019-07-01 23:11 ` Casey Schaufler
2019-07-02 7:42 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-01 23:11 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
On 7/1/2019 2:45 PM, Xing, Cedric wrote:
>> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
>> owner@vger.kernel.org] On Behalf Of Casey Schaufler
>> Sent: Monday, July 01, 2019 12:54 PM
>>> If you understand the purpose,
>> The purpose is to support the SGX hardware, is it not?
>> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
>> this code.
> No, it is NOT to support SGX
Then what *is* it for?
> - i.e. SGX doesn't require this piece of code to work.
>
> And as Dr. Greg pointed out, it can be used for other TEEs than SGX.
That sure makes it sound like it's for SGX to me.
> It doesn't contain SGX h/w specifics.
I never said it did. But no one ever suggested doing anything
here before SGX, and your subject line:
"x86/sgx: Add SGX specific LSM hooks"
says it does.
> It is compiled out because there's no module calling it on other architectures today. But it doesn't conflict with any h/w and may be useful (for other TEEs) on other architectures in future.
>
>>> then why are you objecting the lsm_ prefix as they are APIs to be used
>> by all LSM modules?
>>
>> We name interfaces based on what they provide, not who consumes them.
>> As your code provides enclave services, that is how they should be named.
>>
>>> Or what should be the prefix in your mind?
>> I'm pretty sure that I've consistently suggested "enclave".
>>
>>> Or what kind of APIs do you think can qualify the lsm_ prefix?
>> Code that implements the LSM infrastructure. There is one LSM blob
>> allocation interface, lsm_inode_alloc(), that is used in early set-up
>> that is exported. As I've mentioned more than once, enclave/page
>> management is not an LSM infrastructure function, it's a memory
>> management function.
> It doesn't manage anything.
Sorry, "memory management" as in all that stuff around pages and
TLBs and who gets what pages, as opposed to keeping track of anything
on its own.
> The reason it appears in the infrastructure is because the decision of inserting an EMA depends on the decisions from *all* active LSMs.
You have not been listening. Most LSMs use VFS. We haven't rolled VFS
functions into the LSM infrastructure.
> That is NOT new either, as you can see it in security_file_permission() and security_vm_enough_memory_mm(), both do something after all LSM modules make their decisions.
Did you look to see what it is they're doing? If you had,
you would see that is nothing like what you're proposing.
> Would you please explain why you don't see those as problems but calling EMA functions in security_enclave_load() is a problem?
The enclave code should be calling security_enclave_load(),
not the other way around. That assumes you're using the naming
convention correctly.
security_vm_enough_memory_mm() was discussed at length and there
wasn't a clean way to get the logic right without putting the code
here. security_file_permission() has the fs_notify_perm call for
similar reasons. Neither is anything like what you're suggesting.
>>> And I'd like to clarify that it doesn't break anything, but is just a
>> bit different, in that security_enclave_load() and security_file_free()
>> call into those APIs.
>>
>> There should be nothing in security_enclave_load() except calls to the
>> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
>> nothing in security_file_free() except file blob management calls to the
>> file_free() hooks (e.g. apparmor_file_free()).
> As above, there are examples in security/security.c where the hook does more than just calling registered hooks from LSMs.
And as I've said, that doesn't matter. You're still going about
using the LSM infrastructure backwards.
>>> But there's a need for them because otherwise code/data would have to
>>> be duplicated among LSMs
>> There's plenty of code duplication among the LSMs, because a lot of what
>> they do is the same thing. Someday there may be an effort to address
>> some of that, but I don't think it's on anybody's radar.
>> As for data duplication, there's a reason we use lots of pointers.
> As stated above, security_enclave_load() needs to do something extra after all LSMs make their decisions. How can pointers help here?
I can explain it, but you clearly have no interest in doing
anything to make your code fit into the system. I have a lot
of other things to be doing.
>>> and the logic would be harder to comprehend.
>> Keeping the layering clean is critical to comprehension.
>> There's a lot of VFS code that could have been implemented within the
>> LSM infrastructure, but I don't think that anyone would argue that it
>> should have been.
>>
>>> So that's a trade-off.
>> I remain completely unconvinced that your proposal represents a good way
>> to implement you scheme.
>>
>>> Then what's the practical drawback of doing that?
>> Name space pollution.
> Alright, I can fix the names.
Good!
>> Layering violation.
> Not sure what you are referring to.
The only places where the blob freed by security_file_free()
may be allocated is security_file_alloc(). The security modules
are welcome to do anything they like in addition, provided
they clean up after themselves in their file_free() hooks.
If SELinux wants to support controls on enclave information,
and that requires additional data, SELinux should include
space in its file blob for that information, or a pointer to
the place where the enclave code is maintaining it.
That's the way audit works.
> If you are referring to buffers allocated in one layer and freed in elsewhere, you have got the code wrong. Buffers allocated in security_enclave_load() is freed in security_file_free().
It's up to the security module's file_free() to clean up anything that
wasn't allocated in security_file_free(). Interested security modules
should call enclave_load(), and put the information into their portion
of the security blob. The module specific code can call enclave_file_free(),
or whatever interface you want to provide, to clean up. That might take
place in file_free(), but it also might be elsewhere.
> Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-enclave files, otherwise it could be done in file_alloc() to be more "paired" with file_free(). But I don't see it necessary.
Try looking at maintaining what you've put into the LSM code as
a separate entity. It makes it simpler. Really.
>> Architecture specific implementation detail in a general infrastructure.
> Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction.
Then put it in the TEE system.
> It could be left on all the time or controlled by a different config macro.
True in any case.
> It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only so far) TEE that needs attention from LSM, but there could be more in future.
All the more reason to keep it separate. These things never get simpler
when they get more generalized.
>>> If no, why would we want to pay for the cost for not doing that?
>> Modularity and maintainability come directly to mind.
> Putting it elsewhere will incur more maintenance cost.
I don't believe that for a second. 40 years of C programming
have taught me that trying to do multiple things in one place
is always a bad idea.
>>>> ... and the notion that you allocate data for one blob that gets
>>>> freed relative to another breaks the data management model.
>>> What do you mean here?
>> You're freeing the EMA data from security_file_free().
>> If selinux wants to free EMA data it has allocated in
>> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
>> infrastructure has no need to know about it.
>> EMA needs to manage its own data, just like VFS does.
>> The LSM infrastructure provides blob management so that the security
>> modules can extend data if they want to.
> You've got the code wrong. selinux_enclave_load() doesn't allocate any memory. selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
... and the LSM infrastructure doesn't care and
must not be made to care. It's all up to SELinux.
> The LSM infrastructure doesn't know anything about what LSM modules do, nor does it manage any buffers allocated by any LSM modules.
Right, which is why putting your lsm_ema_blob is wrong, and why
forcing into the file blob is wrong.
> EMA is currently managing its own data. What's needed is the trigger - to let EMA know when to update its states. The trigger could be placed in LSM infrastructure or inside individual LSMs.
Yes. The latter.
> The reason to put it in the infrastructure, is that it depends on the decision of *all* LSMs whether to insert a new EMA.
That's basic stacking behavior. "Bail on fail", which says that once
denial is detected, you're done.
> That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made by the infrastructure but not individual LSMs.
Do you really understand the painful reasons that case is required?
And if so, why you aren't taking steps to avoid them?
>>> EMA blobs are allocated/freed *not* relative to any other blobs.
>> In the code you proposed they are freed in security_file_free().
>> That is for file blob management.
> Yes. EMA contributes to the file blob. But it only frees memory allocated by the infrastructure itself, not anything from any LSM modules.
That's not the way it's supposed to be done. The module tells
the infrastructure what it needs, which may include space for
EMA data. The module asks EMA for the data it needs and stuffs
it somewhere, and the file blob is a fine choice. The module
cleans up in file_free, or at any time before that. If no module
uses EMA, nothing goes in the blob. If two modules use EMA each
is responsible for the data it uses, which may be the same or
may be different.
I've looked at your code. Making it work the way it should would
not be difficult and would likely simplify a bunch of it.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 19:56 ` Xing, Cedric@ 2019-07-02 2:29 ` Andy Lutomirski
2019-07-02 6:35 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Andy Lutomirski @ 2019-07-02 2:29 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, linux-sgx, LSM List, selinux, Schaufler, Casey,
James Morris, Jethro Beekman, Dr. Greg Wettstein,
Stephen Smalley, Jarkko Sakkinen, Christopherson, Sean J
On Mon, Jul 1, 2019 at 12:56 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> > Sent: Monday, July 01, 2019 12:36 PM
> >
> > On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> > wrote:
> > > I intended to say the major reason I objected Sean's approach was its
> > inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> > existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> > dispatched so EAUG couldn't be issued in response to #PF.
> >
> > I still think that, if the kernel wants to support #PF-driven EAUG, it
> > should be an opt-in thing. It would be something like
> > SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> > the driver needs to learn how to track ranges of pages efficiently,
> > which is another reason to consider leaving all the fancy page / page
> > range tracking in the driver.
> >
> > I don't think it's a good idea for a page fault on any non-EADDed page
> > in ELRANGE to automatically populate the page.
>
> I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF. What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages can be mapped before coming into existence.
>
> For "page tracking", what information to track is LSM dependent, so it may run into problems if different LSMs want to track different things. And that's the major reason I think it should be done inside LSM.
>
> Besides, the current page tracking structure in the driver is page oriented and its sole purpose is to serve #PFs. Page protection is better tracked using range oriented structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel by moving it into SGX driver.
It seems to me that the driver is going to need to improve its data
structures to track ranges of pages regardless of any LSM issues. If
we're going to have an enclave with a huge ELRANGE and we're going to
mark some large subset of the full ELRANGE as allocate-on-demand, then
we are going to want to track that range in some efficient way. It
could be a single extent or a set of power-of-two-sized extents (i.e.
radix tree entries), or something else, but a list of pages, of which
some are marked not-yet-allocated, isn't going to cut it.
Once that happens, it seems natural to put whatever permission
tracking we need into the same data structure. That's why my proposal
had the driver getting coarse-grained info from LSM ("may execute
dirty page", for example) rather than asking the LSM to track the
whole state machine.
Does that make sense?
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-02 2:29 ` Andy Lutomirski@ 2019-07-02 6:35 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-02 6:35 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-sgx, LSM List, selinux, Schaufler, Casey, James Morris,
Jethro Beekman, Dr. Greg Wettstein, Stephen Smalley,
Jarkko Sakkinen, Christopherson, Sean J
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Monday, July 1, 2019 7:29 PM
>
> On Mon, Jul 1, 2019 at 12:56 PM Xing, Cedric <cedric.xing@intel.com> wrote:
> >
> > > From: Andy Lutomirski [mailto:luto@kernel.org]
> > > Sent: Monday, July 01, 2019 12:36 PM
> > >
> > > On Mon, Jul 1, 2019 at 11:31 AM Xing, Cedric <cedric.xing@intel.com>
> > > wrote:
> > > > I intended to say the major reason I objected Sean's approach was its
> > > inability to support SGX2 smoothly - as #PF driven EAUG requires non-
> > > existent pages to be mmap()'ed, otherwise vm_ops->fault wouldn't be
> > > dispatched so EAUG couldn't be issued in response to #PF.
> > >
> > > I still think that, if the kernel wants to support #PF-driven EAUG, it
> > > should be an opt-in thing. It would be something like
> > > SGX_IOC_ADD_LAZY_EAUG_PAGES or similar. If it's done that way, then
> > > the driver needs to learn how to track ranges of pages efficiently,
> > > which is another reason to consider leaving all the fancy page / page
> > > range tracking in the driver.
> > >
> > > I don't think it's a good idea for a page fault on any non-EADDed page
> > > in ELRANGE to automatically populate the page.
> >
> > I'm with you. The user code shall be explicit on which range to EAUG pages upon #PF.
> What I'm saying is, a range has to be mapped before the driver could receive #PFs (in the
> form of vm_ops->fault callbacks). But Sean's series doesn’t support that because no pages
> can be mapped before coming into existence.
> >
> > For "page tracking", what information to track is LSM dependent, so it may run into
> problems if different LSMs want to track different things. And that's the major reason I
> think it should be done inside LSM.
> >
> > Besides, the current page tracking structure in the driver is page oriented and its sole
> purpose is to serve #PFs. Page protection is better tracked using range oriented
> structures. Those 2 are orthogonal. It wouldn't reduce the complexity of the whole kernel
> by moving it into SGX driver.
>
> It seems to me that the driver is going to need to improve its data
> structures to track ranges of pages regardless of any LSM issues. If
> we're going to have an enclave with a huge ELRANGE and we're going to
> mark some large subset of the full ELRANGE as allocate-on-demand, then
> we are going to want to track that range in some efficient way. It
> could be a single extent or a set of power-of-two-sized extents (i.e.
> radix tree entries), or something else, but a list of pages, of which
> some are marked not-yet-allocated, isn't going to cut it.
>
> Once that happens, it seems natural to put whatever permission
> tracking we need into the same data structure. That's why my proposal
> had the driver getting coarse-grained info from LSM ("may execute
> dirty page", for example) rather than asking the LSM to track the
> whole state machine.
>
> Does that make sense?
The driver will eventually need some range oriented structures for managing EAUGs. But it doesn't necessarily have to be in the same structure as other per-page information. After all, they are touched by different components of the driver and indeed pretty orthogonal. Evils are always in the details. It may be counter-intuitive but per our prototype years ago, it would be simpler to just keep them separate.
IIUC, your idea is in fact keeping a FSM inside SGX driver and using return values from security_enclave_load() to argument it. That means LSM has to work quite "closely" with SGX driver (i.e. LSM needs to understand the FSM in SGX driver), which is quite different than all other existing hooks, which basically make binary decisions only. And I'm not sure how to chain LSMs if there are multiple active at the same time. Auditing is also a problem, as you can't generate audit log at the time a real mprotect() request is approved/denied. UAPI is also unpleasant as the enclave loader has to "predict" the maximal protection, which is not always available to the loader at load time, or significant changes to enclave build tools would be necessary. I think the FSM is really part of the policy and internal to LSM (or more particularly, SELinux, as different LSM modules may have different FSMs), so it still makes more sense to me to keep "LSM internals" internal to LSM.
^permalinkrawreply [flat|nested] 156+ messages in thread

*RE: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-01 23:11 ` Casey Schaufler@ 2019-07-02 7:42 ` Xing, Cedric
2019-07-02 15:44 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-02 7:42 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Monday, July 1, 2019 4:12 PM
>
> On 7/1/2019 2:45 PM, Xing, Cedric wrote:
> >> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> >> owner@vger.kernel.org] On Behalf Of Casey Schaufler
> >> Sent: Monday, July 01, 2019 12:54 PM
> >>> If you understand the purpose,
> >> The purpose is to support the SGX hardware, is it not?
> >> If you don't have SGX hardware (e.g. MIPS, ARM, s390) you don't need
> >> this code.
> > No, it is NOT to support SGX
>
> Then what *is* it for?
>
> > - i.e. SGX doesn't require this piece of code to work.
> >
> > And as Dr. Greg pointed out, it can be used for other TEEs than SGX.
>
> That sure makes it sound like it's for SGX to me.
I meant it is generic and potentially useful to more TEEs but so far useful to SGX, which is the first technology that uses this infrastructure. I hope that makes more sense.
>
> > It doesn't contain SGX h/w specifics.
>
> I never said it did. But no one ever suggested doing anything
> here before SGX, and your subject line:
>
> "x86/sgx: Add SGX specific LSM hooks"
>
> says it does.
Yes. And in the commit message I also stated that the need for such tracking structure is due to the unique lifespan of enclave pages. Hence this infrastructure will also be useful for other TEEs whose pages share the same lifespan.
>
> > It is compiled out because there's no module calling it on other architectures today.
> But it doesn't conflict with any h/w and may be useful (for other TEEs) on other
> architectures in future.
> >
> >>> then why are you objecting the lsm_ prefix as they are APIs to be used
> >> by all LSM modules?
> >>
> >> We name interfaces based on what they provide, not who consumes them.
> >> As your code provides enclave services, that is how they should be named.
> >>
> >>> Or what should be the prefix in your mind?
> >> I'm pretty sure that I've consistently suggested "enclave".
> >>
> >>> Or what kind of APIs do you think can qualify the lsm_ prefix?
> >> Code that implements the LSM infrastructure. There is one LSM blob
> >> allocation interface, lsm_inode_alloc(), that is used in early set-up
> >> that is exported. As I've mentioned more than once, enclave/page
> >> management is not an LSM infrastructure function, it's a memory
> >> management function.
> > It doesn't manage anything.
>
> Sorry, "memory management" as in all that stuff around pages and
> TLBs and who gets what pages, as opposed to keeping track of anything
> on its own.
>
> > The reason it appears in the infrastructure is because the decision of inserting an EMA
> depends on the decisions from *all* active LSMs.
>
> You have not been listening. Most LSMs use VFS. We haven't rolled VFS
> functions into the LSM infrastructure.
>
> > That is NOT new either, as you can see it in security_file_permission() and
> security_vm_enough_memory_mm(), both do something after all LSM modules make their
> decisions.
>
> Did you look to see what it is they're doing? If you had,
> you would see that is nothing like what you're proposing.
I feel like we are talking about different things. I said those hooks did more than just calling registered hooks. And security_enclave_load() is similar to them, also for a similar reason - something needs to be done after *all* LSM modules make their decisions.
I'm not sure what you are talking about.
>
>
> > Would you please explain why you don't see those as problems but calling EMA functions
> in security_enclave_load() is a problem?
>
> The enclave code should be calling security_enclave_load(),
> not the other way around. That assumes you're using the naming
> convention correctly.
Yes. The enclave code (SGX driver) calls security_enclave_load/init. Never the other way around.
Again, EMA code is similar to auditing code. It is supposed to be called by LSM modules.
>
> security_vm_enough_memory_mm() was discussed at length and there
> wasn't a clean way to get the logic right without putting the code
> here. security_file_permission() has the fs_notify_perm call for
> similar reasons. Neither is anything like what you're suggesting.
Guess we are discussing "at length" right now on how to get the logic right. I'm not sure why "neither is anything like what I'm suggesting".
>
>
> >>> And I'd like to clarify that it doesn't break anything, but is just a
> >> bit different, in that security_enclave_load() and security_file_free()
> >> call into those APIs.
> >>
> >> There should be nothing in security_enclave_load() except calls to the
> >> enclave_load() hooks (e.g. selinux_enclave_load()). There should be
> >> nothing in security_file_free() except file blob management calls to the
> >> file_free() hooks (e.g. apparmor_file_free()).
> > As above, there are examples in security/security.c where the hook does more than just
> calling registered hooks from LSMs.
>
> And as I've said, that doesn't matter. You're still going about
> using the LSM infrastructure backwards.
>
> >>> But there's a need for them because otherwise code/data would have to
> >>> be duplicated among LSMs
> >> There's plenty of code duplication among the LSMs, because a lot of what
> >> they do is the same thing. Someday there may be an effort to address
> >> some of that, but I don't think it's on anybody's radar.
> >> As for data duplication, there's a reason we use lots of pointers.
> > As stated above, security_enclave_load() needs to do something extra after all LSMs make
> their decisions. How can pointers help here?
>
> I can explain it, but you clearly have no interest in doing
> anything to make your code fit into the system. I have a lot
> of other things to be doing.
I'm so interested in getting things fit. Just that what I see fit doesn't seem fit from your perspective.
>
> >>> and the logic would be harder to comprehend.
> >> Keeping the layering clean is critical to comprehension.
> >> There's a lot of VFS code that could have been implemented within the
> >> LSM infrastructure, but I don't think that anyone would argue that it
> >> should have been.
> >>
> >>> So that's a trade-off.
> >> I remain completely unconvinced that your proposal represents a good way
> >> to implement you scheme.
> >>
> >>> Then what's the practical drawback of doing that?
> >> Name space pollution.
> > Alright, I can fix the names.
>
> Good!
>
>
> >> Layering violation.
> > Not sure what you are referring to.
>
> The only places where the blob freed by security_file_free()
> may be allocated is security_file_alloc(). The security modules
> are welcome to do anything they like in addition, provided
> they clean up after themselves in their file_free() hooks.
Exactly!
Like I said, allocation could happen in security_file_alloc() but I did it in security_enclave_load() to avoid unnecessary allocation for non-enclaves.
I know "security" looks close to "selinux" but I beg your attention in the function names. Whatever allocated inside SELinux *never* gets freed by the infrastructure, except those implicit allocations due to EMA splits.
>
> If SELinux wants to support controls on enclave information,
> and that requires additional data, SELinux should include
> space in its file blob for that information, or a pointer to
> the place where the enclave code is maintaining it.
>
> That's the way audit works.
I have to repeat myself. This was what v1 does.
The drawback is, there could be multiple LSMs active at the same time. And an EMA is inserted iff *all* LSMs have approved it. Thus the actual insertion is now done at the end of security_enclave_load(). That makes the logic clear and less error prone, and saves code that'd be duplicated into multiple LSMs otherwise. And that's why I cited security_file_permission()/security_vm_enough_memory() as precedence to security_enclave_load().
>
> > If you are referring to buffers allocated in one layer and freed in elsewhere, you have
> got the code wrong. Buffers allocated in security_enclave_load() is freed in
> security_file_free().
>
> It's up to the security module's file_free() to clean up anything that
> wasn't allocated in security_file_free(). Interested security modules
> should call enclave_load(), and put the information into their portion
> of the security blob. The module specific code can call enclave_file_free(),
> or whatever interface you want to provide, to clean up. That might take
> place in file_free(), but it also might be elsewhere.
Would you please take a closer look at my code? Whatever I added to SELinux did *not* allocate anything! Or are you talking about something else?
>
>
> > Whatever else allocated in LSMs are not seen or taken care of by the infrastructure. The
> purpose of allocating EMAs in enclave_load() is trying to minimize overhead for non-
> enclave files, otherwise it could be done in file_alloc() to be more "paired" with
> file_free(). But I don't see it necessary.
>
> Try looking at maintaining what you've put into the LSM code as
> a separate entity. It makes it simpler. Really.
It is already a separate entity. It has its own header and own C file.
>
> >> Architecture specific implementation detail in a general infrastructure.
> > Stated earlier, it doesn't contain any h/w specifics but just a TEE abstraction.
>
> Then put it in the TEE system.
It's LSM's abstraction of TEE - i.e., it tracks what matters to LSM only. TEE doesn't care. It just provides information and asks for a decision at return. That's how LSM works.
>
> > It could be left on all the time or controlled by a different config macro.
>
> True in any case.
>
> > It is contingent to CONFIG_INTEL_SGX just for convenience, as SGX is the first (and only
> so far) TEE that needs attention from LSM, but there could be more in future.
>
> All the more reason to keep it separate. These things never get simpler
> when they get more generalized.
I have a hard time understanding what you mean by "separate".
>
> >>> If no, why would we want to pay for the cost for not doing that?
> >> Modularity and maintainability come directly to mind.
> > Putting it elsewhere will incur more maintenance cost.
>
> I don't believe that for a second. 40 years of C programming
> have taught me that trying to do multiple things in one place
> is always a bad idea.
Agreed. I'm doing only one thing.
>
>
> >>>> ... and the notion that you allocate data for one blob that gets
> >>>> freed relative to another breaks the data management model.
> >>> What do you mean here?
> >> You're freeing the EMA data from security_file_free().
> >> If selinux wants to free EMA data it has allocated in
> >> selinux_enclave_load() in selinux_file_free() that's fine, but the LSM
> >> infrastructure has no need to know about it.
> >> EMA needs to manage its own data, just like VFS does.
> >> The LSM infrastructure provides blob management so that the security
> >> modules can extend data if they want to.
> > You've got the code wrong. selinux_enclave_load() doesn't allocate any memory.
> selinux_file_mprotect() may, due to EMA split. But that's transparent to all LSMs.
>
> ... and the LSM infrastructure doesn't care and
> must not be made to care. It's all up to SELinux.
It doesn't care the decisions. But it assists in maintaining information on which decisions (from multiple LSMs) are based.
>
> > The LSM infrastructure doesn't know anything about what LSM modules do, nor does it
> manage any buffers allocated by any LSM modules.
>
> Right, which is why putting your lsm_ema_blob is wrong, and why
> forcing into the file blob is wrong.
lsm_ema_blob is NOT part of file blob.
>
> > EMA is currently managing its own data. What's needed is the trigger - to let EMA know
> when to update its states. The trigger could be placed in LSM infrastructure or inside
> individual LSMs.
>
> Yes. The latter.
>
> > The reason to put it in the infrastructure, is that it depends on the decision of *all*
> LSMs whether to insert a new EMA.
>
> That's basic stacking behavior. "Bail on fail", which says that once
> denial is detected, you're done.
Who does the insertion on success, if not the LSM infrastructure? This is again similar to security_file_permission/security_vm_enough_memory. The last step is done by the infrastructure on success.
>
> > That's similar to vm_enough_memory() where the final __vm_enough_memory() call is made
> by the infrastructure but not individual LSMs.
>
> Do you really understand the painful reasons that case is required?
> And if so, why you aren't taking steps to avoid them?
I think I've run into the same painful reasons.
Honestly, I tried not to do anything more than just a call_int_hooks. But I realized that'd make thing much more complicated and error prone in multiple-active-LSM cases.
So I think I've run into the same painful reasons. And I don't see any actionable suggestions from you so far.
>
>
> >>> EMA blobs are allocated/freed *not* relative to any other blobs.
> >> In the code you proposed they are freed in security_file_free().
> >> That is for file blob management.
> > Yes. EMA contributes to the file blob. But it only frees memory allocated by the
> infrastructure itself, not anything from any LSM modules.
>
> That's not the way it's supposed to be done. The module tells
> the infrastructure what it needs, which may include space for
> EMA data. The module asks EMA for the data it needs and stuffs
> it somewhere, and the file blob is a fine choice. The module
> cleans up in file_free, or at any time before that. If no module
> uses EMA, nothing goes in the blob. If two modules use EMA each
> is responsible for the data it uses, which may be the same or
> may be different.
>
> I've looked at your code. Making it work the way it should would
> not be difficult and would likely simplify a bunch of it.
Guess this discussion will never end if we don't get into code. Guess it'd be more productive to talk over phone then come back to this thread with a conclusion. Will that be ok with you?
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-02 7:42 ` Xing, Cedric@ 2019-07-02 15:44 ` Casey Schaufler
2019-07-03 9:46 ` Dr. Greg0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-02 15:44 UTC (permalink / raw)
To: Xing, Cedric, Stephen Smalley, casey
Cc: linux-sgx, linux-security-module, selinux, Schaufler, Casey,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen,
Christopherson, Sean J
On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> ...
> Guess this discussion will never end if we don't get into code. Guess it'd be more productive to talk over phone then come back to this thread with a conclusion. Will that be ok with you?
I don't think that a phone call is going to help. Talking
code issues tends to muddle them in my brain. If you can give
me a few days I will propose a rough version of how I think
your code should be integrated into the LSM environment. I'm
spending more time trying (unsuccessfully :( ) to discribe
the issues in English than it will probably take in C.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-02 15:44 ` Casey Schaufler@ 2019-07-03 9:46 ` Dr. Greg
2019-07-03 15:32 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Dr. Greg @ 2019-07-03 9:46 UTC (permalink / raw)
To: Casey Schaufler
Cc: Xing, Cedric, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen, Christopherson, Sean J
On Tue, Jul 02, 2019 at 08:44:40AM -0700, Casey Schaufler wrote:
Good morning, I hope this note finds the week going well for everyone.
> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> > ...
> > Guess this discussion will never end if we don't get into
> > code. Guess it'd be more productive to talk over phone then come back
> > to this thread with a conclusion. Will that be ok with you?
> I don't think that a phone call is going to help. Talking code
> issues tends to muddle them in my brain. If you can give me a few
> days I will propose a rough version of how I think your code should
> be integrated into the LSM environment. I'm spending more time
> trying (unsuccessfully :( ) to discribe the issues in English than
> it will probably take in C.
While Casey is off writing his rosetta stone, let me suggest that the
most important thing we need to do is to take a little time, step back
and look at the big picture with respect to what we are trying to
accomplish and if we are going about it in a way that makes any sense
from an engineering perspective.
This conversation shouldn't be about SGX, it should be about the best
way for the kernel/LSM to discipline a Trusted Execution Environment
(TEE). As I have noted previously, a TEE is a 'blackbox' that, by
design, is intended to allow execution of code and processing of data
in a manner that is resistant to manipulation or inspection by
untrusted userspace, the kernel and/or the hardware itself.
Given that fact, if we are to be intellectually honest, we need to ask
ourselves how effective we believe we can be in controlling any TEE
with kernel based mechanisms. This is particularly the case if the
author of any code running in the TEE has adversarial intent.
Here is the list of controls that we believe an LSM can, effectively,
implement against a TEE:
1.) Code provenance and origin.
2.) Cryptographic verification of dynamically executable content.
2.) The ability of a TEE to implement anonymous executable content.
If people are in agreement with this concept, it is difficult to
understand why we should be implementing complex state machines and
the like, whether it is in the driver or the LSM. Security code has
to be measured with a metric of effectiveness, otherwise we are
engaging in security theater.
I believe that if we were using this lens, we would already have a
mainline SGX driver, since we seem to have most of the needed LSM
infrastructure and any additional functionality would be a straight
forward implementation. Most importantly, the infrastructure would
not be SGX specific, which would seem to be a desirable political
concept.
If we are not willing to engage in this discussion we are going to end
up with a quasi-technology specific solution that isn't implementing
any relevant security guarantees.
FWIW, we wouldn't even be having this, now lengthy discussion, if I
wouldn't have aggressively advocated, starting last November, that an
SGX driver needed some form of execution control if there was a desire
for the technology to not pose a security risk to the platform. So
humor me a little bit.... :-)
Best wishes for a productive remainder of the week to everyone.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC
4206 N. 19th Ave. Implementing measured information privacy
Fargo, ND 58102 and integrity architectures.
PH: 701-281-1686
FAX: 701-281-3949 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"... remember that innovation is saying 'no' to 1000 things."
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-03 9:46 ` Dr. Greg@ 2019-07-03 15:32 ` Casey Schaufler
2019-07-07 13:30 ` Dr. Greg0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-03 15:32 UTC (permalink / raw)
To: Dr. Greg, casey
Cc: Xing, Cedric, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen, Christopherson, Sean J
On 7/3/2019 2:46 AM, Dr. Greg wrote:
> On Tue, Jul 02, 2019 at 08:44:40AM -0700, Casey Schaufler wrote:
>
> Good morning, I hope this note finds the week going well for everyone.
>
>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
>>> ...
>>> Guess this discussion will never end if we don't get into
>>> code. Guess it'd be more productive to talk over phone then come back
>>> to this thread with a conclusion. Will that be ok with you?
>> I don't think that a phone call is going to help. Talking code
>> issues tends to muddle them in my brain. If you can give me a few
>> days I will propose a rough version of how I think your code should
>> be integrated into the LSM environment. I'm spending more time
>> trying (unsuccessfully :( ) to discribe the issues in English than
>> it will probably take in C.
> While Casey is off writing his rosetta stone,
I'd hardly call it that. More of an effort to round the
corners on the square peg. And Cedric has some ideas on
how to approach that.
> let me suggest that the
> most important thing we need to do is to take a little time, step back
> and look at the big picture with respect to what we are trying to
> accomplish and if we are going about it in a way that makes any sense
> from an engineering perspective.
>
> This conversation shouldn't be about SGX, it should be about the best
> way for the kernel/LSM to discipline a Trusted Execution Environment
> (TEE). As I have noted previously, a TEE is a 'blackbox' that, by
> design, is intended to allow execution of code and processing of data
> in a manner that is resistant to manipulation or inspection by
> untrusted userspace, the kernel and/or the hardware itself.
>
> Given that fact, if we are to be intellectually honest, we need to ask
> ourselves how effective we believe we can be in controlling any TEE
> with kernel based mechanisms. This is particularly the case if the
> author of any code running in the TEE has adversarial intent.
>
> Here is the list of controls that we believe an LSM can, effectively,
> implement against a TEE:
>
> 1.) Code provenance and origin.
>
> 2.) Cryptographic verification of dynamically executable content.
>
> 2.) The ability of a TEE to implement anonymous executable content.
>
> If people are in agreement with this concept, it is difficult to
> understand why we should be implementing complex state machines and
> the like, whether it is in the driver or the LSM. Security code has
> to be measured with a metric of effectiveness, otherwise we are
> engaging in security theater.
>
> I believe that if we were using this lens, we would already have a
> mainline SGX driver, since we seem to have most of the needed LSM
> infrastructure and any additional functionality would be a straight
> forward implementation. Most importantly, the infrastructure would
> not be SGX specific, which would seem to be a desirable political
> concept.
Generality introduced in the absence of multiple instances
often results in unnecessary complexity, unused interfaces
and feature compromise. Guessing what other TEE systems might
do, and constraining SGX to those models (or the other way around)
is a well established road to ruin. The LSM infrastructure is
a fine example. For the first ten years the "general" mechanism
had a single user. I'd say to hold off on the general until there
is more experience with the specific. It's easier to construct
a general mechanism around things that work than to fit things
that need to work into some preconceived notion of generality.
>
> If we are not willing to engage in this discussion we are going to end
> up with a quasi-technology specific solution that isn't implementing
> any relevant security guarantees.
>
> FWIW, we wouldn't even be having this, now lengthy discussion, if I
> wouldn't have aggressively advocated, starting last November, that an
> SGX driver needed some form of execution control if there was a desire
> for the technology to not pose a security risk to the platform. So
> humor me a little bit.... :-)
>
> Best wishes for a productive remainder of the week to everyone.
>
> Dr. Greg
>
> As always,
> Dr. Greg Wettstein, Ph.D, Worker
> IDfusion, LLC
> 4206 N. 19th Ave. Implementing measured information privacy
> Fargo, ND 58102 and integrity architectures.
> PH: 701-281-1686
> FAX: 701-281-3949 EMAIL: greg@idfusion.net
> ------------------------------------------------------------------------------
> "... remember that innovation is saying 'no' to 1000 things."
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 0/3] security/x86/sgx: SGX specific LSM hooks
2019-07-03 23:22 ` Jarkko Sakkinen@ 2019-07-03 23:23 ` Jarkko Sakkinen0 siblings, 0 replies; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-07-03 23:23 UTC (permalink / raw)
To: Cedric Xing
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, sean.j.christopherson
On Thu, Jul 04, 2019 at 02:22:21AM +0300, Jarkko Sakkinen wrote:
> > The eye should be on whether the uapi (e.g. device files, ioctl's) will
> > work for LSM's in a legit way. Do we need more of these different
> > flavors of experimental LSM changes or can we make some conclusions with
> > the real issue we are trying to deal with?
>
> Anyway, sending v21 soonish. Finished it on Thu but have been waiting
> any internal QA feedback. If nothing pops up, I'll send it tmrw.
Ugh, the point I forgot to add was that it contains update to
SGX_IOC_ENCLAVE_ADD_PAGE that is relevant for the discussion (probably
the same as Sean proposed cannot recall if I did tuning to it).
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-06-19 22:23 [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Sean Christopherson
` (16 preceding siblings ...)
2019-06-27 18:56 ` [RFC PATCH v2 3/3] x86/sgx: Implement SGX specific hooks in SELinux Cedric Xing
@ 2019-07-05 16:05 ` Jarkko Sakkinen
2019-07-08 17:29 ` Sean Christopherson17 siblings, 1 reply; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-07-05 16:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
I still don't get why we need this whole mess and do not simply admit
that there are two distinct roles:
1. Creator
2. User
In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
User does not. It just gets the fd from the Creator. I'm sure that all
the SGX2 related functionality can be solved somehow in this role
playing game.
An example would be the usual case where enclave is actually a loader
that loads the actual piece of software that one wants to run. Things
simply need to be designed in a way the Creator runs the loader part.
These are non-trivial problems but oddball security model is not going
to make them disappear - on the contrary it will make designing user
space only more complicated.
I think this is classical example of when something overly complicated
is invented in the kernel only to realize that it should be solved in
the user space.
It would not be like the only use case where some kind of privileged
daemon is used for managing some a kernel provided resource.
I think a really good conclusion from this discussion that has taken two
months is to realize that nothing needs to be done in this area (except
*maybe* noexec check).
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 0/3] security/x86/sgx: SGX specific LSM hooks
2019-07-03 23:16 ` Jarkko Sakkinen
2019-07-03 23:22 ` Jarkko Sakkinen@ 2019-07-06 5:04 ` Xing, Cedric
2019-07-08 14:46 ` Jarkko Sakkinen1 sibling, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-06 5:04 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, sean.j.christopherson
On 7/3/2019 4:16 PM, Jarkko Sakkinen wrote:
> On Thu, Jun 27, 2019 at 11:56:18AM -0700, Cedric Xing wrote:
>
> I think it is fine to have these patch sets as a discussion starters but
> it does not make any sense to me to upstream LSM changes with the SGX
> foundations.
Guess LSM is a gating factor, because otherwise SGX could be abused to
make executable EPC from pages that are otherwise not allowed to be
executable. Am I missing anything?
>
> This is exactly the same situation as with KVM changes. The patch set is
> already way too big to fit to the standards [1].
>
> The eye should be on whether the uapi (e.g. device files, ioctl's) will
> work for LSM's in a legit way. Do we need more of these different
> flavors of experimental LSM changes or can we make some conclusions with
> the real issue we are trying to deal with?
>
> [1] "Do not send more than 15 patches at once to the vger mailing lists!!!"
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#select-the-recipients-for-your-patch
>
> /Jarkko
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-03 15:32 ` Casey Schaufler@ 2019-07-07 13:30 ` Dr. Greg
2019-07-09 0:02 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Dr. Greg @ 2019-07-07 13:30 UTC (permalink / raw)
To: Casey Schaufler
Cc: Xing, Cedric, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen, Christopherson, Sean J
On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
Good morning, I hope the weekend has been enjoyable for everyone.
> >> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> >>> ...
> >>> Guess this discussion will never end if we don't get into
> >>> code. Guess it'd be more productive to talk over phone then come back
> >>> to this thread with a conclusion. Will that be ok with you?
> >> I don't think that a phone call is going to help. Talking code
> >> issues tends to muddle them in my brain. If you can give me a few
> >> days I will propose a rough version of how I think your code should
> >> be integrated into the LSM environment. I'm spending more time
> >> trying (unsuccessfully :( ) to discribe the issues in English than
> >> it will probably take in C.
> > While Casey is off writing his rosetta stone,
> I'd hardly call it that. More of an effort to round the corners on
> the square peg. And Cedric has some ideas on how to approach that.
Should we infer from this comment that, of the two competing
strategies, Cedric's is the favored architecture?
> > let me suggest that the
> > most important thing we need to do is to take a little time, step back
> > and look at the big picture with respect to what we are trying to
> > accomplish and if we are going about it in a way that makes any sense
> > from an engineering perspective.
> >
> > This conversation shouldn't be about SGX, it should be about the best
> > way for the kernel/LSM to discipline a Trusted Execution Environment
> > (TEE). As I have noted previously, a TEE is a 'blackbox' that, by
> > design, is intended to allow execution of code and processing of data
> > in a manner that is resistant to manipulation or inspection by
> > untrusted userspace, the kernel and/or the hardware itself.
> >
> > Given that fact, if we are to be intellectually honest, we need to ask
> > ourselves how effective we believe we can be in controlling any TEE
> > with kernel based mechanisms. This is particularly the case if the
> > author of any code running in the TEE has adversarial intent.
> >
> > Here is the list of controls that we believe an LSM can, effectively,
> > implement against a TEE:
> >
> > 1.) Code provenance and origin.
> >
> > 2.) Cryptographic verification of dynamically executable content.
> >
> > 2.) The ability of a TEE to implement anonymous executable content.
> >
> > If people are in agreement with this concept, it is difficult to
> > understand why we should be implementing complex state machines and
> > the like, whether it is in the driver or the LSM. Security code has
> > to be measured with a metric of effectiveness, otherwise we are
> > engaging in security theater.
> >
> > I believe that if we were using this lens, we would already have a
> > mainline SGX driver, since we seem to have most of the needed LSM
> > infrastructure and any additional functionality would be a straight
> > forward implementation. Most importantly, the infrastructure would
> > not be SGX specific, which would seem to be a desirable political
> > concept.
> Generality introduced in the absence of multiple instances
> often results in unnecessary complexity, unused interfaces
> and feature compromise. Guessing what other TEE systems might
> do, and constraining SGX to those models (or the other way around)
> is a well established road to ruin. The LSM infrastructure is
> a fine example. For the first ten years the "general" mechanism
> had a single user. I'd say to hold off on the general until there
> is more experience with the specific. It's easier to construct
> a general mechanism around things that work than to fit things
> that need to work into some preconceived notion of generality.
All well taken points from an implementation perspective, but they
elide the point I was trying to make. Which is the fact that without
any semblance of a discussion regarding the requirements needed to
implement a security architecture around the concept of a TEE, this
entire process, despite Cedric's well intentioned efforts, amounts to
pounding a square solution into the round hole of a security problem.
Which, as I noted in my e-mail, is tantamount to security theater.
Everyone wants to see this driver upstream. If we would have had a
reasoned discussion regarding what it means to implement proper
controls around a TEE, when we started to bring these issues forward
last November, we could have possibly been on the road to having a
driver with reasoned security controls and one that actually delivers
the security guarantees the hardware was designed to deliver.
Best wishes for a productive week to everyone.
Dr. Greg
As always,
Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
4206 N. 19th Ave. Specializing in information infra-structure
Fargo, ND 58102 development.
PH: 701-281-1686 EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Any intelligent fool can make things bigger and more complex... It
takes a touch of genius - and a lot of courage to move in the opposite
direction."
-- Albert Einstein
^permalinkrawreply [flat|nested] 156+ messages in thread

*[RFC PATCH v3 0/4] security/x86/sgx: SGX specific LSM hooks
2019-06-27 18:56 ` [RFC PATCH v2 0/3] security/x86/sgx: SGX specific LSM hooks Cedric Xing
2019-07-03 23:16 ` Jarkko Sakkinen@ 2019-07-07 23:41 ` " Cedric Xing
2019-07-08 15:55 ` Sean Christopherson
2019-07-07 23:41 ` [RFC PATCH v3 1/4] x86/sgx: Add " Cedric Xing
` (3 subsequent siblings)5 siblings, 1 reply; 156+ messages in thread
From: Cedric Xing @ 2019-07-07 23:41 UTC (permalink / raw)
To: linux-sgx, linux-security-module, selinux
Cc: Cedric Xing, casey.schaufler, jmorris, luto, jethro, greg, sds,
jarkko.sakkinen, sean.j.christopherson
This series intends to make the new SGX subsystem to work with the existing LSM
architecture smoothly so that, say, SGX cannot be abused to work around
restrictions set forth by LSM modules/policies.
This patch is based on and could be applied cleanly on top of Jarkko Sakkinen’s
SGX patch series v20 (https://patchwork.kernel.org/cover/10905141/).
For those who haven’t followed closely, the whole discussion started from the
primary question of how to prevent creating an executable enclave page from a
regular memory page that is NOT executable as prohibited by LSM
modules/policies. And that can be translated into 2 relating questions in
practice, i.e. 1) how to determine the allowed initial protections of enclave
pages when they are being loaded and 2) how to determine the allowed
protections of enclave pages at runtime. Those who are familiar with LSM may
notice that, for regular files, #1 is determined by security_mmap_file() while
#2 is covered by security_file_mprotect(). Those 2 hooks however are
insufficient for enclaves due to the distinct composition and lifespan of
enclave pages. Specifically, security_mmap_file() only passes in the file but
is not specific on which portion of the file being mmap()’ed, with the
assumption that all pages of the same file shall have the same set of
allowed/disallowed protections. But that assumption is no longer true for
enclaves for 2 reasons: a) pages of an enclave may be loaded from different
image files with different attributes and b) enclave pages retain contents
across munmap()/mmap(), therefore, say, if a policy prohibits execution of
modified pages, then pages flagged modified have to stay modified across
munmap()/mmap() so that the policy cannot be circumvented by remapping (i.e.
munmap() followed by mmap() on the same range). But the lack of range
information in security_mmap_file()’s arguments simply blocks LSM modules from
tracking enclave pages properly.
A rational solution would always involve tracking the correspondence between
enclave pages and their origin (e.g. files from which they were loaded), which
is similar to tracking regular memory pages and their origin via vm_file of
struct vm_area_struct. But given the longer lifespan of enclave pages (than
VMAs they are mapped into), such correspondence has to be stored in a separate
data structure outside of VMAs. In theory, the correspondence could be stored
either in LSM or in the SGX subsystem. This series has picked the former
because firstly, such information is useful only within LSM so it makes more
sense to keep it as “LSM internal” and secondly, keeping the data structure
inside LSM would allow additional information to be cached in LSM modules
without affecting the rest of the kernel, while lastly, those data structures
would be gone when LSM is disabled hence would not impose any unnecessary
overhead.
Those who are familiar with this topic and related discussions may also notice
that, Sean Christopherson has sent out an RFC patch recently to address the
same problem as this series. He adopted the other approach of tracking
page/origin correspondence inside the SGX subsystem. However, to reduce memory
overhead in practice, he cached the FSM (Finite State Machine) instead of
page/origin correspondences. By “FSM”, I mean policy FSM defined as sets of
states and events that may trigger state transitions. Generally speaking, any
LSM module has its own definition of FSM and usually uses attributes attached
to files to argument the FSM, then it advances the FSM as events are observed
and gives out decision based on the current FSM state. Sean’s implementation
attempts to move the FSM into the SGX subsystem, and by caching the arguments
returned by LSM it tries to monitor events and reach the same decisions by
itself. So from architecture perspective, that model has to face tough
challenges in reality, such as how to support multiple LSM modules that employ
different FSMs to govern page protection transitions. Implementation wise, his
model also imposes unwanted restrictions specifically to SGX2, such as:
· Complicated/Restricted UAPI – Enclave loaders are required to provide
“maximal protection” at page load time, but such information is NOT always
available. For example, Graphene containers may run different applications
comprised of different set of executables and/or shared objects. Some of
them may contain self-modifying code (or text relocation) while others
don’t. The generic enclave loader usually doesn’t have such information so
wouldn’t be able to provide it ahead of time.
· Inefficient Auditing – Audit logs are supposed to help system
administrators to determine the set of minimally needed permissions and to
detect abnormal behaviors. But consider the “maximal protection” model, if
“maximal protection” is set to be too permissive, then audit log wouldn’t
be able to detect anomalies; or if “maximal protection” is too restrictive,
then audit log cannot identify the file violating the policy. In either
case the audit log cannot fulfill its purposes.
· Inability to support #PF driven EPC allocation in SGX2 – For those
unfamiliar with SGX2 software flows, an SGX2 enclave requests a page by
issuing EACCEPT on the address that a new page is wanted, and the resulted
#PF is expected to be handled by the kernel by EAUG’ing an EPC page at the
fault address, and then the enclave would be resumed and the faulting
EACCEPT retried, and succeed. The key requirement is to allow mmap()’ing
non-existing enclave pages so that the SGX module/subsystem could respond
to #PFs by EAUG’ing new pages. Sean’s implementation doesn’t allow
mmap()’ing non-existing pages for variety of reasons and therefore blocks
this major SGX2 usage.
Please note that this series has only been compile-tested.
History:
· This is version 3 of this patch series, with the following changes over
version 2 per comments/requests from the community.
- Per Casey Schaufler, moved EMA from the LSM infrastructure into a new LSM
module, named “ema”, which is responsible for maintaining EMA maps for
enclaves and offers APIs for other LSM modules to query/update EMA nodes.
- Per Stephen Smalley, removed kernel command line option
“lsm.ema.cache_decisions”. Enclave page origins will always be tracked
and audit logs will always be accurate.
- Per Andy Lutomirski, a new PROCESS2__ENCLAVE_EXECANON permission has been
added to SELinux to allow EADD’ing executable pages from non-executable
anonymous source pages.
- Revised permission checks for enclave pages in SELinux. See
selinux_enclave_load() and enclave_mprotect() functions in
security/selinux/hooks.c for details.
· v2 – https://patchwork.kernel.org/cover/11020301/
· v1 – https://patchwork.kernel.org/cover/10984127/
Cedric Xing (4):
x86/sgx: Add SGX specific LSM hooks
x86/64: Call LSM hooks from SGX subsystem/module
X86/sgx: Introduce EMA as a new LSM module
x86/sgx: Implement SGX specific hooks in SELinux
arch/x86/kernel/cpu/sgx/driver/ioctl.c | 80 ++++++-
arch/x86/kernel/cpu/sgx/driver/main.c | 16 +-
include/linux/lsm_ema.h | 97 +++++++++
include/linux/lsm_hooks.h | 27 +++
include/linux/security.h | 23 ++
security/Makefile | 1 +
security/commonema.c | 277 +++++++++++++++++++++++++
security/security.c | 17 ++
security/selinux/hooks.c | 236 ++++++++++++++++++++-
security/selinux/include/classmap.h | 3 +-
security/selinux/include/objsec.h | 7 +
11 files changed, 770 insertions(+), 14 deletions(-)
create mode 100644 include/linux/lsm_ema.h
create mode 100644 security/commonema.c
--
2.17.1
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 0/4] security/x86/sgx: SGX specific LSM hooks
2019-07-07 23:41 ` [RFC PATCH v3 0/4] " Cedric Xing
@ 2019-07-08 15:55 ` Sean Christopherson
2019-07-08 17:49 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Sean Christopherson @ 2019-07-08 15:55 UTC (permalink / raw)
To: Cedric Xing
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen
On Sun, Jul 07, 2019 at 04:41:30PM -0700, Cedric Xing wrote:
...
> different FSMs to govern page protection transitions. Implementation wise, his
> model also imposes unwanted restrictions specifically to SGX2, such as:
> · Complicated/Restricted UAPI – Enclave loaders are required to provide
I don't think "complicated" is a fair assessment. For SGX1 enclaves it's
literally a direct propagation of the SECINFO RWX flags.
> “maximal protection” at page load time, but such information is NOT always
> available. For example, Graphene containers may run different applications
> comprised of different set of executables and/or shared objects. Some of
> them may contain self-modifying code (or text relocation) while others
> don’t. The generic enclave loader usually doesn’t have such information so
> wouldn’t be able to provide it ahead of time.
I'm unconvinced that it would be remotely difficult to teach an enclave
loader that an enclave or hosted application employs SMC, relocation or
any other behavior that would require declaring RWX on all pages.
> · Inefficient Auditing – Audit logs are supposed to help system
> administrators to determine the set of minimally needed permissions and to
> detect abnormal behaviors. But consider the “maximal protection” model, if
> “maximal protection” is set to be too permissive, then audit log wouldn’t
> be able to detect anomalies;
Huh? Declaring overly permissive protections is only problematic if an
LSM denies the permission, in which case it will generate an accurate
audit log.
If the enclave/loader "requires" a permission it doesn't actually need,
e.g. EXECDIRTY, then it's a software bug that should be fixed. I don't
see how this scenario is any different than an application that uses
assembly code without 'noexecstack' and inadvertantly "requires"
EXECSTACK due to triggering "read implies exec". In both cases the
denied permission is unnecessary due to a userspace application bug.
> or if “maximal protection” is too restrictive,
> then audit log cannot identify the file violating the policy.
Maximal protections that are too restrictive are completely orthogonal to
LSMs as the enclave would fail to run irrespective of LSMs. This is no
different than specifying the wrong RWX flags in SECINFO, or opening a
file as RO instead of RW.
> In either case the audit log cannot fulfill its purposes.
> · Inability to support #PF driven EPC allocation in SGX2 – For those
> unfamiliar with SGX2 software flows, an SGX2 enclave requests a page by
> issuing EACCEPT on the address that a new page is wanted, and the resulted
> #PF is expected to be handled by the kernel by EAUG’ing an EPC page at the
> fault address, and then the enclave would be resumed and the faulting
> EACCEPT retried, and succeed. The key requirement is to allow mmap()’ing
> non-existing enclave pages so that the SGX module/subsystem could respond
> to #PFs by EAUG’ing new pages. Sean’s implementation doesn’t allow
> mmap()’ing non-existing pages for variety of reasons and therefore blocks
> this major SGX2 usage.
This is simply wrong. The key requirement in the theoretical EAUG scheme
is to mmap() pages that have not been added to the _hardware_ maintained
enclave. The pages (or some optimized representation of a range of pages)
would exist in the kernel's software mode of the enclave.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-08 16:26 ` Casey Schaufler@ 2019-07-08 17:16 ` Xing, Cedric
2019-07-08 23:53 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-08 17:16 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
On 7/8/2019 9:26 AM, Casey Schaufler wrote:
> In this scheme you use an ema LSM to manage your ema data.
> A quick sketch looks like:
>
> sgx_something_in() calls
> security_enclave_load() calls
> ema_enclave_load()
> selinux_enclave_load()
> otherlsm_enclave_load()
>
> Why is this better than:
>
> sgx_something_in() calls
> ema_enclave_load()
> security_enclave_load() calls
> selinux_enclave_load()
> otherlsm_enclave_load()
Are you talking about moving EMA somewhere outside LSM? If so, where?
>
>
> If you did really want ema to behave like an LSM
> you would put the file data that SELinux is managing
> into the ema portion of the blob and provide interfaces
> for the SELinux (or whoever) to use that. Also, it's
> an abomination (as I've stated before) for ema to
> rely on SELinux to provide a file_free() hook for
> ema's data. If you continue down the LSM route, you
> need to provide an ema_file_free() hook. You can't
> count on SELinux to do it for you. If there are multiple
> LSMs (coming soon!) that use the ema data, they'll all
> try to free it, and then Bad Things can happen.
I'm afraid you have misunderstood the code. What is kept open and gets
closed in selinux_file_free() is the sigstruct file. SELinux uses it to
determine the page permissions for enclave pages from anonymous sources.
It is a policy choice made inside SELinux and has nothing to do with EMA.
There's indeed an ema_file_free_security() to free the EMA map for
enclaves being closed. EMA does *NOT* rely on any other LSMs to free
data for it. The only exception is when an LSM fails enclave_load(), it
has to call ema_remove_range() to remove the range being added, which
was *not* required originally in v2.
>> +/**
>> + * ema - Enclave Memory Area structure for LSM modules
>
> LSM modules is redundant. "LSM" or "LSMs" would be better.
Noted
>> diff --git a/security/Makefile b/security/Makefile
>> index c598b904938f..b66d03a94853 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>
> The config option and the file name ought to match,
> or at least be closer.
Just trying to match file names as "capability" uses commoncap.c.
Like I said, this feature could potentially be used by TEEs other than
SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I
can rename it to ema.c or enclave.c. Do you have a preference?
>> diff --git a/security/commonema.c b/security/commonema.c
>
> Put this in a subdirectory. Please.
Then why is commoncap.c located in this directory? I'm just trying to
match the existing convention.
>> +static struct lsm_blob_sizes ema_blob_sizes __lsm_ro_after_init = {
>> + .lbs_file = sizeof(atomic_long_t),
>> +};
>
> If this is ema's data ema must manage it. You *must* have
> a file_free().
There is one indeed - ema_file_free_security().
>
>> +
>> +static atomic_long_t *_map_file(struct file *encl)
>> +{
>> + return (void *)((char *)(encl->f_security) + ema_blob_sizes.lbs_file);
>
> I don't trust all the casting going on here, especially since
> you don't end up with the type you should be returning.
Will change.
>> +}
>> +
>> +static struct ema_map *_alloc_map(void)
>
> Function header comments, please.
Will add.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-05 16:05 ` [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM Jarkko Sakkinen
@ 2019-07-08 17:29 ` Sean Christopherson
2019-07-08 17:33 ` Xing, Cedric
` (3 more replies)0 siblings, 4 replies; 156+ messages in thread
From: Sean Christopherson @ 2019-07-08 17:29 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>
> I still don't get why we need this whole mess and do not simply admit
> that there are two distinct roles:
>
> 1. Creator
> 2. User
Because SELinux has existing concepts of EXECMEM and EXECMOD.
> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
> User does not. It just gets the fd from the Creator. I'm sure that all
> the SGX2 related functionality can be solved somehow in this role
> playing game.
>
> An example would be the usual case where enclave is actually a loader
> that loads the actual piece of software that one wants to run. Things
> simply need to be designed in a way the Creator runs the loader part.
> These are non-trivial problems but oddball security model is not going
> to make them disappear - on the contrary it will make designing user
> space only more complicated.
>
> I think this is classical example of when something overly complicated
> is invented in the kernel only to realize that it should be solved in
> the user space.
>
> It would not be like the only use case where some kind of privileged
> daemon is used for managing some a kernel provided resource.
>
> I think a really good conclusion from this discussion that has taken two
> months is to realize that nothing needs to be done in this area (except
> *maybe* noexec check).
Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
That being said, we can do so without functional changes to the SGX uapi,
e.g. add reserved fields so that the initial uapi can be extended *if* we
decide to go with the "userspace provides maximal protections" path, and
use the EPCM permissions as the maximal protections for the initial
upstreaming.
That'd give us a minimal implemenation for initial upstreaming and would
eliminate Cedric's blocking complaint. The "whole mess" of whitelisting,
blacklisting and SGX2 support would be deferred until post-upstreaming.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-08 17:29 ` Sean Christopherson@ 2019-07-08 17:33 ` Xing, Cedric
2019-07-09 16:22 ` Jarkko Sakkinen
` (2 subsequent siblings)3 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-08 17:33 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein, Stephen Smalley
On 7/8/2019 10:29 AM, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>
>> I still don't get why we need this whole mess and do not simply admit
>> that there are two distinct roles:
>>
>> 1. Creator
>> 2. User
>
> Because SELinux has existing concepts of EXECMEM and EXECMOD.
>
>> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
>> User does not. It just gets the fd from the Creator. I'm sure that all
>> the SGX2 related functionality can be solved somehow in this role
>> playing game.
>>
>> An example would be the usual case where enclave is actually a loader
>> that loads the actual piece of software that one wants to run. Things
>> simply need to be designed in a way the Creator runs the loader part.
>> These are non-trivial problems but oddball security model is not going
>> to make them disappear - on the contrary it will make designing user
>> space only more complicated.
>>
>> I think this is classical example of when something overly complicated
>> is invented in the kernel only to realize that it should be solved in
>> the user space.
Are you talking about changing enclave loaders in user mode? That'd
break all existing code. I don't think we shall ever consider this approach.
>>
>> It would not be like the only use case where some kind of privileged
>> daemon is used for managing some a kernel provided resource.
>>
>> I think a really good conclusion from this discussion that has taken two
>> months is to realize that nothing needs to be done in this area (except
>> *maybe* noexec check).
>
> Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
>
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
>
> That'd give us a minimal implemenation for initial upstreaming and would
> eliminate Cedric's blocking complaint. The "whole mess" of whitelisting,
> blacklisting and SGX2 support would be deferred until post-upstreaming.
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 0/4] security/x86/sgx: SGX specific LSM hooks
2019-07-08 15:55 ` Sean Christopherson@ 2019-07-08 17:49 ` Xing, Cedric
2019-07-08 18:49 ` Sean Christopherson0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-08 17:49 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen
On 7/8/2019 8:55 AM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 04:41:30PM -0700, Cedric Xing wrote:
> ...
>
>> different FSMs to govern page protection transitions. Implementation wise, his
>> model also imposes unwanted restrictions specifically to SGX2, such as:
>> · Complicated/Restricted UAPI – Enclave loaders are required to provide
>
> I don't think "complicated" is a fair assessment. For SGX1 enclaves it's
> literally a direct propagation of the SECINFO RWX flags.
True only for SGX1.
>> “maximal protection” at page load time, but such information is NOT always
>> available. For example, Graphene containers may run different applications
>> comprised of different set of executables and/or shared objects. Some of
>> them may contain self-modifying code (or text relocation) while others
>> don’t. The generic enclave loader usually doesn’t have such information so
>> wouldn’t be able to provide it ahead of time.
>
> I'm unconvinced that it would be remotely difficult to teach an enclave
> loader that an enclave or hosted application employs SMC, relocation or
> any other behavior that would require declaring RWX on all pages.
You've been talking as if "enclave loader" is tailored to the enclave it
is loading. But in reality "enclave loader" is usually a library knowing
usually nothing about the enclave. How could it know if an enclave
contains self-modifying code?
>> · Inefficient Auditing – Audit logs are supposed to help system
>> administrators to determine the set of minimally needed permissions and to
>> detect abnormal behaviors. But consider the “maximal protection” model, if
>> “maximal protection” is set to be too permissive, then audit log wouldn’t
>> be able to detect anomalies;
>
> Huh? Declaring overly permissive protections is only problematic if an
> LSM denies the permission, in which case it will generate an accurate
> audit log.
>
> If the enclave/loader "requires" a permission it doesn't actually need,
> e.g. EXECDIRTY, then it's a software bug that should be fixed. I don't
> see how this scenario is any different than an application that uses
> assembly code without 'noexecstack' and inadvertantly "requires"
> EXECSTACK due to triggering "read implies exec". In both cases the
> denied permission is unnecessary due to a userspace application bug.
You see, you've been assuming "enclave loader" knows everything and
tailored to what it loads in a particular application. But the reality
is the loader is generic and probably shared by multiple applications.
It needs some generic way to figure out the "maximal protection". An
implementation could use information embedded in the enclave file, or
could just be "configurable". In the former case, you put extra burdens
on the build tools, while in the latter case, your audit logs cannot
help generating an appropriate configuration.
>> or if “maximal protection” is too restrictive,
>> then audit log cannot identify the file violating the policy.
>
> Maximal protections that are too restrictive are completely orthogonal to
> LSMs as the enclave would fail to run irrespective of LSMs. This is no
> different than specifying the wrong RWX flags in SECINFO, or opening a
> file as RO instead of RW.
Say loader is configurable. By looking at the log, can an administrator
tell which file has too restrictive "maximal protection"?
>> In either case the audit log cannot fulfill its purposes.
>> · Inability to support #PF driven EPC allocation in SGX2 – For those
>> unfamiliar with SGX2 software flows, an SGX2 enclave requests a page by
>> issuing EACCEPT on the address that a new page is wanted, and the resulted
>> #PF is expected to be handled by the kernel by EAUG’ing an EPC page at the
>> fault address, and then the enclave would be resumed and the faulting
>> EACCEPT retried, and succeed. The key requirement is to allow mmap()’ing
>> non-existing enclave pages so that the SGX module/subsystem could respond
>> to #PFs by EAUG’ing new pages. Sean’s implementation doesn’t allow
>> mmap()’ing non-existing pages for variety of reasons and therefore blocks
>> this major SGX2 usage.
>
> This is simply wrong. The key requirement in the theoretical EAUG scheme
> is to mmap() pages that have not been added to the _hardware_ maintained
> enclave. The pages (or some optimized representation of a range of pages)
> would exist in the kernel's software mode of the enclave.
You are right. Code can always change. My assessment was made on what's
in your patch.
The key point here is your patch is more complicated than it seems
because you've been hand-waving on imminent requirements.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 0/4] security/x86/sgx: SGX specific LSM hooks
2019-07-08 17:49 ` Xing, Cedric@ 2019-07-08 18:49 ` Sean Christopherson
2019-07-08 22:26 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Sean Christopherson @ 2019-07-08 18:49 UTC (permalink / raw)
To: Xing, Cedric
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen
On Mon, Jul 08, 2019 at 10:49:59AM -0700, Xing, Cedric wrote:
> On 7/8/2019 8:55 AM, Sean Christopherson wrote:
> >On Sun, Jul 07, 2019 at 04:41:30PM -0700, Cedric Xing wrote:
> True only for SGX1.
> >> “maximal protection” at page load time, but such information is NOT always
> >> available. For example, Graphene containers may run different applications
> >> comprised of different set of executables and/or shared objects. Some of
> >> them may contain self-modifying code (or text relocation) while others
> >> don’t. The generic enclave loader usually doesn’t have such information so
> >> wouldn’t be able to provide it ahead of time.
> >
> >I'm unconvinced that it would be remotely difficult to teach an enclave
> >loader that an enclave or hosted application employs SMC, relocation or
> >any other behavior that would require declaring RWX on all pages.
>
> You've been talking as if "enclave loader" is tailored to the enclave it is
> loading. But in reality "enclave loader" is usually a library knowing
> usually nothing about the enclave. How could it know if an enclave contains
> self-modifying code?
Given the rarity of SMC, require enclaves to declare "I do SMC"... The
Intel SDK already requires the enclave developer to declare heap size,
stack size, thread affinity, etc... I have a very hard time believing
that it can't support SMC and relocation flags.
> >> · Inefficient Auditing – Audit logs are supposed to help system
> >> administrators to determine the set of minimally needed permissions and to
> >> detect abnormal behaviors. But consider the “maximal protection” model, if
> >> “maximal protection” is set to be too permissive, then audit log wouldn’t
> >> be able to detect anomalies;
> >
> >Huh? Declaring overly permissive protections is only problematic if an
> >LSM denies the permission, in which case it will generate an accurate
> >audit log.
> >
> >If the enclave/loader "requires" a permission it doesn't actually need,
> >e.g. EXECDIRTY, then it's a software bug that should be fixed. I don't
> >see how this scenario is any different than an application that uses
> >assembly code without 'noexecstack' and inadvertantly "requires"
> >EXECSTACK due to triggering "read implies exec". In both cases the
> >denied permission is unnecessary due to a userspace application bug.
>
> You see, you've been assuming "enclave loader" knows everything and tailored
> to what it loads in a particular application. But the reality is the loader
> is generic and probably shared by multiple applications.
No, I'm assuming that an enclave can communicate its basic needs without
undue pain.
> It needs some generic way to figure out the "maximal protection". An
> implementation could use information embedded in the enclave file, or could
> just be "configurable". In the former case, you put extra burdens on the build
> tools, while in the latter case, your audit logs cannot help generating an
> appropriate configuration.
I'm contending the "extra burdens" is minimal.
if (do_smc || do_relocation)
max_prot = RWX;
else
max_prot = SECINFO.FLAGS;
> >> or if “maximal protection” is too restrictive,
> >> then audit log cannot identify the file violating the policy.
> >
> >Maximal protections that are too restrictive are completely orthogonal to
> >LSMs as the enclave would fail to run irrespective of LSMs. This is no
> >different than specifying the wrong RWX flags in SECINFO, or opening a
> >file as RO instead of RW.
>
> Say loader is configurable. By looking at the log, can an administrator tell
> which file has too restrictive "maximal protection"?
Again, this fails irrespective of LSMs. So the answer is "no", because
there is no log. But the admin will never have to deal with this issue
because the enclave will *never* run, i.e. would unconditionally fail to
run during initial development. And the developer has bigger problems if
they can't debug their own code.
> >>In either case the audit log cannot fulfill its purposes.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 0/4] security/x86/sgx: SGX specific LSM hooks
2019-07-08 18:49 ` Sean Christopherson@ 2019-07-08 22:26 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-08 22:26 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, casey.schaufler,
jmorris, luto, jethro, greg, sds, jarkko.sakkinen
Hi Sean,
What's in my cover letter is my assessment on what's in your series. You
may disagree. But I don't think it productive until you can prove your
points in code.
The key points I'm making are:
(1) The impact to user mode code due to UAPI change is more significant
than you have envisioned.
(2) Your series has implemented less than required in practice.
For #1, regular shared objects don't carry info like whether it contains
self-modifying code or generates code on the fly. So your requirement of
"maximal protection" is new, and you should at least put together a
story to show everyone how it could be met, especially without changing
build tools.
For #2, SGX2 is imminent, and the upcoming ICX server will support 512GB
of EPC. So the problems in mprotect() performance and EAUG-on-#PF must
be solved, let alone other problems. I guess you have to code them up so
everyone will be able to evaluate whether your approach is really as
simple as you have claimed.
-Cedric
On 7/8/2019 11:49 AM, Sean Christopherson wrote:
> On Mon, Jul 08, 2019 at 10:49:59AM -0700, Xing, Cedric wrote:
>> On 7/8/2019 8:55 AM, Sean Christopherson wrote:
>>> On Sun, Jul 07, 2019 at 04:41:30PM -0700, Cedric Xing wrote:
>> True only for SGX1.
>>>> “maximal protection” at page load time, but such information is NOT always
>>>> available. For example, Graphene containers may run different applications
>>>> comprised of different set of executables and/or shared objects. Some of
>>>> them may contain self-modifying code (or text relocation) while others
>>>> don’t. The generic enclave loader usually doesn’t have such information so
>>>> wouldn’t be able to provide it ahead of time.
>>>
>>> I'm unconvinced that it would be remotely difficult to teach an enclave
>>> loader that an enclave or hosted application employs SMC, relocation or
>>> any other behavior that would require declaring RWX on all pages.
>>
>> You've been talking as if "enclave loader" is tailored to the enclave it is
>> loading. But in reality "enclave loader" is usually a library knowing
>> usually nothing about the enclave. How could it know if an enclave contains
>> self-modifying code?
>
> Given the rarity of SMC, require enclaves to declare "I do SMC"... The
> Intel SDK already requires the enclave developer to declare heap size,
> stack size, thread affinity, etc... I have a very hard time believing
> that it can't support SMC and relocation flags.
>
>>>> · Inefficient Auditing – Audit logs are supposed to help system
>>>> administrators to determine the set of minimally needed permissions and to
>>>> detect abnormal behaviors. But consider the “maximal protection” model, if
>>>> “maximal protection” is set to be too permissive, then audit log wouldn’t
>>>> be able to detect anomalies;
>>>
>>> Huh? Declaring overly permissive protections is only problematic if an
>>> LSM denies the permission, in which case it will generate an accurate
>>> audit log.
>>>
>>> If the enclave/loader "requires" a permission it doesn't actually need,
>>> e.g. EXECDIRTY, then it's a software bug that should be fixed. I don't
>>> see how this scenario is any different than an application that uses
>>> assembly code without 'noexecstack' and inadvertantly "requires"
>>> EXECSTACK due to triggering "read implies exec". In both cases the
>>> denied permission is unnecessary due to a userspace application bug.
>>
>> You see, you've been assuming "enclave loader" knows everything and tailored
>> to what it loads in a particular application. But the reality is the loader
>> is generic and probably shared by multiple applications.
>
> No, I'm assuming that an enclave can communicate its basic needs without
> undue pain.
>
>> It needs some generic way to figure out the "maximal protection". An
>> implementation could use information embedded in the enclave file, or could
>> just be "configurable". In the former case, you put extra burdens on the build
>> tools, while in the latter case, your audit logs cannot help generating an
>> appropriate configuration.
>
> I'm contending the "extra burdens" is minimal.
>
> if (do_smc || do_relocation)
> max_prot = RWX;
> else
> max_prot = SECINFO.FLAGS;
>
>>>> or if “maximal protection” is too restrictive,
>>>> then audit log cannot identify the file violating the policy.
>>>
>>> Maximal protections that are too restrictive are completely orthogonal to
>>> LSMs as the enclave would fail to run irrespective of LSMs. This is no
>>> different than specifying the wrong RWX flags in SECINFO, or opening a
>>> file as RO instead of RW.
>>
>> Say loader is configurable. By looking at the log, can an administrator tell
>> which file has too restrictive "maximal protection"?
>
> Again, this fails irrespective of LSMs. So the answer is "no", because
> there is no log. But the admin will never have to deal with this issue
> because the enclave will *never* run, i.e. would unconditionally fail to
> run during initial development. And the developer has bigger problems if
> they can't debug their own code.
>
>>>> In either case the audit log cannot fulfill its purposes.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-08 17:16 ` Xing, Cedric@ 2019-07-08 23:53 ` Casey Schaufler
2019-07-09 22:13 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-08 23:53 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
On 7/8/2019 10:16 AM, Xing, Cedric wrote:
> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>> In this scheme you use an ema LSM to manage your ema data.
>> A quick sketch looks like:
>>
>> sgx_something_in() calls
>> security_enclave_load() calls
>> ema_enclave_load()
>> selinux_enclave_load()
>> otherlsm_enclave_load()
>>
>> Why is this better than:
>>
>> sgx_something_in() calls
>> ema_enclave_load()
>> security_enclave_load() calls
>> selinux_enclave_load()
>> otherlsm_enclave_load()
>
> Are you talking about moving EMA somewhere outside LSM?
Yes. That's what I've been saying all along.
> If so, where?
I tried to make it obvious. Put the call to your EMA code
on the line before you call security_enclave_load().
>
>>
>> If you did really want ema to behave like an LSM
>> you would put the file data that SELinux is managing
>> into the ema portion of the blob and provide interfaces
>> for the SELinux (or whoever) to use that. Also, it's
>> an abomination (as I've stated before) for ema to
>> rely on SELinux to provide a file_free() hook for
>> ema's data. If you continue down the LSM route, you
>> need to provide an ema_file_free() hook. You can't
>> count on SELinux to do it for you. If there are multiple
>> LSMs (coming soon!) that use the ema data, they'll all
>> try to free it, and then Bad Things can happen.
>
> I'm afraid you have misunderstood the code. What is kept open and gets closed in selinux_file_free() is the sigstruct file. SELinux uses it to determine the page permissions for enclave pages from anonymous sources. It is a policy choice made inside SELinux and has nothing to do with EMA.
OK.
>
> There's indeed an ema_file_free_security() to free the EMA map for enclaves being closed. EMA does *NOT* rely on any other LSMs to free data for it. The only exception is when an LSM fails enclave_load(), it has to call ema_remove_range() to remove the range being added, which was *not* required originally in v2.
OK.
>
>>> +/**
>>> + * ema - Enclave Memory Area structure for LSM modules
>>
>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>
> Noted
>
>>> diff --git a/security/Makefile b/security/Makefile
>>> index c598b904938f..b66d03a94853 100644
>>> --- a/security/Makefile
>>> +++ b/security/Makefile
>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>
>> The config option and the file name ought to match,
>> or at least be closer.
>
> Just trying to match file names as "capability" uses commoncap.c.
Fine, then you should be using CONFIG_SECURITY_EMA.
>
> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
Make
CONFIG_SECURITY_EMA
depends on CONFIG_INTEL_SGX
When another TEE (maybe MIPS_SSRPQ) comes along you can have
CONFIG_SECURITY_EMA
depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>
>>> diff --git a/security/commonema.c b/security/commonema.c
>>
>> Put this in a subdirectory. Please.
>
> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
commoncap is not optional. It is a base part of the
security subsystem. ema is optional.
>
>>> +static struct lsm_blob_sizes ema_blob_sizes __lsm_ro_after_init = {
>>> + .lbs_file = sizeof(atomic_long_t),
>>> +};
>>
>> If this is ema's data ema must manage it. You *must* have
>> a file_free().
>
> There is one indeed - ema_file_free_security().
I see it now.
>
>>
>>> +
>>> +static atomic_long_t *_map_file(struct file *encl)
>>> +{
>>> + return (void *)((char *)(encl->f_security) + ema_blob_sizes.lbs_file);
>>
>> I don't trust all the casting going on here, especially since
>> you don't end up with the type you should be returning.
>
> Will change.
>
>>> +}
>>> +
>>> +static struct ema_map *_alloc_map(void)
>>
>> Function header comments, please.
>
> Will add.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-07 13:30 ` Dr. Greg@ 2019-07-09 0:02 ` Casey Schaufler
2019-07-09 1:52 ` Sean Christopherson
2019-07-11 10:22 ` Dr. Greg0 siblings, 2 replies; 156+ messages in thread
From: Casey Schaufler @ 2019-07-09 0:02 UTC (permalink / raw)
To: Dr. Greg, casey
Cc: Xing, Cedric, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen, Christopherson, Sean J
On 7/7/2019 6:30 AM, Dr. Greg wrote:
> On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
>
> Good morning, I hope the weekend has been enjoyable for everyone.
>
>>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
>>>>> ...
>>>>> Guess this discussion will never end if we don't get into
>>>>> code. Guess it'd be more productive to talk over phone then come back
>>>>> to this thread with a conclusion. Will that be ok with you?
>>>> I don't think that a phone call is going to help. Talking code
>>>> issues tends to muddle them in my brain. If you can give me a few
>>>> days I will propose a rough version of how I think your code should
>>>> be integrated into the LSM environment. I'm spending more time
>>>> trying (unsuccessfully :( ) to discribe the issues in English than
>>>> it will probably take in C.
>>> While Casey is off writing his rosetta stone,
>> I'd hardly call it that. More of an effort to round the corners on
>> the square peg. And Cedric has some ideas on how to approach that.
> Should we infer from this comment that, of the two competing
> strategies, Cedric's is the favored architecture?
With Cedric's latest patches I'd say there's only one
strategy. There's still some refinement to do, but we're
getting there.
>>> let me suggest that the
>>> most important thing we need to do is to take a little time, step back
>>> and look at the big picture with respect to what we are trying to
>>> accomplish and if we are going about it in a way that makes any sense
>>> from an engineering perspective.
>>>
>>> This conversation shouldn't be about SGX, it should be about the best
>>> way for the kernel/LSM to discipline a Trusted Execution Environment
>>> (TEE). As I have noted previously, a TEE is a 'blackbox' that, by
>>> design, is intended to allow execution of code and processing of data
>>> in a manner that is resistant to manipulation or inspection by
>>> untrusted userspace, the kernel and/or the hardware itself.
>>>
>>> Given that fact, if we are to be intellectually honest, we need to ask
>>> ourselves how effective we believe we can be in controlling any TEE
>>> with kernel based mechanisms. This is particularly the case if the
>>> author of any code running in the TEE has adversarial intent.
>>>
>>> Here is the list of controls that we believe an LSM can, effectively,
>>> implement against a TEE:
>>>
>>> 1.) Code provenance and origin.
>>>
>>> 2.) Cryptographic verification of dynamically executable content.
>>>
>>> 2.) The ability of a TEE to implement anonymous executable content.
>>>
>>> If people are in agreement with this concept, it is difficult to
>>> understand why we should be implementing complex state machines and
>>> the like, whether it is in the driver or the LSM. Security code has
>>> to be measured with a metric of effectiveness, otherwise we are
>>> engaging in security theater.
>>>
>>> I believe that if we were using this lens, we would already have a
>>> mainline SGX driver, since we seem to have most of the needed LSM
>>> infrastructure and any additional functionality would be a straight
>>> forward implementation. Most importantly, the infrastructure would
>>> not be SGX specific, which would seem to be a desirable political
>>> concept.
>> Generality introduced in the absence of multiple instances
>> often results in unnecessary complexity, unused interfaces
>> and feature compromise. Guessing what other TEE systems might
>> do, and constraining SGX to those models (or the other way around)
>> is a well established road to ruin. The LSM infrastructure is
>> a fine example. For the first ten years the "general" mechanism
>> had a single user. I'd say to hold off on the general until there
>> is more experience with the specific. It's easier to construct
>> a general mechanism around things that work than to fit things
>> that need to work into some preconceived notion of generality.
> All well taken points from an implementation perspective, but they
> elide the point I was trying to make. Which is the fact that without
> any semblance of a discussion regarding the requirements needed to
> implement a security architecture around the concept of a TEE, this
> entire process, despite Cedric's well intentioned efforts, amounts to
> pounding a square solution into the round hole of a security problem.
Lead with code. I love a good requirements document, but
one of the few places where I agree with the agile folks is
that working code speaks loudly.
> Which, as I noted in my e-mail, is tantamount to security theater.
Not buying that. Not rejecting it, either. Without code
to judge it's kind of hard to say.
> Everyone wants to see this driver upstream. If we would have had a
> reasoned discussion regarding what it means to implement proper
> controls around a TEE, when we started to bring these issues forward
> last November, we could have possibly been on the road to having a
> driver with reasoned security controls and one that actually delivers
> the security guarantees the hardware was designed to deliver.
>
> Best wishes for a productive week to everyone.
>
> Dr. Greg
>
> As always,
> Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
> 4206 N. 19th Ave. Specializing in information infra-structure
> Fargo, ND 58102 development.
> PH: 701-281-1686 EMAIL: greg@enjellic.com
> ------------------------------------------------------------------------------
> "Any intelligent fool can make things bigger and more complex... It
> takes a touch of genius - and a lot of courage to move in the opposite
> direction."
> -- Albert Einstein
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-09 0:02 ` Casey Schaufler@ 2019-07-09 1:52 ` Sean Christopherson
2019-07-09 21:16 ` Xing, Cedric
2019-07-11 10:22 ` Dr. Greg1 sibling, 1 reply; 156+ messages in thread
From: Sean Christopherson @ 2019-07-09 1:52 UTC (permalink / raw)
To: Casey Schaufler
Cc: Dr. Greg, Xing, Cedric, Stephen Smalley, linux-sgx,
linux-security-module, selinux, Schaufler, Casey, jmorris, luto,
jethro, sds, jarkko.sakkinen
On Mon, Jul 08, 2019 at 05:02:00PM -0700, Casey Schaufler wrote:
> On 7/7/2019 6:30 AM, Dr. Greg wrote:
> > On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
> >
> > Good morning, I hope the weekend has been enjoyable for everyone.
> >
> >>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
> >>>>> ...
> >>>>> Guess this discussion will never end if we don't get into
> >>>>> code. Guess it'd be more productive to talk over phone then come back
> >>>>> to this thread with a conclusion. Will that be ok with you?
> >>>> I don't think that a phone call is going to help. Talking code
> >>>> issues tends to muddle them in my brain. If you can give me a few
> >>>> days I will propose a rough version of how I think your code should
> >>>> be integrated into the LSM environment. I'm spending more time
> >>>> trying (unsuccessfully :( ) to discribe the issues in English than
> >>>> it will probably take in C.
> >>> While Casey is off writing his rosetta stone,
> >> I'd hardly call it that. More of an effort to round the corners on
> >> the square peg. And Cedric has some ideas on how to approach that.
> > Should we infer from this comment that, of the two competing
> > strategies, Cedric's is the favored architecture?
>
> With Cedric's latest patches I'd say there's only one
> strategy. There's still some refinement to do, but we're
> getting there.
Dynamic tracking has an unsolvable race condition. If process A maps a
page W and process B maps the same page X, then the result of W^X checks
depends on the order of mprotect() calls between A and B.
If we're ok saying "don't do that" then I can get behind dynamic tracking
as a whole. Even if we settle on dynamic tracking, where that tracking
code lives is still an open IMO.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-09 1:52 ` Sean Christopherson@ 2019-07-09 21:16 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-09 21:16 UTC (permalink / raw)
To: Sean Christopherson, Casey Schaufler
Cc: Dr. Greg, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen
On 7/8/2019 6:52 PM, Sean Christopherson wrote:
> On Mon, Jul 08, 2019 at 05:02:00PM -0700, Casey Schaufler wrote:
>> On 7/7/2019 6:30 AM, Dr. Greg wrote:
>>> On Wed, Jul 03, 2019 at 08:32:10AM -0700, Casey Schaufler wrote:
>>>
>>> Good morning, I hope the weekend has been enjoyable for everyone.
>>>
>>>>>> On 7/2/2019 12:42 AM, Xing, Cedric wrote:
>>>>>>> ...
>>>>>>> Guess this discussion will never end if we don't get into
>>>>>>> code. Guess it'd be more productive to talk over phone then come back
>>>>>>> to this thread with a conclusion. Will that be ok with you?
>>>>>> I don't think that a phone call is going to help. Talking code
>>>>>> issues tends to muddle them in my brain. If you can give me a few
>>>>>> days I will propose a rough version of how I think your code should
>>>>>> be integrated into the LSM environment. I'm spending more time
>>>>>> trying (unsuccessfully :( ) to discribe the issues in English than
>>>>>> it will probably take in C.
>>>>> While Casey is off writing his rosetta stone,
>>>> I'd hardly call it that. More of an effort to round the corners on
>>>> the square peg. And Cedric has some ideas on how to approach that.
>>> Should we infer from this comment that, of the two competing
>>> strategies, Cedric's is the favored architecture?
>>
>> With Cedric's latest patches I'd say there's only one
>> strategy. There's still some refinement to do, but we're
>> getting there.
>
> Dynamic tracking has an unsolvable race condition. If process A maps a
> page W and process B maps the same page X, then the result of W^X checks
> depends on the order of mprotect() calls between A and B.
I don't quite understand where the term "dynamic tracking" came from.
What's done in the patch is just to track which page was contributed by
which file. It's been done for all file mappings in Linux.
Back to the "race condition". A similar situation already exists in
SELinux, between EXECMOD and EXECMEM. Say a file does *not* have EXECMOD
but the calling process has EXECMEM. Then WX could be granted to a fresh
private mapping (because of EXECMEM). However, once the mapping has been
written to, X should have been revoked (because of lack of EXECMOD) but
will still be retained until dropped by an explicit mprotect().
Afterwards, mprotect(X) will be denied. That's not the same situation as
in this enclave case but they do share one thing in common - X should
have been revoked from an existing mapping but it isn't, which is just a
policy choice.
Nothing is unsolvable. Here are 2 options.
(1) We argue that it doesn't matter, similar to the EXECMOD/EXECMEM case
on regular file mappings described above; or
(2) EXECMOD is required for both W->X and X->W transitions, hence W
requested by the 2nd process will be denied because X has been granted
to the 1st process while EXECMOD is absent.
Please note that #2 is effectively the same concept as
PROCESS2__ENCLAVE_EXECDIRTY in Sean's patch, except that EXECMOD is per
file while ENCLAVE_EXECDIRTY is per process.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-08 23:53 ` Casey Schaufler@ 2019-07-09 22:13 ` Xing, Cedric
2019-07-10 0:10 ` Casey Schaufler0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-09 22:13 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
On 7/8/2019 4:53 PM, Casey Schaufler wrote:
> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>> In this scheme you use an ema LSM to manage your ema data.
>>> A quick sketch looks like:
>>>
>>> sgx_something_in() calls
>>> security_enclave_load() calls
>>> ema_enclave_load()
>>> selinux_enclave_load()
>>> otherlsm_enclave_load()
>>>
>>> Why is this better than:
>>>
>>> sgx_something_in() calls
>>> ema_enclave_load()
>>> security_enclave_load() calls
>>> selinux_enclave_load()
>>> otherlsm_enclave_load()
>>
>> Are you talking about moving EMA somewhere outside LSM?
>
> Yes. That's what I've been saying all along.
>
>> If so, where?
>
> I tried to make it obvious. Put the call to your EMA code
> on the line before you call security_enclave_load().
Sorry but I'm still confused.
EMA code is used by LSMs only. Making it callable from other parts of
the kernel IMHO is probably not a good idea. And more importantly I
don't understand the motivation behind it. Would you please elaborate?
>>>> +/**
>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>
>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>
>> Noted
>>
>>>> diff --git a/security/Makefile b/security/Makefile
>>>> index c598b904938f..b66d03a94853 100644
>>>> --- a/security/Makefile
>>>> +++ b/security/Makefile
>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>
>>> The config option and the file name ought to match,
>>> or at least be closer.
>>
>> Just trying to match file names as "capability" uses commoncap.c.
>
> Fine, then you should be using CONFIG_SECURITY_EMA.
>
>>
>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>
> Make
> CONFIG_SECURITY_EMA
> depends on CONFIG_INTEL_SGX
>
> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>
> CONFIG_SECURITY_EMA
> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
Your suggestions are reasonable. Given such config change wouldn't
affect any code, can we do it later, e.g., when additional TEEs come
online and make use of these new hooks? After all,
security_enclave_init() will need amendment anyway as one of its current
parameters is of type 'struct sgx_sigstruct', which will need to be
replaced with something more generic. At the time being, I'd like to
keep things intuitive so as not to confuse reviewers.
>>
>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>
>>> Put this in a subdirectory. Please.
>>
>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>
> commoncap is not optional. It is a base part of the
> security subsystem. ema is optional.
Alright. I'd move it into a sub-folder and rename it to ema.c. Would you
be ok with that?
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-09 22:13 ` Xing, Cedric@ 2019-07-10 0:10 ` Casey Schaufler
2019-07-10 0:55 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Casey Schaufler @ 2019-07-10 0:10 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
On 7/9/2019 3:13 PM, Xing, Cedric wrote:
> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>> In this scheme you use an ema LSM to manage your ema data.
>>>> A quick sketch looks like:
>>>>
>>>> sgx_something_in() calls
>>>> security_enclave_load() calls
>>>> ema_enclave_load()
>>>> selinux_enclave_load()
>>>> otherlsm_enclave_load()
>>>>
>>>> Why is this better than:
>>>>
>>>> sgx_something_in() calls
>>>> ema_enclave_load()
>>>> security_enclave_load() calls
>>>> selinux_enclave_load()
>>>> otherlsm_enclave_load()
>>>
>>> Are you talking about moving EMA somewhere outside LSM?
>>
>> Yes. That's what I've been saying all along.
>>
>>> If so, where?
>>
>> I tried to make it obvious. Put the call to your EMA code
>> on the line before you call security_enclave_load().
>
> Sorry but I'm still confused.
>
> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
LSM modules implement additional access control restrictions.
The EMA code does not do that, it provides management of data
that is used by security modules. It is not one itself. VFS
also performs this role, but no one would consider making VFS
a security module.
>>>>> +/**
>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>
>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>
>>> Noted
>>>
>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>> index c598b904938f..b66d03a94853 100644
>>>>> --- a/security/Makefile
>>>>> +++ b/security/Makefile
>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>
>>>> The config option and the file name ought to match,
>>>> or at least be closer.
>>>
>>> Just trying to match file names as "capability" uses commoncap.c.
>>
>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>
>>>
>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>
>> Make
>> CONFIG_SECURITY_EMA
>> depends on CONFIG_INTEL_SGX
>>
>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>
>> CONFIG_SECURITY_EMA
>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>
> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
That doesn't make the current options any less confusing,
and it will be easier to make the change now than at some
point in the future.
> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
Reviewers (including me) are already confused by the inconsistency.
>
>>>
>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>
>>>> Put this in a subdirectory. Please.
>>>
>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>
>> commoncap is not optional. It is a base part of the
>> security subsystem. ema is optional.
>
> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
Sounds fine.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-10 0:10 ` Casey Schaufler@ 2019-07-10 0:55 ` Xing, Cedric
2019-07-10 21:14 ` Casey Schaufler
2019-07-11 13:51 ` Stephen Smalley0 siblings, 2 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-10 0:55 UTC (permalink / raw)
To: Casey Schaufler, linux-sgx, linux-security-module, selinux
On 7/9/2019 5:10 PM, Casey Schaufler wrote:
> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>> A quick sketch looks like:
>>>>>
>>>>> sgx_something_in() calls
>>>>> security_enclave_load() calls
>>>>> ema_enclave_load()
>>>>> selinux_enclave_load()
>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Why is this better than:
>>>>>
>>>>> sgx_something_in() calls
>>>>> ema_enclave_load()
>>>>> security_enclave_load() calls
>>>>> selinux_enclave_load()
>>>>> otherlsm_enclave_load()
>>>>
>>>> Are you talking about moving EMA somewhere outside LSM?
>>>
>>> Yes. That's what I've been saying all along.
>>>
>>>> If so, where?
>>>
>>> I tried to make it obvious. Put the call to your EMA code
>>> on the line before you call security_enclave_load().
>>
>> Sorry but I'm still confused.
>>
>> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
>
> LSM modules implement additional access control restrictions.
> The EMA code does not do that, it provides management of data
> that is used by security modules. It is not one itself. VFS
> also performs this role, but no one would consider making VFS
> a security module.
You are right. EMA is more like a helper library than a real LSM. But
the practical problem is, it has a piece of initialization code, to
basically request some space in the file blob from the LSM
infrastructure. That cannot be done by any LSMs at runtime. So it has to
either be done in LSM infrastructure directly, or make itself an LSM to
make its initialization function invoked by LSM infrastructure
automatically. You have objected to the former, so I switched to the
latter. Are you now objecting to the latter as well? Then what are you
suggesting, really?
VFS is a completely different story. It's the file system abstraction so
it has a natural place to live in the kernel, and its initialization
doesn't depend on the LSM infrastructure. EMA on the other hand, shall
belong to LSM because it is both produced and consumed within LSM.
And, Stephen, do you have an opinion on this?
>>>>>> +/**
>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>
>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>
>>>> Noted
>>>>
>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>> --- a/security/Makefile
>>>>>> +++ b/security/Makefile
>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>
>>>>> The config option and the file name ought to match,
>>>>> or at least be closer.
>>>>
>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>
>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>
>>>>
>>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>>
>>> Make
>>> CONFIG_SECURITY_EMA
>>> depends on CONFIG_INTEL_SGX
>>>
>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>
>>> CONFIG_SECURITY_EMA
>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>
>> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
>
> That doesn't make the current options any less confusing,
> and it will be easier to make the change now than at some
> point in the future.
>
>> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
>
> Reviewers (including me) are already confused by the inconsistency.
OK. Let me make this change.
>>>>
>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>
>>>>> Put this in a subdirectory. Please.
>>>>
>>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>>
>>> commoncap is not optional. It is a base part of the
>>> security subsystem. ema is optional.
>>
>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
>
> Sounds fine.
This is another part that confuses me. Per you comment here, I think you
are OK with EMA being part of LSM (I mean, living somewhere under
security/). But your other comment of calling ema_enclave_load()
alongside security_enclave_load() made me think EMA and LSM were
separate. What do you want really?
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-08 17:29 ` Sean Christopherson
2019-07-08 17:33 ` Xing, Cedric
2019-07-09 16:22 ` Jarkko Sakkinen@ 2019-07-10 1:28 ` Dr. Greg
2019-07-10 2:04 ` Xing, Cedric
2019-07-10 3:21 ` Jethro Beekman3 siblings, 1 reply; 156+ messages in thread
From: Dr. Greg @ 2019-07-10 1:28 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Cedric Xing, Andy Lutomirski, Jethro Beekman, Stephen Smalley
On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
Good evening to everyone.
> That being said, we can do so without functional changes to the SGX
> uapi, e.g. add reserved fields so that the initial uapi can be
> extended *if* we decide to go with the "userspace provides maximal
> protections" path, and use the EPCM permissions as the maximal
> protections for the initial upstreaming.
>
> That'd give us a minimal implemenation for initial upstreaming and
> would eliminate Cedric's blocking complaint. The "whole mess" of
> whitelisting, blacklisting and SGX2 support would be deferred until
> post-upstreaming.
Are we convinced the 'mess' will be any easier to clean up after the
driver is upstreamed?
The primary problem is that we haven't addressed the issue of what
this technology is designed to do and its implications with respect to
the kernel. As a result we are attempting to implement controls which
we are comfortable with and understand rather then those that are
relevant.
Have a good evening.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC Implementing SGX secured and modeled
4206 N. 19th Ave. intelligent network endpoints.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Courage is not the absence of fear, but rather the judgement that
something else is more important than fear."
-- Ambrose Redmoon
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-10 1:28 ` Dr. Greg@ 2019-07-10 2:04 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-10 2:04 UTC (permalink / raw)
To: Dr. Greg, Sean Christopherson
Cc: Jarkko Sakkinen, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Stephen Smalley
On 7/9/2019 6:28 PM, Dr. Greg wrote:
> On Mon, Jul 08, 2019 at 10:29:30AM -0700, Sean Christopherson wrote:
>
> Good evening to everyone.
>
>> That being said, we can do so without functional changes to the SGX
>> uapi, e.g. add reserved fields so that the initial uapi can be
>> extended *if* we decide to go with the "userspace provides maximal
>> protections" path, and use the EPCM permissions as the maximal
>> protections for the initial upstreaming.
>>
>> That'd give us a minimal implemenation for initial upstreaming and
>> would eliminate Cedric's blocking complaint. The "whole mess" of
>> whitelisting, blacklisting and SGX2 support would be deferred until
>> post-upstreaming.
>
> Are we convinced the 'mess' will be any easier to clean up after the
> driver is upstreamed?
>
> The primary problem is that we haven't addressed the issue of what
> this technology is designed to do and its implications with respect to
> the kernel. As a result we are attempting to implement controls which
> we are comfortable with and understand rather then those that are
> relevant.
I don't think it's about easier or harder to clean up the mess, but a
divide-and-conquer strategy. After all, SGX and LSM are kind of
orthogonal as long as SGX doesn't compromise the protection provided by LSM.
Let's step back and look at what started this lengthy discussion. The
primary problem of v20 was that the SGX module allows executable enclave
pages to be created from non-executable regular pages, which could be
exploited by adversaries to grant executable permissions to pages that
would otherwise be denied without SGX. And that could be fixed simply by
capping EPCM permissions to whatever allowed on the source page, without
touching LSM. Of course the drawback is loss of functionality - i.e.
self-modifying enclaves cannot be loaded unless the calling process has
EXECMEM. But that should suffice, as most SGX1 applications don't
contain self-modifying code anyway.
Then we could switch our focus back to LSM and work out what's relevant,
especially for SGX2 and beyond.
> Have a good evening.
>
> Dr. Greg
>
> As always,
> Dr. Greg Wettstein, Ph.D, Worker
> IDfusion, LLC Implementing SGX secured and modeled
> 4206 N. 19th Ave. intelligent network endpoints.
> Fargo, ND 58102
> PH: 701-281-1686 EMAIL: greg@idfusion.net
> ------------------------------------------------------------------------------
> "Courage is not the absence of fear, but rather the judgement that
> something else is more important than fear."
> -- Ambrose Redmoon
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-08 17:29 ` Sean Christopherson
` (2 preceding siblings ...)
2019-07-10 1:28 ` Dr. Greg@ 2019-07-10 3:21 ` Jethro Beekman3 siblings, 0 replies; 156+ messages in thread
From: Jethro Beekman @ 2019-07-10 3:21 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Dr . Greg Wettstein, Stephen Smalley
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]
On 2019-07-08 10:29, Sean Christopherson wrote:
> On Fri, Jul 05, 2019 at 07:05:49PM +0300, Jarkko Sakkinen wrote:
>> On Wed, Jun 19, 2019 at 03:23:49PM -0700, Sean Christopherson wrote:
>>
>> I still don't get why we need this whole mess and do not simply admit
>> that there are two distinct roles:
>>
>> 1. Creator
>> 2. User
>
> Because SELinux has existing concepts of EXECMEM and EXECMOD.
>
>> In the SELinux context Creator needs FILE__WRITE and FILE__EXECUTE but
>> User does not. It just gets the fd from the Creator. I'm sure that all
>> the SGX2 related functionality can be solved somehow in this role
>> playing game.
>>
>> An example would be the usual case where enclave is actually a loader
>> that loads the actual piece of software that one wants to run. Things
>> simply need to be designed in a way the Creator runs the loader part.
>> These are non-trivial problems but oddball security model is not going
>> to make them disappear - on the contrary it will make designing user
>> space only more complicated.
>>
>> I think this is classical example of when something overly complicated
>> is invented in the kernel only to realize that it should be solved in
>> the user space.
>>
>> It would not be like the only use case where some kind of privileged
>> daemon is used for managing some a kernel provided resource.
>>
>> I think a really good conclusion from this discussion that has taken two
>> months is to realize that nothing needs to be done in this area (except
>> *maybe* noexec check).
>
> Hmm, IMO we need to support at least equivalents to EXECMEM and EXECMOD.
>
> That being said, we can do so without functional changes to the SGX uapi,
> e.g. add reserved fields so that the initial uapi can be extended *if* we
> decide to go with the "userspace provides maximal protections" path, and
> use the EPCM permissions as the maximal protections for the initial
> upstreaming.
Why do you need to add reserved fields now? Isn't this what
incorporating the struct size in the ioctl number is for?
--
Jethro Beekman | Fortanix
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3990 bytes --]^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
2019-07-10 15:49 ` Sean Christopherson
2019-07-10 16:08 ` Jethro Beekman@ 2019-07-10 17:54 ` Xing, Cedric1 sibling, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-10 17:54 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-sgx, linux-security-module, selinux
On 7/10/2019 8:49 AM, Sean Christopherson wrote:
> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>> selinux_enclave_init() determines if an enclave is allowed to launch, using the
>> criteria described earlier. This implementation does NOT accept SIGSTRUCT in
>> anonymous memory. The backing file is also cached in struct
>> file_security_struct and will serve as the base for decisions for anonymous
>> pages.
>
> Did we ever reach a consensus on whether sigstruct must reside in a file?
No. We reached the opposite agreement of *not* requiring sigstruct to
reside in a file at the interface level - i.e., security_enclave_init()
takes a VMA but *not* a file struct as input.
At the implementation level, an LSM may require sigstruct to reside in a
file. But that's a per-LSM decision.
>> + /* Store SIGSTRUCT file for future use */
>> + if (atomic_long_cmpxchg(&fsec->encl_ss, 0, (long)src->vm_file))
>> + return -EEXIST;
>> +
>> + get_file(src->vm_file);
>
> My understanding is that Andy is strongly against pinning a file for the
> duration of the enclave, has that changed?
I think everyone including Andy prefers not to pin any files. But it's a
trade-off among code simplicity, auditing accuracy and memory
consumption. I think the latest suggestion from Stephen was to keep
files open, for SELinux. Again, that's a per-LSM decision.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 4/4] x86/sgx: Implement SGX specific hooks in SELinux
2019-07-10 16:08 ` Jethro Beekman@ 2019-07-10 18:16 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-10 18:16 UTC (permalink / raw)
To: Jethro Beekman, Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux
On 7/10/2019 9:08 AM, Jethro Beekman wrote:
> On 2019-07-10 08:49, Sean Christopherson wrote:
>> On Sun, Jul 07, 2019 at 04:41:34PM -0700, Cedric Xing wrote:
>>> selinux_enclave_init() determines if an enclave is allowed to launch,
>>> using the
>>> criteria described earlier. This implementation does NOT accept
>>> SIGSTRUCT in
>>> anonymous memory. The backing file is also cached in struct
>>> file_security_struct and will serve as the base for decisions for
>>> anonymous
>>> pages.
>>
>> Did we ever reach a consensus on whether sigstruct must reside in a file?
>
> This would be inconvenient for me, but I guess I can create a memfd?
No, sigstruct doesn't have to reside in a file.
But the current direction is, in SELinux, what the enclave can do
depends on permissions given to the file containing sigstruct. That
said, if SELinux is in effect, sigstruct has to reside in a real file
with FILE__EXECUTE permission for the enclave to launch. memfd wouldn't
work. To some extent, that serves the purpose of whitelisting.
> --
> Jethro Beekman | Fortanix
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-10 0:55 ` Xing, Cedric@ 2019-07-10 21:14 ` Casey Schaufler
2019-07-11 13:51 ` Stephen Smalley1 sibling, 0 replies; 156+ messages in thread
From: Casey Schaufler @ 2019-07-10 21:14 UTC (permalink / raw)
To: Xing, Cedric, linux-sgx, linux-security-module, selinux, casey
On 7/9/2019 5:55 PM, Xing, Cedric wrote:
> On 7/9/2019 5:10 PM, Casey Schaufler wrote:
>> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>>> A quick sketch looks like:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> security_enclave_load() calls
>>>>>> ema_enclave_load()
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>>
>>>>>> Why is this better than:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> ema_enclave_load()
>>>>>> security_enclave_load() calls
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Are you talking about moving EMA somewhere outside LSM?
>>>>
>>>> Yes. That's what I've been saying all along.
>>>>
>>>>> If so, where?
>>>>
>>>> I tried to make it obvious. Put the call to your EMA code
>>>> on the line before you call security_enclave_load().
>>>
>>> Sorry but I'm still confused.
>>>
>>> EMA code is used by LSMs only. Making it callable from other parts of the kernel IMHO is probably not a good idea. And more importantly I don't understand the motivation behind it. Would you please elaborate?
>>
>> LSM modules implement additional access control restrictions.
>> The EMA code does not do that, it provides management of data
>> that is used by security modules. It is not one itself. VFS
>> also performs this role, but no one would consider making VFS
>> a security module.
>
> You are right.
So far, so good ...
> EMA is more like a helper library than a real LSM.
Then you should use it as such.
> But the practical problem is, it has a piece of initialization code, to basically request some space in the file blob from the LSM infrastructure.
The security modules that want to use EMA should
request that space.
> That cannot be done by any LSMs at runtime.
Sure it can. And it has to. What if you don't
have any security modules that use the EMA data?
Surely you don't want to be allocating blob space
for EMA data if no one is going to use it.
> So it has to either be done in LSM infrastructure directly, or make itself an LSM to make its initialization function invoked by LSM infrastructure automatically.
That is not true. The security module that wants to use
the EMA data can call whatever allocation function you use.
Or, the call can be made from the code just before you call
the security hook, which would be identical to calling it
as a "first" hook.
> You have objected to the former, so I switched to the latter. Are you now objecting to the latter as well? Then what are you suggesting, really?
Call your allocation function just before you call
security_enclave_load(). There is no way that selinux_enclave_load()
could tell the difference.
> VFS is a completely different story. It's the file system abstraction so it has a natural place to live in the kernel, and its initialization doesn't depend on the LSM infrastructure. EMA on the other hand, shall belong to LSM because it is both produced and consumed within LSM.
And this is the enclave abstraction, or rather, should be
according to at least half the people joining in on the
thread. It does not belong in the LSM infrastructure because
it is it's own thing, with its own state and data, which it
needs to maintain in its own way and place. It needs interfaces
so that security modules can use that information appropriately.
It needs a hook or two so that the enclave abstraction can ask
the security modules to make decisions.
>
> And, Stephen, do you have an opinion on this?
>
>>>>>>> +/**
>>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>>
>>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>>
>>>>> Noted
>>>>>
>>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>>> --- a/security/Makefile
>>>>>>> +++ b/security/Makefile
>>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>>
>>>>>> The config option and the file name ought to match,
>>>>>> or at least be closer.
>>>>>
>>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>>
>>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>>
>>>>>
>>>>> Like I said, this feature could potentially be used by TEEs other than SGX. For now, SGX is the only user so it is tied to CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you have a preference?
>>>>
>>>> Make
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX
>>>>
>>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>>
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>>
>>> Your suggestions are reasonable. Given such config change wouldn't affect any code, can we do it later,
>>
>> That doesn't make the current options any less confusing,
>> and it will be easier to make the change now than at some
>> point in the future.
>>
>>> e.g., when additional TEEs come online and make use of these new hooks? After all, security_enclave_init() will need amendment anyway as one of its current parameters is of type 'struct sgx_sigstruct', which will need to be replaced with something more generic. At the time being, I'd like to keep things intuitive so as not to confuse reviewers.
>>
>> Reviewers (including me) are already confused by the inconsistency.
>
> OK. Let me make this change.
Thank you.
>>>>>
>>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>>
>>>>>> Put this in a subdirectory. Please.
>>>>>
>>>>> Then why is commoncap.c located in this directory? I'm just trying to match the existing convention.
>>>>
>>>> commoncap is not optional. It is a base part of the
>>>> security subsystem. ema is optional.
>>>
>>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would you be ok with that?
>>
>> Sounds fine.
>
> This is another part that confuses me. Per you comment here, I think you are OK with EMA being part of LSM
Ah. Being in the security directory does not mean it's
a part of the LSM system. Keys and integrity are security
subsystems that are related to, but not part of, the LSM
sub-system.
> (I mean, living somewhere under security/). But your other comment of calling ema_enclave_load() alongside security_enclave_load() made me think EMA and LSM were separate. What do you want really?
Please stop asking the same question over and over.
You're not going to get a different answer from what
you've gotten already. Look back at it what's already
been said.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-09 17:09 ` Sean Christopherson
2019-07-09 20:41 ` Xing, Cedric
2019-07-10 20:19 ` Jarkko Sakkinen@ 2019-07-10 22:16 ` Jarkko Sakkinen
2019-07-10 23:16 ` Xing, Cedric2 siblings, 1 reply; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-07-10 22:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Cedric Xing,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
Just some questions on these.
On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
> on an enclave page loaded from a regular file
One thing that I have hard time to perceive is that whether the process
or the target object has them. So would this be in the files extended
attribute or does process need to possess this or both?
> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
> required to gain W->X on an enclave page
Still puzzling with EXECMOD given that how it is documented in
https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
document is out of date, would be nice if it was updated.
> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
> an enclave page that is loaded from an
> anonymous mapping
>
> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
> enclave page
I guess these three belong to the process and are not attached to file.
How in SELinux anyway process in the first place acquires any SELinux
permissions? I guess getty or whatever login process can set its perms
before setuid() et al somehow (I don't know how) because they run as
root?
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-10 22:16 ` Jarkko Sakkinen@ 2019-07-10 23:16 ` Xing, Cedric
2019-07-11 9:26 ` Jarkko Sakkinen0 siblings, 1 reply; 156+ messages in thread
From: Xing, Cedric @ 2019-07-10 23:16 UTC (permalink / raw)
To: Jarkko Sakkinen, Sean Christopherson
Cc: linux-sgx, linux-security-module, selinux, Bill Roberts,
Casey Schaufler, James Morris, Dave Hansen, Andy Lutomirski,
Jethro Beekman, Dr . Greg Wettstein, Stephen Smalley
On 7/10/2019 3:16 PM, Jarkko Sakkinen wrote:
> Just some questions on these.
>
> On Tue, Jul 09, 2019 at 10:09:17AM -0700, Sean Christopherson wrote:
>> - FILE__ENCLAVE_EXECUTE: equivalent to FILE__EXECUTE, required to gain X
>> on an enclave page loaded from a regular file
>
> One thing that I have hard time to perceive is that whether the process
> or the target object has them. So would this be in the files extended
> attribute or does process need to possess this or both?
The target object.
>> - PROCESS2__ENCLAVE_EXECDIRTY: hybrid of EXECMOD and EXECUTE+WRITE,
>> required to gain W->X on an enclave page
>
> Still puzzling with EXECMOD given that how it is documented in
> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> document is out of date, would be nice if it was updated.
If you search for "EXECMOD" in security/selinux/hooks.c in the latest
(Linux-5.2) master, you'll find only one occurrence - at line 3702.
The logic over there, if translated into English, basically says
FILE__EXECMOD is required (on the backing file) if mprotect() is called
to request X on a private file mapping that has been modified by the
calling process. That's what Sean meant by "W->X".
EXCLAVE_EXECDIRTY is similar to EXECMOD but because of his "maximal
protection" model, LSM couldn't distinguish between "W->X" and "X->W",
hence those two are collapsed into a single case - WX in "maximal
protection".
>> - PROCESS2__ENCLAVE_EXECANON: subset of EXECMEM, required to gain X on
>> an enclave page that is loaded from an
>> anonymous mapping
>>
>> - PROCESS2__ENCLAVE_MAPWX: subset of EXECMEM, required to gain WX on an
>> enclave page
>
> I guess these three belong to the process and are not attached to file.
Correct.
ENCLAVE_EXECANON basically means the calling process doesn't care what
permissions given to enclave pages as the SIGSTRUCT alone is considered
sufficient validation. This has a security impact process wide so shall
be a process permission.
ENCLAVE_{EXECDIRTY|MAPWX} express enclave specific
requirements/behaviors and IMO shall be enclave permissions, probably
manifested as file permissions on the file containing SIGSTRUCT. Sean
was taking a shortcut to make them process scope in order to avoid
keeping the SIGSTRUCT file around, which was what I criticized as
"illogical".
> How in SELinux anyway process in the first place acquires any SELinux
> permissions? I guess getty or whatever login process can set its perms
> before setuid() et al somehow (I don't know how) because they run as
> root?
>
> /Jarkko
>
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-10 23:16 ` Xing, Cedric@ 2019-07-11 9:26 ` Jarkko Sakkinen
2019-07-11 14:32 ` Stephen Smalley0 siblings, 1 reply; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-07-11 9:26 UTC (permalink / raw)
To: Xing, Cedric
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein,
Stephen Smalley
On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
> > Still puzzling with EXECMOD given that how it is documented in
> > https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
> > document is out of date, would be nice if it was updated.
>
> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
>
> The logic over there, if translated into English, basically says
> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
> request X on a private file mapping that has been modified by the calling
> process. That's what Sean meant by "W->X".
Looking at that part of code, there is this comment:
/*
* We are making executable a file mapping that has
* had some COW done. Since pages might have been
* written, check ability to execute the possibly
* modified content. This typically should only
* occur for text relocations.
*/
There is no COW done with enclaves, never. Thus, EXECMOD does not
connect in any possible way to SGX. OR, that comment is false.
Which one is it?
Also the official documentation for SELinux speaks only about COW
mappings.
Also the condition supports all this as a *private* file mapping ends up
to the anon_vma list when it gets written. We have a *shared* file
mapping
Nothing that you say makes sense to me, sorry...
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v2 1/3] x86/sgx: Add SGX specific LSM hooks
2019-07-09 0:02 ` Casey Schaufler
2019-07-09 1:52 ` Sean Christopherson@ 2019-07-11 10:22 ` Dr. Greg
2019-07-15 22:23 ` Andy Lutomirski1 sibling, 1 reply; 156+ messages in thread
From: Dr. Greg @ 2019-07-11 10:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: Xing, Cedric, Stephen Smalley, linux-sgx, linux-security-module,
selinux, Schaufler, Casey, jmorris, luto, jethro, sds,
jarkko.sakkinen, Christopherson, Sean J
On Mon, Jul 08, 2019 at 05:02:00PM -0700, Casey Schaufler wrote:
> > On 7/7/2019 6:30 AM, Dr. Greg wrote:
> > All well taken points from an implementation perspective, but they
> > elide the point I was trying to make. Which is the fact that without
> > any semblance of a discussion regarding the requirements needed to
> > implement a security architecture around the concept of a TEE, this
> > entire process, despite Cedric's well intentioned efforts, amounts to
> > pounding a square solution into the round hole of a security problem.
> Lead with code. I love a good requirements document, but one of the
> few places where I agree with the agile folks is that working code
> speaks loudly.
>
> > Which, as I noted in my e-mail, is tantamount to security theater.
>
> Not buying that. Not rejecting it, either. Without code
> to judge it's kind of hard to say.
We tried the code approach.
By most accounts it seemed to go poorly, given that it ended up with
Jonathan Corbet writing an LWN feature article on the state of
dysfunction and chaos surrounding Linux SGX driver development.
So we are standing around and mumbling until we can figure out what
kind of code we need to write to make the new driver relevant to the
CISO's and security architects we need to defend SGX to.
Have a good week.
Dr. Greg
As always,
Dr. Greg Wettstein, Ph.D, Worker
IDfusion, LLC Implementing SGX secured and modeled
4206 N. 19th Ave. intelligent network endpoints.
Fargo, ND 58102
PH: 701-281-1686 EMAIL: greg@idfusion.net
------------------------------------------------------------------------------
"Five year projections, are you kidding me. We don't know what we are
supposed to be doing at the 4 o'clock meeting this afternoon."
-- Terry Wieland
Resurrection
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-10 0:55 ` Xing, Cedric
2019-07-10 21:14 ` Casey Schaufler@ 2019-07-11 13:51 ` Stephen Smalley
2019-07-11 15:12 ` Sean Christopherson1 sibling, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-07-11 13:51 UTC (permalink / raw)
To: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris
On 7/9/19 8:55 PM, Xing, Cedric wrote:
> On 7/9/2019 5:10 PM, Casey Schaufler wrote:
>> On 7/9/2019 3:13 PM, Xing, Cedric wrote:
>>> On 7/8/2019 4:53 PM, Casey Schaufler wrote:
>>>> On 7/8/2019 10:16 AM, Xing, Cedric wrote:
>>>>> On 7/8/2019 9:26 AM, Casey Schaufler wrote:
>>>>>> In this scheme you use an ema LSM to manage your ema data.
>>>>>> A quick sketch looks like:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> security_enclave_load() calls
>>>>>> ema_enclave_load()
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>>
>>>>>> Why is this better than:
>>>>>>
>>>>>> sgx_something_in() calls
>>>>>> ema_enclave_load()
>>>>>> security_enclave_load() calls
>>>>>> selinux_enclave_load()
>>>>>> otherlsm_enclave_load()
>>>>>
>>>>> Are you talking about moving EMA somewhere outside LSM?
>>>>
>>>> Yes. That's what I've been saying all along.
>>>>
>>>>> If so, where?
>>>>
>>>> I tried to make it obvious. Put the call to your EMA code
>>>> on the line before you call security_enclave_load().
>>>
>>> Sorry but I'm still confused.
>>>
>>> EMA code is used by LSMs only. Making it callable from other parts of
>>> the kernel IMHO is probably not a good idea. And more importantly I
>>> don't understand the motivation behind it. Would you please elaborate?
>>
>> LSM modules implement additional access control restrictions.
>> The EMA code does not do that, it provides management of data
>> that is used by security modules. It is not one itself. VFS
>> also performs this role, but no one would consider making VFS
>> a security module.
>
> You are right. EMA is more like a helper library than a real LSM. But
> the practical problem is, it has a piece of initialization code, to
> basically request some space in the file blob from the LSM
> infrastructure. That cannot be done by any LSMs at runtime. So it has to
> either be done in LSM infrastructure directly, or make itself an LSM to
> make its initialization function invoked by LSM infrastructure
> automatically. You have objected to the former, so I switched to the
> latter. Are you now objecting to the latter as well? Then what are you
> suggesting, really?
>
> VFS is a completely different story. It's the file system abstraction so
> it has a natural place to live in the kernel, and its initialization
> doesn't depend on the LSM infrastructure. EMA on the other hand, shall
> belong to LSM because it is both produced and consumed within LSM.
>
> And, Stephen, do you have an opinion on this?
I don't really understand Casey's position. I also wouldn't necessarily
view his opinion on the matter as necessarily authoritative since he is
not the LSM maintainer as far as I know although he has contributed a
lot of changes in recent years.
I understood the architecture of the original EMA implementation (i.e. a
support library to be used by the security modules to help them in
implementing their own access control policies), although I do have some
concerns about the complexity and lifecycle issues, and wonder if a
simpler model as suggested by Dr. Greg isn't feasible.
I'd also feel better if there was clear consensus among all of the
@intel.com participants that this is the right approach. To date that
has seemed elusive.
If there were consensus that the EMA approach was the right one and that
a simpler model is not feasible, and if the only obstacle to adopting
EMA was Casey's objections, then I'd say just put it directly into
SELinux and be done with it. I originally thought that was a mistake
but if other security modules don't want the support, so be it.
>
>>>>>>> +/**
>>>>>>> + * ema - Enclave Memory Area structure for LSM modules
>>>>>>
>>>>>> LSM modules is redundant. "LSM" or "LSMs" would be better.
>>>>>
>>>>> Noted
>>>>>
>>>>>>> diff --git a/security/Makefile b/security/Makefile
>>>>>>> index c598b904938f..b66d03a94853 100644
>>>>>>> --- a/security/Makefile
>>>>>>> +++ b/security/Makefile
>>>>>>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
>>>>>>> obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
>>>>>>> obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
>>>>>>> obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
>>>>>>> +obj-$(CONFIG_INTEL_SGX) += commonema.o
>>>>>>
>>>>>> The config option and the file name ought to match,
>>>>>> or at least be closer.
>>>>>
>>>>> Just trying to match file names as "capability" uses commoncap.c.
>>>>
>>>> Fine, then you should be using CONFIG_SECURITY_EMA.
>>>>
>>>>>
>>>>> Like I said, this feature could potentially be used by TEEs other
>>>>> than SGX. For now, SGX is the only user so it is tied to
>>>>> CONFIG_INTEL_SGX. I can rename it to ema.c or enclave.c. Do you
>>>>> have a preference?
>>>>
>>>> Make
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX
>>>>
>>>> When another TEE (maybe MIPS_SSRPQ) comes along you can have
>>>>
>>>> CONFIG_SECURITY_EMA
>>>> depends on CONFIG_INTEL_SGX || CONFIG_MIPS_SSRPQ
>>>
>>> Your suggestions are reasonable. Given such config change wouldn't
>>> affect any code, can we do it later,
>>
>> That doesn't make the current options any less confusing,
>> and it will be easier to make the change now than at some
>> point in the future.
>>
>>> e.g., when additional TEEs come online and make use of these new
>>> hooks? After all, security_enclave_init() will need amendment anyway
>>> as one of its current parameters is of type 'struct sgx_sigstruct',
>>> which will need to be replaced with something more generic. At the
>>> time being, I'd like to keep things intuitive so as not to confuse
>>> reviewers.
>>
>> Reviewers (including me) are already confused by the inconsistency.
>
> OK. Let me make this change.
>
>>>>>
>>>>>>> diff --git a/security/commonema.c b/security/commonema.c
>>>>>>
>>>>>> Put this in a subdirectory. Please.
>>>>>
>>>>> Then why is commoncap.c located in this directory? I'm just trying
>>>>> to match the existing convention.
>>>>
>>>> commoncap is not optional. It is a base part of the
>>>> security subsystem. ema is optional.
>>>
>>> Alright. I'd move it into a sub-folder and rename it to ema.c. Would
>>> you be ok with that?
>>
>> Sounds fine.
>
> This is another part that confuses me. Per you comment here, I think you
> are OK with EMA being part of LSM (I mean, living somewhere under
> security/). But your other comment of calling ema_enclave_load()
> alongside security_enclave_load() made me think EMA and LSM were
> separate. What do you want really?
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-11 9:26 ` Jarkko Sakkinen@ 2019-07-11 14:32 ` Stephen Smalley
2019-07-11 17:51 ` Jarkko Sakkinen0 siblings, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-07-11 14:32 UTC (permalink / raw)
To: Jarkko Sakkinen, Xing, Cedric
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein
On 7/11/19 5:26 AM, Jarkko Sakkinen wrote:
> On Wed, Jul 10, 2019 at 04:16:42PM -0700, Xing, Cedric wrote:
>>> Still puzzling with EXECMOD given that how it is documented in
>>> https://selinuxproject.org/page/ObjectClassesPerms. If anything in that
>>> document is out of date, would be nice if it was updated.
>>
>> If you search for "EXECMOD" in security/selinux/hooks.c in the latest
>> (Linux-5.2) master, you'll find only one occurrence - at line 3702.
>>
>> The logic over there, if translated into English, basically says
>> FILE__EXECMOD is required (on the backing file) if mprotect() is called to
>> request X on a private file mapping that has been modified by the calling
>> process. That's what Sean meant by "W->X".
>
> Looking at that part of code, there is this comment:
>
> /*
> * We are making executable a file mapping that has
> * had some COW done. Since pages might have been
> * written, check ability to execute the possibly
> * modified content. This typically should only
> * occur for text relocations.
> */
>
> There is no COW done with enclaves, never. Thus, EXECMOD does not
> connect in any possible way to SGX. OR, that comment is false.
>
> Which one is it?
>
> Also the official documentation for SELinux speaks only about COW
> mappings.
>
> Also the condition supports all this as a *private* file mapping ends up
> to the anon_vma list when it gets written. We have a *shared* file
> mapping
>
> Nothing that you say makes sense to me, sorry...
The existing permissions don't map cleanly to SGX but I think Sean and
Cedric were trying to make a best-effort approximation to the underlying
concepts in a manner that permits control over the introduction of
executable content.
Sure, the existing EXECMOD check is only applied today when there is an
attempt to make executable a previously modified (detected based on COW
having occurred) private file mapping. But the general notion of
controlling the ability to execute modified content is still meaningful.
In the case of regular files, having both FILE__WRITE and FILE__EXECUTE
to the file is sufficient because that implies the ability to execute
modified content. And those FILE__* checks predated the introduction of
EXECMOD and EXECMEM.
The mapping of /dev/sgx/enclave doesn't really fit existing categories
because it doesn't provide the same semantics as a shared mapping of a
regular file. Userspace will always need both FILE__WRITE and
FILE__EXECUTE to /dev/sgx/enclave.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-11 13:51 ` Stephen Smalley@ 2019-07-11 15:12 ` Sean Christopherson
2019-07-11 16:11 ` Stephen Smalley0 siblings, 1 reply; 156+ messages in thread
From: Sean Christopherson @ 2019-07-11 15:12 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris
On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> I'd also feel better if there was clear consensus among all of the
> @intel.com participants that this is the right approach. To date that has
> seemed elusive.
That's a very kind way to phrase things :-)
For initial upstreaming, we've agreed that there is no need to extend the
uapi, i.e. we can punt on deciding between on-the-fly tracking and having
userspace specify maximal permissions until we add SGX2 support.
The last open (knock on wood) for initial upstreaming is whether SELinux
would prefer to have new enclave specific permissions or reuse the
existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
My understanding is that enclave specific permissions are preferred.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-11 15:12 ` Sean Christopherson@ 2019-07-11 16:11 ` Stephen Smalley
2019-07-11 16:25 ` Sean Christopherson0 siblings, 1 reply; 156+ messages in thread
From: Stephen Smalley @ 2019-07-11 16:11 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris, Paul Moore
On 7/11/19 11:12 AM, Sean Christopherson wrote:
> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>> I'd also feel better if there was clear consensus among all of the
>> @intel.com participants that this is the right approach. To date that has
>> seemed elusive.
>
> That's a very kind way to phrase things :-)
>
> For initial upstreaming, we've agreed that there is no need to extend the
> uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> userspace specify maximal permissions until we add SGX2 support.
>
> The last open (knock on wood) for initial upstreaming is whether SELinux
> would prefer to have new enclave specific permissions or reuse the
> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> My understanding is that enclave specific permissions are preferred.
I was left unclear on this topic after the email exchanges with Cedric.
There are at least three options:
1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
Existing distro policies will be applied in the expected manner with
respect to the introduction of executable code into the system,
consistent control will be provided over the enclave and the host
process, no change for users/documentation wrt policy. Cons: Existing
permissions don't map exactly to SGX semantics, no ability to
distinguish executable content within the enclave versus the host
process at the LSM level (argued earlier by Cedric to be unnecessary and
perhaps meaningless), need to allow FILE__EXECUTE or other checks on
sigstruct files that may not actually contain code.
2) Define new permissions within existing security classes (e.g.
process2, file). Pros: Can tailor permission names and definitions to
SGX semantics, ability to distinguish enclave versus host process
execute access, no need to grant FILE__EXECUTE to sigstruct files, class
matches the target object, permissions computed and cached upon existing
checks (i.e. when a process accesses a file, all of the permissions to
that file are computed and then cached at once, including the
enclave-related ones). Cons: Typical distro policies (unlike Android)
allow unknown permissions by default for forward kernel compatibility
reasons, so existing policies will permit these new permissions by
default and enforcement will only truly take effect once policies are
updated, adding new permissions to existing classes requires an update
to the base policy (so they can't be shipped as a third party policy
module alongside the SGX driver or installed as a local module by an
admin, for example), documentation/user education required for new
permissions.
3) Define new permissions in new security classes (e.g. enclave). Pros
relative to #2: New classes and permissions can be defined and installed
in third party or local policy module without requiring a change to the
base policy. Cons relative to #2: Class won't correspond to the target
object, permissions won't be computed and cached upon existing checks
(only when performing the checks against the new classes).
Combinations are also possible, of course.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-11 16:11 ` Stephen Smalley@ 2019-07-11 16:25 ` Sean Christopherson
2019-07-11 16:32 ` Stephen Smalley0 siblings, 1 reply; 156+ messages in thread
From: Sean Christopherson @ 2019-07-11 16:25 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Casey Schaufler, linux-sgx, linux-security-module,
selinux, James Morris, Paul Moore
On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
> On 7/11/19 11:12 AM, Sean Christopherson wrote:
> >On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
> >>I'd also feel better if there was clear consensus among all of the
> >>@intel.com participants that this is the right approach. To date that has
> >>seemed elusive.
> >
> >That's a very kind way to phrase things :-)
> >
> >For initial upstreaming, we've agreed that there is no need to extend the
> >uapi, i.e. we can punt on deciding between on-the-fly tracking and having
> >userspace specify maximal permissions until we add SGX2 support.
> >
> >The last open (knock on wood) for initial upstreaming is whether SELinux
> >would prefer to have new enclave specific permissions or reuse the
> >existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
> >My understanding is that enclave specific permissions are preferred.
>
> I was left unclear on this topic after the email exchanges with Cedric.
> There are at least three options:
>
> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
> Existing distro policies will be applied in the expected manner with respect
> to the introduction of executable code into the system, consistent control
> will be provided over the enclave and the host process, no change for
> users/documentation wrt policy. Cons: Existing permissions don't map
> exactly to SGX semantics, no ability to distinguish executable content
> within the enclave versus the host process at the LSM level (argued earlier
> by Cedric to be unnecessary and perhaps meaningless), need to allow
> FILE__EXECUTE or other checks on sigstruct files that may not actually
> contain code.
>
> 2) Define new permissions within existing security classes (e.g. process2,
> file). Pros: Can tailor permission names and definitions to SGX semantics,
> ability to distinguish enclave versus host process execute access, no need
> to grant FILE__EXECUTE to sigstruct files, class matches the target object,
> permissions computed and cached upon existing checks (i.e. when a process
> accesses a file, all of the permissions to that file are computed and then
> cached at once, including the enclave-related ones). Cons: Typical distro
> policies (unlike Android) allow unknown permissions by default for forward
> kernel compatibility reasons, so existing policies will permit these new
> permissions by default and enforcement will only truly take effect once
> policies are updated, adding new permissions to existing classes requires an
> update to the base policy (so they can't be shipped as a third party policy
> module alongside the SGX driver or installed as a local module by an admin,
> for example), documentation/user education required for new permissions.
>
> 3) Define new permissions in new security classes (e.g. enclave). Pros
> relative to #2: New classes and permissions can be defined and installed in
> third party or local policy module without requiring a change to the base
> policy. Cons relative to #2: Class won't correspond to the target object,
> permissions won't be computed and cached upon existing checks (only when
> performing the checks against the new classes).
>
> Combinations are also possible, of course.
What's the impact on distros/ecosystems if we go with #1 for now and later
decide to switch to #2 after upstreaming? I.e. can we take a minimal-ish
approach now without painting ourselves into a corner?
We can map quite closely to the existing intent of EXECUTE, EXECMOD and
EXECMEM via a combination of checking protections at enclave load time and
again at mmap()/mprotect(), e.g.:
#ifdef CONFIG_INTEL_SGX
static inline int enclave_has_perm(u32 sid, u32 requested)
{
return avc_has_perm(&selinux_state, sid, sid,
SECCLASS_PROCESS, requested, NULL);
}
static int selinux_enclave_map(unsigned long prot)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
return enclave_has_perm(sid, PROCESS__EXECMEM);
return 0;
}
static int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot)
{
const struct cred *cred = current_cred();
u32 sid = cred_sid(cred);
int ret;
/* Only executable enclave pages are restricted in any way. */
if (!(prot & PROT_EXEC))
return 0;
if (!vma->vm_file || IS_PRIVATE(file_inode(vma->vm_file))) {
ret = enclave_has_perm(sid, PROCESS__EXECMEM);
} else {
ret = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
if (ret)
goto out;
/*
* Load code from a modified private mapping or from a file
* with the ability to do W->X within the enclave.
*/
if (vma->anon_vma || (prot & PROT_WRITE))
ret = file_has_perm(cred, vma->vm_file,
FILE__EXECMOD);
}
out:
return ret;
}
#endif
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-11 14:32 ` Stephen Smalley@ 2019-07-11 17:51 ` Jarkko Sakkinen
2019-07-12 0:08 ` Xing, Cedric0 siblings, 1 reply; 156+ messages in thread
From: Jarkko Sakkinen @ 2019-07-11 17:51 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Sean Christopherson, linux-sgx,
linux-security-module, selinux, Bill Roberts, Casey Schaufler,
James Morris, Dave Hansen, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein
On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
> The existing permissions don't map cleanly to SGX but I think Sean and
> Cedric were trying to make a best-effort approximation to the underlying
> concepts in a manner that permits control over the introduction of
> executable content.
>
> Sure, the existing EXECMOD check is only applied today when there is an
> attempt to make executable a previously modified (detected based on COW
> having occurred) private file mapping. But the general notion of
> controlling the ability to execute modified content is still meaningful.
OK to summarize EXECMOD does not connect with SGX in any possible way
but SGX needs something that mimics EXECMOD behaviour?
And the same probably goes for EXECMEM, which is also private mapping
related concept.
> In the case of regular files, having both FILE__WRITE and FILE__EXECUTE to
> the file is sufficient because that implies the ability to execute modified
> content. And those FILE__* checks predated the introduction of EXECMOD and
> EXECMEM.
Right.
> The mapping of /dev/sgx/enclave doesn't really fit existing categories
> because it doesn't provide the same semantics as a shared mapping of a
> regular file. Userspace will always need both FILE__WRITE and FILE__EXECUTE
> to /dev/sgx/enclave.
Right.
Yeah, that has been the confusing part for me that they've been shuffled
in the discussion together with SGX concepts and hooks like there was
any connection. Thank you for clarifying this!
/Jarkko
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v3 3/4] X86/sgx: Introduce EMA as a new LSM module
2019-07-11 16:32 ` Stephen Smalley@ 2019-07-11 23:41 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-11 23:41 UTC (permalink / raw)
To: Stephen Smalley, Sean Christopherson
Cc: Casey Schaufler, linux-sgx, linux-security-module, selinux,
James Morris, Paul Moore
On 7/11/2019 9:32 AM, Stephen Smalley wrote:
> On 7/11/19 12:25 PM, Sean Christopherson wrote:
>> On Thu, Jul 11, 2019 at 12:11:06PM -0400, Stephen Smalley wrote:
>>> On 7/11/19 11:12 AM, Sean Christopherson wrote:
>>>> On Thu, Jul 11, 2019 at 09:51:19AM -0400, Stephen Smalley wrote:
>>>>> I'd also feel better if there was clear consensus among all of the
>>>>> @intel.com participants that this is the right approach. To date
>>>>> that has
>>>>> seemed elusive.
>>>>
>>>> That's a very kind way to phrase things :-)
>>>>
>>>> For initial upstreaming, we've agreed that there is no need to
>>>> extend the
>>>> uapi, i.e. we can punt on deciding between on-the-fly tracking and
>>>> having
>>>> userspace specify maximal permissions until we add SGX2 support.
>>>>
>>>> The last open (knock on wood) for initial upstreaming is whether
>>>> SELinux
>>>> would prefer to have new enclave specific permissions or reuse the
>>>> existing PROCESS__EXECMEM, FILE__EXECUTE and FILE__EXECMOD permissions.
>>>> My understanding is that enclave specific permissions are preferred.
>>>
>>> I was left unclear on this topic after the email exchanges with Cedric.
>>> There are at least three options:
>>>
>>> 1) Reuse the existing EXECMEM, EXECUTE, and EXECMOD permissions. Pros:
>>> Existing distro policies will be applied in the expected manner with
>>> respect
>>> to the introduction of executable code into the system, consistent
>>> control
>>> will be provided over the enclave and the host process, no change for
>>> users/documentation wrt policy. Cons: Existing permissions don't map
>>> exactly to SGX semantics, no ability to distinguish executable content
>>> within the enclave versus the host process at the LSM level (argued
>>> earlier
>>> by Cedric to be unnecessary and perhaps meaningless), need to allow
>>> FILE__EXECUTE or other checks on sigstruct files that may not actually
>>> contain code.
>>>
>>> 2) Define new permissions within existing security classes (e.g.
>>> process2,
>>> file). Pros: Can tailor permission names and definitions to SGX
>>> semantics,
>>> ability to distinguish enclave versus host process execute access, no
>>> need
>>> to grant FILE__EXECUTE to sigstruct files, class matches the target
>>> object,
>>> permissions computed and cached upon existing checks (i.e. when a
>>> process
>>> accesses a file, all of the permissions to that file are computed and
>>> then
>>> cached at once, including the enclave-related ones). Cons: Typical
>>> distro
>>> policies (unlike Android) allow unknown permissions by default for
>>> forward
>>> kernel compatibility reasons, so existing policies will permit these new
>>> permissions by default and enforcement will only truly take effect once
>>> policies are updated, adding new permissions to existing classes
>>> requires an
>>> update to the base policy (so they can't be shipped as a third party
>>> policy
>>> module alongside the SGX driver or installed as a local module by an
>>> admin,
>>> for example), documentation/user education required for new permissions.
>>>
>>> 3) Define new permissions in new security classes (e.g. enclave). Pros
>>> relative to #2: New classes and permissions can be defined and
>>> installed in
>>> third party or local policy module without requiring a change to the
>>> base
>>> policy. Cons relative to #2: Class won't correspond to the target
>>> object,
>>> permissions won't be computed and cached upon existing checks (only when
>>> performing the checks against the new classes).
>>>
>>> Combinations are also possible, of course.
>>
>> What's the impact on distros/ecosystems if we go with #1 for now and
>> later
>> decide to switch to #2 after upstreaming? I.e. can we take a minimal-ish
>> approach now without painting ourselves into a corner?
>
> Yes, I think that's fine.
I can't agree more on this. It's easier to add new things than to take
existing things out. We can just wait until usages come up that really
require new permissions.
^permalinkrawreply [flat|nested] 156+ messages in thread

*Re: [RFC PATCH v4 00/12] security: x86/sgx: SGX vs. LSM
2019-07-11 17:51 ` Jarkko Sakkinen@ 2019-07-12 0:08 ` Xing, Cedric0 siblings, 0 replies; 156+ messages in thread
From: Xing, Cedric @ 2019-07-12 0:08 UTC (permalink / raw)
To: Jarkko Sakkinen, Stephen Smalley
Cc: Sean Christopherson, linux-sgx, linux-security-module, selinux,
Bill Roberts, Casey Schaufler, James Morris, Dave Hansen,
Andy Lutomirski, Jethro Beekman, Dr . Greg Wettstein
On 7/11/2019 10:51 AM, Jarkko Sakkinen wrote:
> On Thu, Jul 11, 2019 at 10:32:41AM -0400, Stephen Smalley wrote:
>> The existing permissions don't map cleanly to SGX but I think Sean and
>> Cedric were trying to make a best-effort approximation to the underlying
>> concepts in a manner that permits control over the introduction of
>> executable content.
>>
>> Sure, the existing EXECMOD check is only applied today when there is an
>> attempt to make executable a previously modified (detected based on COW
>> having occurred) private file mapping. But the general notion of
>> controlling the ability to execute modified content is still meaningful.
>
> OK to summarize EXECMOD does not connect with SGX in any possible way
> but SGX needs something that mimics EXECMOD behaviour?
Stephen may correct me if I'm wrong. EXECMOD is granted to files, to
indicate the bearer contains self-modifying code (or text relocation).
So if it applies the enclaves, there are two aspects of it:
(1) An enclave may be loaded from multiple image files, among which some
may contain self-modifying code and hence would require EXECMOD on each
of them. At runtime, W->X will be allowed/denied for pages loaded from
image files having/not having EXECMOD.
(2) But there are pages not loaded from any files - e.g. pages EAUG'ed
at runtime. We are trying to use the file containing SIGSTRUCT as the
"proxy file" - i.e. EXECMOD on the proxy file indicates the enclave may
load code into EAUG'ed pages at runtime.
(3) Well, this is not an aspect of EXECMOD. Yet there's another category
of pages - pages EADD'ed from anonymous memory. They are different than
EAUG'ed pages in that their contents are supplied by untrusted code. How
to control their accesses is still being debated. My argument was that
the source pages must be executable before they could be loaded
executable in EPC. Andy argued that SIGSTRUCT alone could be considered
sufficient validation on the contents in certain usages, so both Sean
and I had proposed PROCESS2__ENCLAVE_EXECANON as a new permission. But
for the 1st upstream I think we all agree to do just the minimum until
real requirements come up. After all, adding is generally easier than
taking away existing things.
^permalinkrawreply [flat|nested] 156+ messages in thread