* Re: [PATCH 00/58] LSM: Module stacking for AppArmor
From: Stephen Smalley @ 2019-06-04 12:29 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
On 6/2/19 12:50 PM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
Please explain the motivation - why do we want to allow AppArmor to
stack with other modules, who would use it, how would it be used, what
does it provide that isn't already possible in the absence of it. Also,
Ubuntu fully upstreamed all of their changes to AppArmor, would this
still suffice to enable stacking of AppArmor or do they rely on hooks
that are not handled here?
Please explain the cost of the change - what do we pay in terms of
memory, runtime, or other overheads in order to support this change?
>
> A new process attribute identifies which security module
> information should be reported by SO_PEERSEC and the
> /proc/.../attr/current interface. This is provided by
> /proc/.../attr/display. Writing the name of the security
> module desired to this interface will set which LSM hooks
> will be called for this information. The first security
> module providing the hooks will be used by default.
Doesn't this effectively undo making the hooks read-only after init, at
least for the subset involved? What are the security implications thereof?
> The use of integer based security tokens (secids) is
> generally (but not completely) replaced by a structure
> lsm_export. The lsm_export structure can contain information
> for each of the security modules that export information
> outside the LSM layer.
>
> The LSM interfaces that provide "secctx" text strings
> have been changed to use a structure "lsm_context"
> instead of a pointer/length pair. In some cases the
> interfaces used a "char *" pointer and in others a
> "void *". This was necessary to ensure that the correct
> release mechanism for the text is used. It also makes
> many of the interfaces cleaner.
>
> https://github.com/cschaufler/lsm-stacking.git#stack-5.2-v1-apparmor
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> drivers/android/binder.c | 25 ++-
> fs/kernfs/dir.c | 6 +-
> fs/kernfs/inode.c | 31 ++-
> fs/kernfs/kernfs-internal.h | 3 +-
> fs/nfs/inode.c | 13 +-
> fs/nfs/internal.h | 8 +-
> fs/nfs/nfs4proc.c | 17 +-
> fs/nfs/nfs4xdr.c | 16 +-
> fs/nfsd/nfs4proc.c | 8 +-
> fs/nfsd/nfs4xdr.c | 14 +-
> fs/nfsd/vfs.c | 7 +-
> fs/proc/base.c | 1 +
> include/linux/cred.h | 3 +-
> include/linux/lsm_hooks.h | 91 +++++----
> include/linux/nfs4.h | 8 +-
> include/linux/security.h | 133 +++++++++----
> include/net/af_unix.h | 2 +-
> include/net/netlabel.h | 10 +-
> include/net/scm.h | 14 +-
> kernel/audit.c | 43 ++--
> kernel/audit.h | 9 +-
> kernel/auditfilter.c | 6 +-
> kernel/auditsc.c | 77 ++++----
> kernel/cred.c | 15 +-
> net/ipv4/cipso_ipv4.c | 13 +-
> net/ipv4/ip_sockglue.c | 12 +-
> net/netfilter/nf_conntrack_netlink.c | 29 ++-
> net/netfilter/nf_conntrack_standalone.c | 16 +-
> net/netfilter/nfnetlink_queue.c | 38 ++--
> net/netfilter/nft_meta.c | 13 +-
> net/netfilter/xt_SECMARK.c | 14 +-
> net/netlabel/netlabel_kapi.c | 5 +-
> net/netlabel/netlabel_unlabeled.c | 101 +++++-----
> net/netlabel/netlabel_unlabeled.h | 2 +-
> net/netlabel/netlabel_user.c | 13 +-
> net/netlabel/netlabel_user.h | 2 +-
> net/unix/af_unix.c | 6 +-
> security/apparmor/audit.c | 4 +-
> security/apparmor/include/audit.h | 2 +-
> security/apparmor/include/net.h | 6 +-
> security/apparmor/include/secid.h | 9 +-
> security/apparmor/lsm.c | 64 +++---
> security/apparmor/secid.c | 42 ++--
> security/integrity/ima/ima.h | 14 +-
> security/integrity/ima/ima_api.c | 9 +-
> security/integrity/ima/ima_appraise.c | 6 +-
> security/integrity/ima/ima_main.c | 34 ++--
> security/integrity/ima/ima_policy.c | 19 +-
> security/security.c | 338 +++++++++++++++++++++++++++-----
> security/selinux/hooks.c | 259 ++++++++++++------------
> security/selinux/include/audit.h | 5 +-
> security/selinux/include/objsec.h | 42 +++-
> security/selinux/netlabel.c | 25 +--
> security/selinux/ss/services.c | 18 +-
> security/smack/smack.h | 18 ++
> security/smack/smack_lsm.c | 238 +++++++++++-----------
> security/smack/smack_netfilter.c | 8 +-
> security/smack/smackfs.c | 12 +-
> 58 files changed, 1217 insertions(+), 779 deletions(-)
>
^ permalink raw reply
* Re: [RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()
From: Jarkko Sakkinen @ 2019-06-04 12:24 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190531233159.30992-5-sean.j.christopherson@intel.com>
On Fri, May 31, 2019 at 04:31:54PM -0700, Sean Christopherson wrote:
> SGX will use the mprotect() hook to prevent userspace from circumventing
> various security checks, i.e. Linux Security Modules.
>
> Enclaves are built by copying data from normal memory into the Enclave
> Page Cache (EPC). Due to the nature of SGX, the EPC is represented by a
> single file that must be MAP_SHARED, i.e. mprotect() only ever sees a
> single MAP_SHARED vm_file. Furthermore, all enclaves will need read,
> write and execute pages in the EPC.
What does the last sentence is pointing out? Enclaves read, write and
execute pages, so?
> As a result, LSM policies cannot be meaningfully applied, e.g. an LSM
> can deny access to the EPC as a whole, but can't deny PROT_EXEC on page
> that originated in a non-EXECUTE file (which is long gone by the time
> mprotect() is called).
I'm not sure what kind of scenario this is describing where some LSM
can't dent PROT_EXEC. Kind of cryptic paragraph, have to say.
> By hooking mprotect(), SGX can make explicit LSM upcalls while an
> enclave is being built, i.e. when the kernel has a handle to origin of
> each enclave page, and enforce the result of the LSM policy whenever
> userspace maps the enclave page in the future.
How does mprotect() enabled adding new LSM hooks?
> Alternatively, SGX could play games with MAY_{READ,WRITE,EXEC}, but
> that approach is quite ugly, e.g. would require userspace to call an
> SGX ioctl() prior to using mprotect() to extend a page's protections.
Not really sure I got this. SGX gets page permissions in SECINFO.
Also recurring comment about MAY_* constants.
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> include/linux/mm.h | 2 ++
> mm/mprotect.c | 15 +++++++++++----
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..50a42364a885 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -458,6 +458,8 @@ struct vm_operations_struct {
> void (*close)(struct vm_area_struct * area);
> int (*split)(struct vm_area_struct * area, unsigned long addr);
> int (*mremap)(struct vm_area_struct * area);
> + int (*mprotect)(struct vm_area_struct * area, unsigned long start,
> + unsigned long end, unsigned long prot);
Right, the hook must be here obviously because mprotect() can be called
when /dev/sgx/enclave is closed. Can you describe start and end i.e.
what range they are in?
> vm_fault_t (*fault)(struct vm_fault *vmf);
> vm_fault_t (*huge_fault)(struct vm_fault *vmf,
> enum page_entry_size pe_size);
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index bf38dfbbb4b4..e466ca5e4fe0 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -547,13 +547,20 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> goto out;
> }
>
> - error = security_file_mprotect(vma, reqprot, prot);
> - if (error)
> - goto out;
> -
> tmp = vma->vm_end;
> if (tmp > end)
> tmp = end;
> +
> + if (vma->vm_ops && vma->vm_ops->mprotect) {
> + error = vma->vm_ops->mprotect(vma, nstart, tmp, prot);
> + if (error)
> + goto out;
> + }
> +
> + error = security_file_mprotect(vma, reqprot, prot);
Why is mprotect callback called post the LSM hook?
> + if (error)
> + goto out;
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Jarkko Sakkinen @ 2019-06-04 11:55 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190531233159.30992-4-sean.j.christopherson@intel.com>
On Fri, May 31, 2019 at 04:31:53PM -0700, Sean Christopherson wrote:
> ...to improve performance when building enclaves by reducing the number
> of user<->system transitions. Rather than provide arbitrary batching,
> e.g. with per-page SECINFO and mrmask, take advantage of the fact that
> any sane enclave will have large swaths of pages with identical
> properties, e.g. code vs. data sections.
>
> For simplicity and stability in the initial implementation, loop over
> the existing add page flow instead of taking a more agressive approach,
> which would require tracking transitions between VMAs and holding
> mmap_sem for an extended duration.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
I think this completely ruins the rest of the series. We should first
get the model for security done (including documentation). I would even
send v21 with just that update because this series does not even apply
to the mainline.
I would request an update to the series with just the change to the
security model. Also the very first should be dropped as it is
completely unrelated cosmetic fix.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 2/9] x86/sgx: Do not naturally align MAP_FIXED address
From: Jarkko Sakkinen @ 2019-06-04 11:49 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190531233159.30992-3-sean.j.christopherson@intel.com>
On Fri, May 31, 2019 at 04:31:52PM -0700, Sean Christopherson wrote:
> SGX enclaves have an associated Enclave Linear Range (ELRANGE) that is
> tracked and enforced by the CPU using a base+mask approach, similar to
> how hardware range registers such as the variable MTRRs. As a result,
> the ELRANGE must be naturally sized and aligned.
>
> To reduce boilerplate code that would be needed in every userspace
> enclave loader, the SGX driver naturally aligns the mmap() address and
> also requires the range to be naturally sized. Unfortunately, SGX fails
> to grant a waiver to the MAP_FIXED case, e.g. incorrectly rejects mmap()
> if userspace is attempting to map a small slice of an existing enclave.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Why you want to allow mmap() to be called multiple times? mmap() could
be allowed only once with PROT_NONE and denied afterwards. Is this for
sending fd to another process that would map already existing enclave?
I don't see any checks for whether the is enclave underneath. Also, I
think that in all cases mmap() callback should allow only PROT_NONE
as permissions for clarity even if it could called multiple times.
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 1/9] x86/sgx: Remove unused local variable in sgx_encl_release()
From: Jarkko Sakkinen @ 2019-06-04 11:41 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190531233159.30992-2-sean.j.christopherson@intel.com>
On Fri, May 31, 2019 at 04:31:51PM -0700, Sean Christopherson wrote:
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
How this patch is essential to demonstrate anything?
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-04 11:39 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, William Roberts,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190603212336.GM13384@linux.intel.com>
On Mon, Jun 03, 2019 at 02:23:36PM -0700, Sean Christopherson wrote:
> Please read through the RFC, I think it address a lot of your questions.
> Hopefully that will help us avoid some thrash.
I promise to read it through with detail albeit I just said that as a
patch set it is broken :-) Internals still need documentation tho...
/Jarkko
^ permalink raw reply
* Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Jarkko Sakkinen @ 2019-06-04 11:15 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, Dave Hansen, Thomas Gleixner, Linus Torvalds,
LKML, X86 ML, linux-sgx, Andrew Morton, nhorman, npmccallum,
Serge Ayoun, Shay Katz-zamir, Haitao Huang, Andy Shevchenko,
Kai Svahn, Borislav Petkov, Josh Triplett, Kai Huang,
David Rientjes, William Roberts, Philip Tricca
In-Reply-To: <20190531233159.30992-1-sean.j.christopherson@intel.com>
On Fri, May 31, 2019 at 04:31:50PM -0700, Sean Christopherson wrote:
> This series is the result of a rather absurd amount of discussion over
> how to get SGX to play nice with LSM policies, without having to resort
> to evil shenanigans or put undue burden on userspace. The discussion
> definitely wandered into completely insane territory at times, but I
> think/hope we ended up with something reasonable.
By definition this is a broken series because it does not apply to
mainline. Even RFC series should at least apply. Would be better idea to
discuss design ideas and use snippets instead. Now you have to take
original v20 and apply to these patches to evaluate anything.
> The basic gist of the approach is to require userspace to declare what
> protections are maximally allowed for any given page, e.g. add a flags
> field for loading enclave pages that takes ALLOW_{READ,WRITE,EXEC}. LSMs
> can then adjust the allowed protections, e.g. clear ALLOW_EXEC to prevent
> ever mapping the page with PROT_EXEC. SGX enforces the allowed perms
> via a new mprotect() vm_ops hook, e.g. like regular mprotect() uses
> MAY_{READ,WRITE,EXEC}.
mprotect() does not use MAY_{READ,WRITE,EXEC} constants. It uses
VM_MAY{READ,WRITE,EXEC,SHARED} constants.
What are ALLOW_{READ,WRITE,EXEC} and how they are used? What does the
hook do and why it is in vm_ops and not in file_operations? Are they
arguments to the ioctl or internal variables that are set based on
SECINFO?
/Jarkko
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: James Bottomley @ 2019-06-04 8:57 UTC (permalink / raw)
To: Roberto Sassu, Mimi Zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <b38d75b1-873a-1630-0148-41c49571531a@huawei.com>
On Mon, 2019-06-03 at 16:44 +0200, Roberto Sassu wrote:
> On 6/3/2019 4:31 PM, James Bottomley wrote:
> > On Mon, 2019-06-03 at 16:29 +0200, Roberto Sassu wrote:
[...]
> > > How would you prevent root in the container from updating
> > > security.ima?
> >
> > We don't. We only guarantee immutability for unprivileged
> > containers, so root can't be inside.
>
> Ok.
>
> Regarding the new behavior, this must be explicitly enabled by adding
> ima_appraise=enforce-evm or log-evm to the kernel command line.
> Otherwise, the current behavior is preserved with this patch. Would
> this be ok?
Sure, as long as it's an opt-in flag, meaning the behaviour of my
kernels on physical cloud systems doesn't change as I upgrade them, I'm
fine with that.
James
^ permalink raw reply
* Re: [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Kaiwan N Billimoria @ 2019-06-04 6:01 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Andrew Morton, Christoph Lameter,
Dmitry Vyukov, James Morris, Jann Horn, Kostya Serebryany,
Laura Abbott, Mark Rutland, Masahiro Yamada, Matthew Wilcox,
Nick Desaulniers, Randy Dunlap, Sandeep Patil, Serge E. Hallyn,
Souptick Joarder, Marco Elver, Kernel Hardening,
Linux Memory Management List, linux-security-module
In-Reply-To: <201906032010.8E630B7@keescook>
On Tue, Jun 4, 2019 at 8:44 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jun 03, 2019 at 11:24:49AM +0200, Alexander Potapenko wrote:
> > On Sat, Jun 1, 2019 at 3:18 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 29 May 2019 14:38:11 +0200 Alexander Potapenko <glider@google.com> wrote:
> > >
> > > > Print the currently enabled stack and heap initialization modes.
> > > >
> > > > The possible options for stack are:
> > > > - "all" for CONFIG_INIT_STACK_ALL;
> > > > - "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
> > > > - "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
> > > > - "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
> > > > - "off" otherwise.
> > > >
> > > > Depending on the values of init_on_alloc and init_on_free boottime
> > > > options we also report "heap alloc" and "heap free" as "on"/"off".
> > >
> > > Why?
> > >
> > > Please fully describe the benefit to users so that others can judge the
> > > desirability of the patch. And so they can review it effectively, etc.
> > I'm going to update the description with the following passage:
> >
> > Print the currently enabled stack and heap initialization modes.
> >
> > Stack initialization is enabled by a config flag, while heap
> > initialization is configured at boot time with defaults being set
> > in the config. It's more convenient for the user to have all information
> > about these hardening measures in one place.
> >
> > Does this make sense?
> > > Always!
> > >
> > > > In the init_on_free mode initializing pages at boot time may take some
> > > > time, so print a notice about that as well.
> > >
> > > How much time?
> > I've seen pauses up to 1 second, not actually sure they're worth a
> > separate line in the log.
> > Kees, how long were the delays in your case?
>
> I didn't measure it, but I think it was something like 0.5 second per GB.
> I noticed because normally boot flashes by. With init_on_free it pauses
> for no apparent reason, which is why I suggested the note. (I mean *I*
> knew why it was pausing, but it might surprise someone who sets
> init_on_free=1 without really thinking about what's about to happen at
> boot.)
(Pardon the gmail client)
How about:
- if (want_init_on_free())
- pr_info("Clearing system memory may take some time...\n");
+ if (want_init_on_free())
+ pr_info("meminit: clearing system memory may take some
time...\n");
or even
+ if (want_init_on_free())
+ pr_info("meminit (init_on_free == 1): clearing system
memory may take some time...\n");
or some combo thereof?
--
Kaiwan
>
> --
> Kees Cook
>
^ permalink raw reply
* Re: [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Kees Cook @ 2019-06-04 3:14 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Dmitry Vyukov, James Morris,
Jann Horn, Kostya Serebryany, Laura Abbott, Mark Rutland,
Masahiro Yamada, Matthew Wilcox, Nick Desaulniers, Randy Dunlap,
Sandeep Patil, Serge E. Hallyn, Souptick Joarder, Marco Elver,
Kernel Hardening, Linux Memory Management List,
linux-security-module
In-Reply-To: <CAG_fn=XBq-ipvZng3hEiGwyQH2rRNFbN_Cj0r+5VoJqou0vovA@mail.gmail.com>
On Mon, Jun 03, 2019 at 11:24:49AM +0200, Alexander Potapenko wrote:
> On Sat, Jun 1, 2019 at 3:18 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 29 May 2019 14:38:11 +0200 Alexander Potapenko <glider@google.com> wrote:
> >
> > > Print the currently enabled stack and heap initialization modes.
> > >
> > > The possible options for stack are:
> > > - "all" for CONFIG_INIT_STACK_ALL;
> > > - "byref_all" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL;
> > > - "byref" for CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF;
> > > - "__user" for CONFIG_GCC_PLUGIN_STRUCTLEAK_USER;
> > > - "off" otherwise.
> > >
> > > Depending on the values of init_on_alloc and init_on_free boottime
> > > options we also report "heap alloc" and "heap free" as "on"/"off".
> >
> > Why?
> >
> > Please fully describe the benefit to users so that others can judge the
> > desirability of the patch. And so they can review it effectively, etc.
> I'm going to update the description with the following passage:
>
> Print the currently enabled stack and heap initialization modes.
>
> Stack initialization is enabled by a config flag, while heap
> initialization is configured at boot time with defaults being set
> in the config. It's more convenient for the user to have all information
> about these hardening measures in one place.
>
> Does this make sense?
> > Always!
> >
> > > In the init_on_free mode initializing pages at boot time may take some
> > > time, so print a notice about that as well.
> >
> > How much time?
> I've seen pauses up to 1 second, not actually sure they're worth a
> separate line in the log.
> Kees, how long were the delays in your case?
I didn't measure it, but I think it was something like 0.5 second per GB.
I noticed because normally boot flashes by. With init_on_free it pauses
for no apparent reason, which is why I suggested the note. (I mean *I*
knew why it was pausing, but it might surprise someone who sets
init_on_free=1 without really thinking about what's about to happen at
boot.)
--
Kees Cook
^ permalink raw reply
* Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-06-04 1:36 UTC (permalink / raw)
To: Xing, Cedric
Cc: Jarkko Sakkinen, Andy Lutomirski, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654ED042@ORSMSX116.amr.corp.intel.com>
On Mon, Jun 03, 2019 at 11:30:54AM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Monday, June 03, 2019 10:16 AM
> >
> > On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> > > Hi Sean,
> > >
> > > Generally I agree with your direction but think ALLOW_* flags are
> > > completely internal to LSM because they can be both produced and
> > > consumed inside an LSM module. So spilling them into SGX driver and
> > > also user mode code makes the solution ugly and in some cases
> > > impractical because not every enclave host process has a priori
> > > knowledge on whether or not an enclave page would be EMODPE'd at
> > runtime.
> >
> > In this case, the host process should tag *all* pages it *might* convert
> > to executable as ALLOW_EXEC. LSMs can (and should/will) be written in
> > such a way that denying ALLOW_EXEC is fatal to the enclave if and only
> > if the enclave actually attempts mprotect(PROT_EXEC).
>
> What if those pages contain self-modifying code but the host doesn't know
> ahead of time? Would it require ALLOW_WRITE|ALLOW_EXEC at EADD? Then would it
> prevent those pages to start with PROT_EXEC?
Without ALLOW_WRITE+ALLOW_EXEC, the enclave would build and launch, but
fail at mprotect(..., PROT_WRITE), e.g. when it attempted to gain write
access to do self-modifying code. And it would would fail irrespective of
LSM restrictions.
> Anyway, my point is that it is unnecessary even if it works.
Unnecessary in an ideal world, yes. Realistically, it's the least bad
option.
> > Take the SELinux path for example. The only scenario in which
> > PROT_WRITE is cleared from @allowed_prot is if the page *starts* with
> > PROT_EXEC.
> > If PROT_EXEC is denied on a page that starts RW, e.g. an EAUG'd page,
> > then PROT_EXEC will be cleared from @allowed_prot.
> >
> > As Stephen pointed out, auditing the denials on @allowed_prot means the
> > log will contain false positives of a sort. But this is more of a noise
> > issue than true false positives. E.g. there are three possible outcomes
> > for the enclave.
> >
> > - The enclave does not do EMODPE[PROT_EXEC] in any scenario, ever.
> > Requesting ALLOW_EXEC is either a straightforward a userspace bug or
> > a poorly written generic enclave loader.
> >
> > - The enclave conditionally performs EMODPE[PROT_EXEC]. In this case
> > the denial is a true false positive.
> >
> > - The enclave does EMODPE[PROT_EXEC] and its host userspace then fails
> > on mprotect(PROT_EXEC), i.e. the LSM denial is working as intended.
> > The audit log will be noisy, but viewed as a whole the denials
> > aren't
> > false positives.
>
> What I was talking about was EMODPE[PROT_WRITE] on an RX page.
As above, mprotect(..., PROT_WRITE) would fail without ALLOW_WRITE.
> > The potential for noisy audit logs and/or false positives is unfortunate,
> > but it's (by far) the lesser of many evils.
> >
> > > Theoretically speaking, what you really need is a per page flag (let's
> > > name it WRITTEN?) indicating whether a page has ever been written to
> > > (or more precisely, granted PROT_WRITE), which will be used to decide
> > > whether to grant PROT_EXEC when requested in future. Given the fact
> > > that all mprotect() goes through LSM and mmap() is limited to
> > > PROT_NONE, it's easy for LSM to capture that flag by itself instead of
> > asking user mode code to provide it.
> > >
> > > That said, here is the summary of what I think is a better approach.
> > > * In hook security_file_alloc(), if @file is an enclave, allocate some
> > data
> > > structure to store for every page, the WRITTEN flag as described
> > above.
> > > WRITTEN is cleared initially for all pages.
> >
> > This would effectively require *every* LSM to duplicate the SGX driver's
> > functionality, e.g. track per-page metadata, implement locking to
> > prevent races between multiple mm structs, etc...
>
> Architecturally we shouldn't dictate how LSM makes decisions. ALLOW_* are no
> difference than PROCESS__* or FILE__* flags, which are just artifacts to
> assist particular LSMs in decision making. They are never considered part of
> the LSM interface, even if other LSMs than SELinux may adopt the same/similar
> approach.
No, the flags are tracked and managed by SGX. We are not dictating LSM
behavior in any way, e.g. an LSM could completely ignore @allowed_prot and
nothing would break.
> If code duplication is what you are worrying about, you can put them in a
> library, or implement/export them in some new file (maybe
> security/enclave.c?) as utility functions.
Code duplication is the least of my concerns. Tracking file pointers
would require a global list/tree of some form, along with a locking and/or
RCU scheme to protect accesses to that container. Another lock would be
needed to prevent races between mprotect() calls from different processes.
> But spilling them into user mode is what I think is unacceptable.
Why is it unacceptable? There's effectively no cost to userspace for SGX1.
The ALLOW_* flags only come into play in the event of a noexec or LSM
restriction, i.e. worst case scenario an enclave that wants to do arbitrary
self-modifying code can declare RWX on everything.
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Sean Christopherson @ 2019-06-04 0:55 UTC (permalink / raw)
To: Xing, Cedric
Cc: Hansen, Dave, Jarkko Sakkinen, Andy Lutomirski, Stephen Smalley,
James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Thomas Gleixner,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes, Roberts, William C, Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654ED499@ORSMSX116.amr.corp.intel.com>
On Mon, Jun 03, 2019 at 04:48:47PM -0700, Xing, Cedric wrote:
> > How about this for the intermediate patch:
> >
> > struct sgx_enclave_add_region {
> > __u64 addr;
> > __u64 src;
> > __u64 size;
> > __u64 secinfo;
> > __u16 mrmask;
> > __u16 reserved16;
> > __u32 reserved;
> > }
> >
> > and with the flags field:
> >
> > struct sgx_enclave_add_region {
> > __u64 addr;
> > __u64 src;
> > __u64 size;
> > __u64 secinfo;
> > __u16 mrmask;
> > __u16 flags;
>
> What is "flags" here?
In the RFC, @flags holds SGX_ALLOW_{READ,WRITE,EXEC}.
>
> > __u32 reserved;
> > }
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Sean Christopherson @ 2019-06-04 0:54 UTC (permalink / raw)
To: Xing, Cedric
Cc: Jarkko Sakkinen, Andy Lutomirski, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <960B34DE67B9E140824F1DCDEC400C0F654ED478@ORSMSX116.amr.corp.intel.com>
On Mon, Jun 03, 2019 at 04:45:45PM -0700, Xing, Cedric wrote:
> > From: Christopherson, Sean J
> > Sent: Monday, June 03, 2019 1:40 PM
> >
> > On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > > From: Christopherson, Sean J
> > > > > Sent: Friday, May 31, 2019 4:32 PM
> > > > >
> > > > > +/**
> > > > > + * sgx_ioc_enclave_add_pages - handler for
> > > > > +%SGX_IOC_ENCLAVE_ADD_PAGES
> > > > > + *
> > > > > + * @filep: open file to /dev/sgx
> > > > > + * @cmd: the command value
> > > > > + * @arg: pointer to an &sgx_enclave_add_page instance
> > > > > + *
> > > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > > +optionally
> > > > > + * extend the enclave's measurement with the contents of the page
> > (EEXTEND).
> > > > > + * The range of pages must be virtually contiguous. The SECINFO
> > > > > +and
> > > > > + * measurement maskare applied to all pages, i.e. pages with
> > > > > +different
> > > > > + * properties must be added in separate calls.
> > > > > + *
> > > > > + * EADD and EEXTEND are done asynchronously via worker threads.
> > > > > +A successful
> > > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been
> > > > > +added to the
> > > > > + * work queue, it does not guarantee adding the pages to the
> > > > > +enclave will
> > > > > + * succeed.
> > > > > + *
> > > > > + * Return:
> > > > > + * 0 on success,
> > > > > + * -errno otherwise
> > > > > + */
> > > > > +static long sgx_ioc_enclave_add_pages(struct file *filep,
> > unsigned int cmd,
> > > > > + unsigned long arg)
> > > > > +{
> > > > > + struct sgx_enclave_add_pages *addp = (void *)arg;
> > > > > + struct sgx_encl *encl = filep->private_data;
> > > > > + struct sgx_secinfo secinfo;
> > > > > + unsigned int i;
> > > > > + int ret;
> > > > > +
> > > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > > + sizeof(secinfo)))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > > + if (signal_pending(current))
> > > > > + return -ERESTARTSYS;
> > > >
> > > > If interrupted, how would user mode code know how many pages have
> > been EADD'ed?
> > >
> > > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > > userspace, e.g. as opposed to overloading the return value.
> >
> > Or maybe update @addr and @src as well? That would allow userspace to
> > re-invoke the ioctl() without having to modify the struct.
>
> How about returning the number of pages (or bytes) EADD'ed, similar to
> write() syscall?
I thought about that as well, but I think it'd be useful to update the
offset on any failure, not just -ERESTARTSYS.
^ permalink raw reply
* Re: [PATCH 05/58] LSM: Use lsm_export in the inode_getsecid hooks
From: Casey Schaufler @ 2019-06-04 0:29 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, Stephen Smalley, casey
In-Reply-To: <201906011845.96774A4@keescook>
On 6/1/2019 6:57 PM, Kees Cook wrote:
> On Fri, May 31, 2019 at 04:09:27PM -0700, Casey Schaufler wrote:
>> Convert the inode_getsecid hooks to use the lsm_export
>> structure instead of a u32 secid. There is some scaffolding
>> involved that will be removed when security_inode_getsecid()
>> is updated.
> So, there are like 20 patches that all have basically identical subject
> and changelog, but some evolve the API in subtle ways. For example,
> in this patch, there is no mention of adding lsm_export_init(). I would
> expect all the lsm_export infrastructure and support functions to be
> introduced in patch 4 where struct lsm_export is initially introduced.
Fair enough. I didn't introduce helpers until they were used.
I hoped to avoid the "what is this for?" question that can
come up when you add functions that aren't used.
> Instead, various helper functions are scattered through these patches
> and I'm left struggling to figure out where things are actually
> changing.
I think it's possible that the patches may be too small
to contain enough context for them to be sensible. It may
make things more obvious if I combined
[PATCH 05/58] LSM: Use lsm_export in the inode_getsecid hooks
[PATCH 20/58] LSM: Use lsm_export in security_inode_getsecid
into a single patch. That would reduce the amount of scaffolding
that has to get set up and torn down.
> Also, I find the helper naming to be not easy to follow but this is
> mainly due to me repeatedly trying to parse the helpers as
> noun-verb-noun, so lsm_export_secid() kind of makes sense ("write to
> secid, based on the flags") but then I see smack_export_secid() and this
> is "write to the internal lsm_export storage, the value of this secid".
> The direction here is what ends up confusing me. Which direction is the
> data moving?
>
> lsm_export_to_secid() <- from lsm_export to secid
> smack_secid_to_lsm_export() <- from secid to smack's lsm_export record
The inconsistency is comes from my use of "lsm_export" for
the name of the LSM data structure. This is something you've
commented on elsewhere. The underscore makes the function name
look like it has an lsm_ prefix, rather than just being the
name of the structure. If I changed "struct lsm_export" to
"struct lsmdata" the names:
lsm_lsmdata_to_secid() and smack_secid_to_lsmdata()
would be more consistent.
>> +static inline void selinux_export_secid(struct lsm_export *l, u32 secid)
>> +{
>> + l->selinux = secid;
>> + l->flags |= LSM_EXPORT_SELINUX;
>> +}
> Which brings me to another thing I find awkward here: I feel like an LSM
> shouldn't need to do anything with this object: it should be opaque to
> the LSM. The LSM infrastructure knows which LSM it has called into. Why
> isn't this just like the other blobs?
There's a lot more rework required if the lsm_export has to be
life-cycle managed. The audit code, for example, passes them about,
copying, storing and dropping them without a care. I'm not completely
opposed to taking that on, but it's essentially a rewrite of the
audit LSM handling. The SO_PEERSEC handling probably has issues as
well. I think netlabel would be OK, but there's stuff going on elsewhere
in the networking stack that isn't going to like anything it has to
worry about allocating and/or freeing.
> Anyway, I'll keep reading maybe I just haven't gotten far enough, but
> I'd love some help in the 0/NN cover letter describing what the
> evolution actually does through the series so I can follow along and you
> can set my expectations about what I'll be looking for in each patch.
OK. Good feedback. The next batch will be better.
^ permalink raw reply
* RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Xing, Cedric @ 2019-06-03 23:48 UTC (permalink / raw)
To: Christopherson, Sean J, Hansen, Dave
Cc: Jarkko Sakkinen, Andy Lutomirski, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Thomas Gleixner,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes, Roberts, William C, Tricca, Philip B
In-Reply-To: <20190603203712.GI13384@linux.intel.com>
> -----Original Message-----
> From: Christopherson, Sean J
> Sent: Monday, June 03, 2019 1:37 PM
> To: Hansen, Dave <dave.hansen@intel.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Andy Lutomirski
> <luto@kernel.org>; Xing, Cedric <cedric.xing@intel.com>; Stephen Smalley
> <sds@tycho.nsa.gov>; James Morris <jmorris@namei.org>; Serge E . Hallyn
> <serge@hallyn.com>; LSM List <linux-security-module@vger.kernel.org>;
> Paul Moore <paul@paul-moore.com>; Eric Paris <eparis@parisplace.org>;
> selinux@vger.kernel.org; Jethro Beekman <jethro@fortanix.com>; Thomas
> Gleixner <tglx@linutronix.de>; Linus Torvalds <torvalds@linux-
> foundation.org>; LKML <linux-kernel@vger.kernel.org>; X86 ML
> <x86@kernel.org>; linux-sgx@vger.kernel.org; Andrew Morton <akpm@linux-
> foundation.org>; nhorman@redhat.com; npmccallum@redhat.com; Ayoun, Serge
> <serge.ayoun@intel.com>; Katz-zamir, Shay <shay.katz-zamir@intel.com>;
> Huang, Haitao <haitao.huang@intel.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Svahn, Kai <kai.svahn@intel.com>;
> Borislav Petkov <bp@alien8.de>; Josh Triplett <josh@joshtriplett.org>;
> Huang, Kai <kai.huang@intel.com>; David Rientjes <rientjes@google.com>;
> Roberts, William C <william.c.roberts@intel.com>; Tricca, Philip B
> <philip.b.tricca@intel.com>
> Subject: Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple
> pages in single ioctl()
>
> On Mon, Jun 03, 2019 at 01:14:45PM -0700, Dave Hansen wrote:
> > On 5/31/19 4:31 PM, Sean Christopherson wrote:
> > > -struct sgx_enclave_add_page {
> > > +struct sgx_enclave_add_pages {
> > > __u64 addr;
> > > __u64 src;
> > > __u64 secinfo;
> > > + __u32 nr_pages;
> > > __u16 mrmask;
> > > } __attribute__((__packed__));
> >
> > IMNHO this follows a user interface anti-pattern: exposing page sizes
> > where not strictly required.
> >
> > Think of how this would look to an application if page size was
> > variable. With this interface, they always need to scale their
> > operations by page size instead of just aligning it.
>
> I briefly considered taking size in bytes, but I took a shortcut because
> EPC pages are architecturally defined to be 4k sized and aligned. That
> being said, I don't necessarily disagree, especially if nr_pages isn't
> squeezed into a u32.
>
> > BTW, why is nr_pages a u32? Do we never envision a case where you can
> > add more than 4TB of memory to an enclave? ;)
>
> Heh, fair enough. IIRC, a while back someone posted about having
> problems building a 512gb enclave in a 92mb EPC...
>
> How about this for the intermediate patch:
>
> struct sgx_enclave_add_region {
> __u64 addr;
> __u64 src;
> __u64 size;
> __u64 secinfo;
> __u16 mrmask;
> __u16 reserved16;
> __u32 reserved;
> }
>
> and with the flags field:
>
> struct sgx_enclave_add_region {
> __u64 addr;
> __u64 src;
> __u64 size;
> __u64 secinfo;
> __u16 mrmask;
> __u16 flags;
What is "flags" here?
> __u32 reserved;
> }
^ permalink raw reply
* RE: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Xing, Cedric @ 2019-06-03 23:45 UTC (permalink / raw)
To: Christopherson, Sean J
Cc: Jarkko Sakkinen, Andy Lutomirski, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes, Roberts, William C,
Tricca, Philip B
In-Reply-To: <20190603203950.GJ13384@linux.intel.com>
> From: Christopherson, Sean J
> Sent: Monday, June 03, 2019 1:40 PM
>
> On Mon, Jun 03, 2019 at 01:08:04PM -0700, Sean Christopherson wrote:
> > On Sun, Jun 02, 2019 at 11:26:09PM -0700, Xing, Cedric wrote:
> > > > From: Christopherson, Sean J
> > > > Sent: Friday, May 31, 2019 4:32 PM
> > > >
> > > > +/**
> > > > + * sgx_ioc_enclave_add_pages - handler for
> > > > +%SGX_IOC_ENCLAVE_ADD_PAGES
> > > > + *
> > > > + * @filep: open file to /dev/sgx
> > > > + * @cmd: the command value
> > > > + * @arg: pointer to an &sgx_enclave_add_page instance
> > > > + *
> > > > + * Add a range of pages to an uninitialized enclave (EADD), and
> > > > +optionally
> > > > + * extend the enclave's measurement with the contents of the page
> (EEXTEND).
> > > > + * The range of pages must be virtually contiguous. The SECINFO
> > > > +and
> > > > + * measurement maskare applied to all pages, i.e. pages with
> > > > +different
> > > > + * properties must be added in separate calls.
> > > > + *
> > > > + * EADD and EEXTEND are done asynchronously via worker threads.
> > > > +A successful
> > > > + * sgx_ioc_enclave_add_page() only indicates the pages have been
> > > > +added to the
> > > > + * work queue, it does not guarantee adding the pages to the
> > > > +enclave will
> > > > + * succeed.
> > > > + *
> > > > + * Return:
> > > > + * 0 on success,
> > > > + * -errno otherwise
> > > > + */
> > > > +static long sgx_ioc_enclave_add_pages(struct file *filep,
> unsigned int cmd,
> > > > + unsigned long arg)
> > > > +{
> > > > + struct sgx_enclave_add_pages *addp = (void *)arg;
> > > > + struct sgx_encl *encl = filep->private_data;
> > > > + struct sgx_secinfo secinfo;
> > > > + unsigned int i;
> > > > + int ret;
> > > > +
> > > > + if (copy_from_user(&secinfo, (void __user *)addp->secinfo,
> > > > + sizeof(secinfo)))
> > > > + return -EFAULT;
> > > > +
> > > > + for (i = 0, ret = 0; i < addp->nr_pages && !ret; i++) {
> > > > + if (signal_pending(current))
> > > > + return -ERESTARTSYS;
> > >
> > > If interrupted, how would user mode code know how many pages have
> been EADD'ed?
> >
> > Hmm, updating nr_pages would be fairly simple and shouldn't confuse
> > userspace, e.g. as opposed to overloading the return value.
>
> Or maybe update @addr and @src as well? That would allow userspace to
> re-invoke the ioctl() without having to modify the struct.
How about returning the number of pages (or bytes) EADD'ed, similar to write() syscall?
^ permalink raw reply
* Re: [PATCH] Smack: Restore the smackfsdef mount option and add missing prefixes
From: Casey Schaufler @ 2019-06-03 23:07 UTC (permalink / raw)
To: James Morris, David Howells
Cc: viro, stable, Jose Bollo, torvalds, linux-security-module,
linux-kernel, casey
In-Reply-To: <alpine.LRH.2.21.1906040842110.13657@namei.org>
On 6/3/2019 3:42 PM, James Morris wrote:
> On Fri, 31 May 2019, David Howells wrote:
>
>> Should this go via Al's tree, James's tree, Casey's tree or directly to Linus?
> If it's specific to one LSM (as this is), via Casey, who can decide to
> forward to Al or Linus.
I would very much appreciate it if Al could send this fix along.
I am not fully set up for sending directly to Linus.
^ permalink raw reply
* Re: [PATCH] Smack: Restore the smackfsdef mount option and add missing prefixes
From: James Morris @ 2019-06-03 22:42 UTC (permalink / raw)
To: David Howells
Cc: viro, stable, Jose Bollo, Casey Schaufler, torvalds,
linux-security-module, linux-kernel
In-Reply-To: <17467.1559300202@warthog.procyon.org.uk>
On Fri, 31 May 2019, David Howells wrote:
> Should this go via Al's tree, James's tree, Casey's tree or directly to Linus?
If it's specific to one LSM (as this is), via Casey, who can decide to
forward to Al or Linus.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-03 21:08 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, William Roberts,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190530213601.GC27551@linux.intel.com>
On Thu, May 30, 2019 at 02:36:01PM -0700, Sean Christopherson wrote:
> Assuming MRENCLAVE generated by Graphene or any other hosting scheme are
> stable[1], then avoiding EXEC<whatever> means the user can effectively
> whitelist what enclaves are runnable by Graphene, even if the kernel
> doesn't implement security_enclave_create/init().
>
> I agree that it probably isn't all that important, it's more of a "why
> not" argument, i.e. what is gained by not using sigstruct as a proxy?
>
> [1] What in the world is being attested if MRENCLAVE isn't stable?
If I've understood correctly, Graphene uses a single loader enclave
that loads the executable in.
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-03 21:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, William Roberts,
James Morris, Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jethro Beekman, Hansen, Dave,
Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML, X86 ML,
linux-sgx@vger.kernel.org, Andrew Morton, nhorman@redhat.com,
npmccallum@redhat.com, Ayoun, Serge, Katz-zamir, Shay,
Huang, Haitao, Andy Shevchenko, Svahn, Kai, Borislav Petkov,
Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <20190530180110.GB23930@linux.intel.com>
On Thu, May 30, 2019 at 11:01:10AM -0700, Sean Christopherson wrote:
> - Requires enclave builder to mark enclave pages executable in the
> non-enclave VMAs, which may unnecessarily require EXECMOD on the
> source file, or even worse, EXECMEM, and potentially increases the
> attack surface since the file must be executable.
Enclave builder marks *non-enclave pages*? Not following.
> W^X handling:
> - mmap() to /dev/sgx/enclave only allowed with PROT_NONE, i.e. force
> userspace through mprotect() to simplify the kernel implementation.
> - Add vm_ops mprotect() ops hook (I'll refer to SGX's implementation
> as SGX.mprotect())
> - Take explicit ALLOW_WRITE at ADD_REGION, a.k.a. EADD
> - ADD_REGION also used to describe EAUG region (tentatively for SGX2).
> - Track "can be written at some point in time (past or future)" as
> ALLOW_WRITE (to avoid confusiong with MAY_WRITE). A priori knowledge
> of writability avoids having to track/coordinate PROT_WRITE across
> VMAs and MMs.
Still not sure why you want to use vm_ops instead of file_operations.
The approach I've been proposing earlier in this email thread before
these new proposals can be summarized from hook perspective as:
- Allow mmap() only before ECREATE and require it to be size
of the ELRANGE (ECREATE ioctl would check this). This would
be with PROT_NONE.
- Disallow mprotect() before EINIT. Requires a new callback
to file_operations like mmap() has.
- After EINIT check for each mprotect() that it matches the
permissions of underlying enclave pages. Disallow mmap()
after EINIT.
/Jarkko
^ permalink raw reply
* Re: [PATCH 22/58] Audit: Change audit_sig_sid to audit_sig_lsm
From: Casey Schaufler @ 2019-06-03 22:23 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201906011900.143B72A@keescook>
On 6/1/2019 7:03 PM, Kees Cook wrote:
> On Fri, May 31, 2019 at 04:09:44PM -0700, Casey Schaufler wrote:
>> Remove lsm_export scaffolding around audit_sig_sid by
>> changing the u32 secid into an lsm_export structure named
>> audit_sig_lsm.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> include/linux/security.h | 7 +++++++
>> kernel/audit.c | 18 ++++++++----------
>> kernel/audit.h | 2 +-
>> kernel/auditsc.c | 3 +--
>> 4 files changed, 17 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 40aa7b9f3c83..e76d7a9dbe50 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -93,6 +93,13 @@ static inline void lsm_export_init(struct lsm_export *l)
>> memset(l, 0, sizeof(*l));
>> }
>>
>> +static inline bool lsm_export_any(struct lsm_export *l)
>> +{
>> + return (((l->flags & LSM_EXPORT_SELINUX) && l->selinux) ||
>> + ((l->flags & LSM_EXPORT_SMACK) && l->smack) ||
>> + ((l->flags & LSM_EXPORT_APPARMOR) && l->apparmor));
>> +}
> All of these helpers need kerndoc.
Point.
> Bikeshed on naming:
> - struct lsm_export renamed to lsm_secid
I want to get away from the expectation that what an
LSM exports has to be a u32 secid. It's not in any patchset
yet, but I plan to replace the Smack u32 with a struct smack_known *
at some point in the future. That will require a little work
in the secmark code, but will have significant performance
improvement in audit and UDS.
> - lsm_export_any renamed to lsm_secid_defined() or ..._is_set() or
> ..._non_zero() ?
I'll admit lsm_export_any() isn't a great name. The state it has
to convey is "some LSM has set a value, and it isn't an error value."
Like "secid != 0", except that it matters whether the 0 came from
secid having never been set, as opposed to it was set because something
went wrong. At the same time, I don't want it to imply that the value
is set for all LSMs, because it may not be. That's why I used "any".
Some LSM *has* set a value. That value may not be the one you're hoping
for, but you may need to call the subsystem (e.g.audit) that's going to
look.
Maybe lsm_export_is_interesting()?
I'd love to discover there's a convention I could adhere to.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-03 20:54 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Xing, Cedric, Christopherson, Sean J,
William Roberts, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrWsEXzUC33eJpGCpdMCBO4aYVviZLRD-CLMNaG5Jv-TCA@mail.gmail.com>
On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote:
> > What is the "source file" i.e. the target of the check? Enclave file,
> > sigstruct file, or /dev/sgx/enclave?
>
> Enclave file -- that is, the file backing the vma from which the data
> is loaded.
Wonder why KVM gets away without having this given that enclaves are
lot alike VMs.
> It's provided by userspace based on whether it thinks the data in
> question is enclave code. source->vm_file is the file from which the
> code is being loaded. I'm assuming that the user code will only set
> excute_intent ==true if it actually wants to execute the code, so, if
> there's a denial, it will be fatal. The normal case will be that the
> request will be granted on the basis of EXECUTE.
AFAIK user spaces tells that already with the SECINFO flags. I don't
get why we need a duplicate parameter.
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-03 20:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Smalley, Xing, Cedric, Christopherson, Sean J,
William Roberts, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <CALCETrWXB9fNNDH7gZxPTx05F78Og6K=ZtAr2aA++BDwY09Wbg@mail.gmail.com>
On Thu, May 30, 2019 at 07:31:14AM -0700, Andy Lutomirski wrote:
> - To create an X mapping of an enclave page that came from EADD, you
> need EXECUTE on the source file. Optionally, we could also permit
> this if you have EXECMOD.
Source file? EADD ioctl takes memory buffer in right now.
> And I have two design proposals. One is static and one is dynamic.
> To implement either one, we will probably need a new .may_mprotect vm
> operation, and that operation can call an LSM hook. Or we can give
> LSMs a way to detect that a given vm_area_struct is an enclave. As I
> see it, this is an implementation detail that is certainly solveable.
Why VM operation and not file operation?
> EADD takes an execute_intent flag. It calls a new hook:
>
> int security_enclave_load(struct vm_area_struct *source, bool execute_intent);
>
> This hook will fail if execute_intent==true and the caller has neither
> EXECUTE, EXECMOD, nor EXECMEM.
>
> EAUG sets execute_intent = false.
>
> EINIT takes a sigstruct pointer. SGX can (when initially upstreamed
> or later on once there's demand) call a new hook:
>
> security_enclave_init(struct sigstruct *sigstruct, struct
> vm_area_struct *source);
What is the source VMA in these callbacks? Why is @execute_intent
needed anyway as a ioctl arugment and not deduced from SECINFO?
/Jarkko
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Jarkko Sakkinen @ 2019-06-03 20:47 UTC (permalink / raw)
To: Stephen Smalley
Cc: Andy Lutomirski, Xing, Cedric, Christopherson, Sean J,
William Roberts, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Eric Paris, selinux@vger.kernel.org, Jethro Beekman,
Hansen, Dave, Thomas Gleixner, Dr. Greg, Linus Torvalds, LKML,
X86 ML, linux-sgx@vger.kernel.org, Andrew Morton,
nhorman@redhat.com, npmccallum@redhat.com, Ayoun, Serge,
Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko, Svahn, Kai,
Borislav Petkov, Josh Triplett, Huang, Kai, David Rientjes
In-Reply-To: <c1135352-0b5e-4694-b1a9-105876095877@tycho.nsa.gov>
On Thu, May 30, 2019 at 11:04:24AM -0400, Stephen Smalley wrote:
> Does this occur for both setting initial permissions and runtime permissions
> or just runtime? Both userspace- and driver-initiated mmap/mprotect
> operations or just userspace-initiated ones? Does the driver use interfaces
> that call the mmap/mprotect hooks or lower level functions?
The driver never initiates mmap() or mprotect().
/Jarkko
^ permalink raw reply
* Re: possible deadlock in __do_page_fault (2)
From: syzbot @ 2019-06-03 22:04 UTC (permalink / raw)
To: adilger.kernel, hdanton, jmorris, linux-ext4, linux-kernel,
linux-security-module, serge, syzkaller-bugs, tytso, zohar
In-Reply-To: <000000000000fa91e1058a358cd5@google.com>
syzbot has bisected this bug to:
commit 69d61f577d147b396be0991b2ac6f65057f7d445
Author: Mimi Zohar <zohar@linux.ibm.com>
Date: Wed Apr 3 21:47:46 2019 +0000
ima: verify mprotect change is consistent with mmap policy
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16461c5aa00000
start commit: 3c09c195 Add linux-next specific files for 20190531
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=15461c5aa00000
console output: https://syzkaller.appspot.com/x/log.txt?x=11461c5aa00000
kernel config: https://syzkaller.appspot.com/x/.config?x=6cfb24468280cd5c
dashboard link: https://syzkaller.appspot.com/bug?extid=606e524a3ca9617cf8c0
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10572ca6a00000
Reported-by: syzbot+606e524a3ca9617cf8c0@syzkaller.appspotmail.com
Fixes: 69d61f577d14 ("ima: verify mprotect change is consistent with mmap
policy")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox