* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-06-03 14:44 UTC (permalink / raw)
To: James Bottomley, Mimi Zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <1559572305.5052.19.camel@HansenPartnership.com>
On 6/3/2019 4:31 PM, James Bottomley wrote:
> On Mon, 2019-06-03 at 16:29 +0200, Roberto Sassu wrote:
>> On 6/3/2019 3:43 PM, James Bottomley wrote:
>>> On Mon, 2019-06-03 at 11:25 +0200, Roberto Sassu wrote:
>>>> On 5/30/2019 2:00 PM, Mimi Zohar wrote:
>>>>> On Wed, 2019-05-29 at 15:30 +0200, Roberto Sassu wrote:
>>>>>> Currently, ima_appraise_measurement() ignores the EVM status
>>>>>> when evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file
>>>>>> has a valid security.ima xattr with type IMA_XATTR_DIGEST or
>>>>>> IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns
>>>>>> INTEGRITY_PASS regardless of the EVM status. The problem is
>>>>>> that the EVM status is overwritten with the appraisal statu
>>>>>
>>>>> Roberto, your framing of this problem is harsh and
>>>>> misleading. IMA and EVM are intentionally independent of each
>>>>> other and can be configured independently of each other. The
>>>>> intersection of the two is the call to
>>>>> evm_verifyxattr(). INTEGRITY_UNKNOWN is
>>>>> returned for a number of reasons - when EVM is not configured,
>>>>> the EVM hmac key has not yet been loaded, the protected
>>>>> security attribute is unknown, or the file is not in policy.
>>>>>
>>>>> This patch does not differentiate between any of the above
>>>>> cases, requiring mutable files to always be protected by EVM,
>>>>> when specified as an "ima_appraise=" option on the boot command
>>>>> line.
>>>>>
>>>>> IMA could be extended to require EVM on a per IMA policy rule
>>>>> basis. Instead of framing allowing IMA file hashes without EVM
>>>>> as a bug that has existed from the very beginning, now that
>>>>> IMA/EVM have matured and is being used, you could frame it as
>>>>> extending IMA or hardening.
>>>>
>>>> I'm seeing it from the perspective of an administrator that
>>>> manages an already hardened system, and expects that the system
>>>> only grants access to files with a valid signature/HMAC. That
>>>> system would not enforce this behavior if EVM keys are removed
>>>> and the digest in security.ima is set to the actual file digest.
>>>>
>>>> Framing it as a bug rather than an extension would in my opinion
>>>> help to convince people about the necessity to switch to the safe
>>>> mode, if their system is already hardened.
>>>
>>> I have a use case for IMA where I use it to enforce immutability of
>>> containers. In this use case, the cluster admin places hashes on
>>> executables as the image is unpacked so that if an executable file
>>> is changed, IMA will cause an execution failure. For this use
>>> case, I don't care about the EVM, in fact we don't use it, because
>>> the only object is to fail execution if a binary is mutated.
>>
>> 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?
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [RFC PATCH 9/9] security/selinux: Add enclave_load() implementation
From: Stephen Smalley @ 2019-06-03 15:01 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, 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-10-sean.j.christopherson@intel.com>
On 5/31/19 7:31 PM, Sean Christopherson wrote:
> The goal of selinux_enclave_load() is to provide a facsimile of the
> existing selinux_file_mprotect() and file_map_prot_check() policies,
> but tailored to the unique properties of SGX.
>
> For example, an enclave page is technically backed by a MAP_SHARED file,
> but the "file" is essentially shared memory that is never persisted
> anywhere and also requires execute permissions (for some pages).
>
> The basic concept is to require appropriate execute permissions on the
> source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> an enclave page is being loaded from a regular file, require
> FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> anonymous/private mapping, require PROCESS__EXECMEM since the process
> is essentially executing from the mapping, albeit in a roundabout way.
>
> Note, FILE__READ and FILE__WRITE are intentionally not required even if
> the source page is backed by a regular file. Writes to the enclave page
> are contained to the EPC, i.e. never hit the original file, and read
> permissions have already been vetted (or the VMA doesn't have PROT_READ,
> in which case loading the page into the enclave will fail).
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> security/selinux/hooks.c | 85 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702cf46ca..f436a055dda7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> }
> #endif
>
> +#ifdef CONFIG_INTEL_SGX
> +int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot)
> +{
> + const struct cred *cred = current_cred();
> + u32 sid = cred_sid(cred);
> + int rc;
> +
> + /* SGX is supported only in 64-bit kernels. */
> + WARN_ON_ONCE(!default_noexec);
> +
> + /*
> + * SGX is responsible for checking @prot vs @allowed_prot, and SELinux
> + * only cares about execute related permissions for enclaves.
> + */
> + if (!(*allowed_prot & PROT_EXEC))
> + return 0;
> +
> + /*
> + * Loading an executable enclave page from a VMA that is not executable
> + * itself requires EXECUTE permissions on the source file, or if there
> + * is no regular source file, EXECMEM since the page is being loaded
> + * from a non-executable anonymous mapping.
> + */
> + if (!(vma->vm_flags & VM_EXEC)) {
> + if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> + rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
We might need an EXECMOD check here as well if (vma->vm_file &&
vma->anon_vma). The scenario would be that the host application mapped
the file with PROT_WRITE, modified it, but haven't mapped it PROT_EXEC.
Now the enclave loader requests PROT_EXEC without PROT_WRITE or allows
it. FILE__EXECUTE is insufficient for this case.
> + else
> + rc = avc_has_perm(&selinux_state,
> + sid, sid, SECCLASS_PROCESS,
> + PROCESS__EXECMEM, NULL);
These calls will audit FILE__EXECUTE or PROCESS__EXECMEM denials even
when userspace never asked for PROT_EXEC. Possibly we should use
avc_has_perm_noaudit() and only call avc_audit() if (prot & PROT_EXEC)?
And similarly introduce file_has_perm_noaudit() ->
inode_has_perm_noaudit() -> avc_has_perm_noaudit() or inline here and
switch to avc_has_perm_noaudit() throughout?
> +
> + /*
> + * Reject the load if the enclave *needs* the page to be
> + * executable, otherwise prevent it from becoming executable.
> + */
> + if (rc) {
> + if (prot & PROT_EXEC)
> + return rc;
> +
> + *allowed_prot &= ~PROT_EXEC;
> + }
> + }
> +
> + /*
> + * An enclave page that may do RW->RX or W+X requires EXECMOD (backed
> + * by a regular file) or EXECMEM (loaded from an anonymous mapping).
At present EXECMEM is also triggered for W+X private file mappings, to
allow denying W+X while permitting exceptions for W->X for text relocations.
> + * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
> + * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
> + * a non-persistent shared file, but each enclave is a private domain
> + * within that shared file, so delegate to the source of the enclave.
> + */
> + if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) {
> + if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> + rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> + else
> + rc = avc_has_perm(&selinux_state,
> + sid, sid, SECCLASS_PROCESS,
> + PROCESS__EXECMEM, NULL);
Same issue wrt auditing here. Could also potentially skip the EXECMEM
check this time if we performed it above (if so, then we must have
passed it because *allowed_prot still had PROT_EXEC set).
> + /*
> + * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are
> + * lacking and @prot has neither PROT_WRITE or PROT_EXEC. If
> + * userspace wanted RX they would have requested RX, and due to
> + * lack of permissions they can never get RW->RX, i.e. the only
> + * useful transition is R->RW.
> + */
> + if (rc) {
> + if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> + return rc;
> +
> + if (prot & PROT_EXEC)
> + *allowed_prot &= ~PROT_WRITE;
> + else
> + *allowed_prot &= ~PROT_EXEC;
> + }
> + }
> +
> + return 0;
> +}
> +#endif
> +
> struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_cred = sizeof(struct task_security_struct),
> .lbs_file = sizeof(struct file_security_struct),
> @@ -6968,6 +7049,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
> #endif
> +
> +#ifdef CONFIG_INTEL_SGX
> + LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
> +#endif
> };
>
> static __init int selinux_init(void)
>
^ permalink raw reply
* Re: [RFC PATCH 9/9] security/selinux: Add enclave_load() implementation
From: Sean Christopherson @ 2019-06-03 15:50 UTC (permalink / raw)
To: Stephen Smalley
Cc: Jarkko Sakkinen, Andy Lutomirski, Cedric Xing, 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: <b75cd16f-41a5-0933-3847-02aaa5b2c7d0@tycho.nsa.gov>
On Mon, Jun 03, 2019 at 11:01:32AM -0400, Stephen Smalley wrote:
> On 5/31/19 7:31 PM, Sean Christopherson wrote:
> >The goal of selinux_enclave_load() is to provide a facsimile of the
> >existing selinux_file_mprotect() and file_map_prot_check() policies,
> >but tailored to the unique properties of SGX.
> >
> >For example, an enclave page is technically backed by a MAP_SHARED file,
> >but the "file" is essentially shared memory that is never persisted
> >anywhere and also requires execute permissions (for some pages).
> >
> >The basic concept is to require appropriate execute permissions on the
> >source of the enclave for pages that are requesting PROT_EXEC, e.g. if
> >an enclave page is being loaded from a regular file, require
> >FILE__EXECUTE and/or FILE__EXECMOND, and if it's coming from an
> >anonymous/private mapping, require PROCESS__EXECMEM since the process
> >is essentially executing from the mapping, albeit in a roundabout way.
> >
> >Note, FILE__READ and FILE__WRITE are intentionally not required even if
> >the source page is backed by a regular file. Writes to the enclave page
> >are contained to the EPC, i.e. never hit the original file, and read
> >permissions have already been vetted (or the VMA doesn't have PROT_READ,
> >in which case loading the page into the enclave will fail).
> >
> >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> >---
> > security/selinux/hooks.c | 85 ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 85 insertions(+)
> >
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index 3ec702cf46ca..f436a055dda7 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -6726,6 +6726,87 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> > }
> > #endif
> >+#ifdef CONFIG_INTEL_SGX
> >+int selinux_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> >+ unsigned long *allowed_prot)
> >+{
> >+ const struct cred *cred = current_cred();
> >+ u32 sid = cred_sid(cred);
> >+ int rc;
> >+
> >+ /* SGX is supported only in 64-bit kernels. */
> >+ WARN_ON_ONCE(!default_noexec);
> >+
> >+ /*
> >+ * SGX is responsible for checking @prot vs @allowed_prot, and SELinux
> >+ * only cares about execute related permissions for enclaves.
> >+ */
> >+ if (!(*allowed_prot & PROT_EXEC))
> >+ return 0;
> >+
> >+ /*
> >+ * Loading an executable enclave page from a VMA that is not executable
> >+ * itself requires EXECUTE permissions on the source file, or if there
> >+ * is no regular source file, EXECMEM since the page is being loaded
> >+ * from a non-executable anonymous mapping.
> >+ */
> >+ if (!(vma->vm_flags & VM_EXEC)) {
> >+ if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> >+ rc = file_has_perm(cred, vma->vm_file, FILE__EXECUTE);
>
> We might need an EXECMOD check here as well if (vma->vm_file &&
> vma->anon_vma). The scenario would be that the host application mapped the
> file with PROT_WRITE, modified it, but haven't mapped it PROT_EXEC. Now the
> enclave loader requests PROT_EXEC without PROT_WRITE or allows it.
> FILE__EXECUTE is insufficient for this case.
Ya, agreed.
> >+ else
> >+ rc = avc_has_perm(&selinux_state,
> >+ sid, sid, SECCLASS_PROCESS,
> >+ PROCESS__EXECMEM, NULL);
>
> These calls will audit FILE__EXECUTE or PROCESS__EXECMEM denials even when
> userspace never asked for PROT_EXEC. Possibly we should use
> avc_has_perm_noaudit() and only call avc_audit() if (prot & PROT_EXEC)? And
> similarly introduce file_has_perm_noaudit() -> inode_has_perm_noaudit() ->
> avc_has_perm_noaudit() or inline here and switch to avc_has_perm_noaudit()
> throughout?
Hmm, I think we want to audit the denials even if userspace technically
hasn't requested PROT_EXEC, yet... The idea of @allowed_prot is for
userspace to declare its intent, i.e. for all intents and purposes,
userspace is asking for PROT_EXEC by declaring PROT_EXEC in @allowed_prot.
For SGX1 enclaves, where permissions are fixed at enclave build time,
declaring PROT_EXEC but not actually mapping the page as PROT_EXEC is a
userspace bug, and a fairly egregious one at that.
For SGX2 enclaves, where enclaves can convert RW->RX (among others), not
auditing would make it difficult for userspace to debug failures due to
the denial. E.g. a properly written application that declared PROT_EXEC
on a RW page with the intent of converting it to RX would eventually fail
at mprotect() due to PROT_EXEC being cleared. Without the audit, there
wouldn't be any indication that mprotect() failed with -EACCES due to LSM
restrictions.
> >+
> >+ /*
> >+ * Reject the load if the enclave *needs* the page to be
> >+ * executable, otherwise prevent it from becoming executable.
> >+ */
> >+ if (rc) {
> >+ if (prot & PROT_EXEC)
> >+ return rc;
> >+
> >+ *allowed_prot &= ~PROT_EXEC;
> >+ }
> >+ }
> >+
> >+ /*
> >+ * An enclave page that may do RW->RX or W+X requires EXECMOD (backed
> >+ * by a regular file) or EXECMEM (loaded from an anonymous mapping).
>
> At present EXECMEM is also triggered for W+X private file mappings, to allow
> denying W+X while permitting exceptions for W->X for text relocations.
Doh, missed that one. Thanks!
> >+ * Note, this hybrid EXECMOD and EXECMEM behavior is intentional and
> >+ * reflects the nature of enclaves and the EPC, e.g. EPC is effectively
> >+ * a non-persistent shared file, but each enclave is a private domain
> >+ * within that shared file, so delegate to the source of the enclave.
> >+ */
> >+ if ((*allowed_prot & PROT_EXEC) && (*allowed_prot & PROT_WRITE)) {
> >+ if (vma->vm_file && !IS_PRIVATE(file_inode(vma->vm_file)))
> >+ rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> >+ else
> >+ rc = avc_has_perm(&selinux_state,
> >+ sid, sid, SECCLASS_PROCESS,
> >+ PROCESS__EXECMEM, NULL);
>
> Same issue wrt auditing here. Could also potentially skip the EXECMEM check
> this time if we performed it above (if so, then we must have passed it
> because *allowed_prot still had PROT_EXEC set).
Skipping the second EXECMEM check crossed my mind as well. I'll play with
the code to see if I can come up with clean way of avoiding multiple
EXECMEM (or EXECMOD) checks.
> >+ /*
> >+ * Clear ALLOW_EXEC instead of ALLOWED_WRITE if permissions are
> >+ * lacking and @prot has neither PROT_WRITE or PROT_EXEC. If
> >+ * userspace wanted RX they would have requested RX, and due to
> >+ * lack of permissions they can never get RW->RX, i.e. the only
> >+ * useful transition is R->RW.
> >+ */
> >+ if (rc) {
> >+ if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> >+ return rc;
> >+
> >+ if (prot & PROT_EXEC)
> >+ *allowed_prot &= ~PROT_WRITE;
> >+ else
> >+ *allowed_prot &= ~PROT_EXEC;
> >+ }
> >+ }
> >+
> >+ return 0;
> >+}
> >+#endif
> >+
> > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> > .lbs_cred = sizeof(struct task_security_struct),
> > .lbs_file = sizeof(struct file_security_struct),
> >@@ -6968,6 +7049,10 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> > LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> > LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
> > #endif
> >+
> >+#ifdef CONFIG_INTEL_SGX
> >+ LSM_HOOK_INIT(enclave_load, selinux_enclave_load),
> >+#endif
> > };
> > static __init int selinux_init(void)
> >
>
^ permalink raw reply
* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
From: Paul Moore @ 2019-06-03 15:57 UTC (permalink / raw)
To: Janne Karhunen
Cc: Stephen Smalley, zohar, linux-integrity, linux-security-module
In-Reply-To: <20190531140237.9199-1-janne.karhunen@gmail.com>
On Fri, May 31, 2019 at 10:03 AM Janne Karhunen
<janne.karhunen@gmail.com> wrote:
> Atomic policy updaters are not very useful as they cannot
> usually perform the policy updates on their own. Since it
> seems that there is no strict need for the atomicity,
> switch to the blocking variant.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
> security/security.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 23cbb1a295a3..c5e69ce81521 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
> #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
> struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> static struct kmem_cache *lsm_inode_cache;
> @@ -432,19 +432,19 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>
> int call_lsm_notifier(enum lsm_event event, void *data)
As I mentioned in the other thread, I would like to see "blocking", or
similar, added to the lsm_notifier functions with this change. It
makes it easier if/when we need to add both atomic and blocking
variants, as well as making it much more clear which version is being
used (helpful even now with just one variant).
For example: call_lsm_notifier() -> call_lsm_blocking_notifier(),
register_lsm_notifier() -> register_lsm_blocking_notifier().
> {
> - return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
> + return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
> }
> EXPORT_SYMBOL(call_lsm_notifier);
>
> int register_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
> }
> EXPORT_SYMBOL(register_lsm_notifier);
>
> int unregister_lsm_notifier(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
> + return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
> }
> EXPORT_SYMBOL(unregister_lsm_notifier);
>
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 3/7] vfs: Add a mount-notification facility
From: David Howells @ 2019-06-03 16:30 UTC (permalink / raw)
To: Casey Schaufler
Cc: dhowells, Jann Horn, Al Viro, raven, linux-fsdevel, Linux API,
linux-block, keyrings, linux-security-module, kernel list,
Andy Lutomirski
In-Reply-To: <c95dd6cd-5530-6b70-68f6-4038edd72352@schaufler-ca.com>
Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> should be used. Someone or something caused the event. It can
> >> be important who it was.
> > The kernel's normal security model means that you should be able to
> > e.g. accept FDs that random processes send you and perform
> > read()/write() calls on them without acting as a subject in any
> > security checks; let alone close().
>
> Passed file descriptors are an anomaly in the security model
> that (in this developer's opinion) should have never been
> included. More than one of the "B" level UNIX systems disabled
> them outright.
Considering further on this, I think the only way to implement what you're
suggesting is to add a field to struct file to record the last fputter's creds
as the procedure of fputting is offloaded to a workqueue.
Note that's last fputter, not the last closer, as we don't track the number of
open fds linked to a file struct.
In the case of AF_UNIX sockets that contain in-the-process-of-being-passed fds
at the time of closure, this is further complicated by the socket fput being
achieved in the work item - thereby adding layers of indirection.
It might be possible to replace f_cred rather than adding a new field, but
that might get used somewhere after that point.
Note also that fsnotify_close() doesn't appear to use the last fputter's path
since it's not available if called from deferred fput.
David
^ permalink raw reply
* Re: possible deadlock in process_measurement
From: syzbot @ 2019-06-03 16:35 UTC (permalink / raw)
To: dmitry.kasatkin, jmorris, linux-integrity, linux-kernel,
linux-security-module, serge, syzkaller-bugs, zohar, zohar
In-Reply-To: <0000000000003302870578477029@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 3c09c195 Add linux-next specific files for 20190531
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=10f61a0ea00000
kernel config: https://syzkaller.appspot.com/x/.config?x=6cfb24468280cd5c
dashboard link: https://syzkaller.appspot.com/bug?extid=5ab61747675a87ea359d
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=177c3d16a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ec01baa00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+5ab61747675a87ea359d@syzkaller.appspotmail.com
get_swap_device: Bad swap file entry 6000000000000001
get_swap_device: Bad swap file entry 6400000000000001
get_swap_device: Bad swap file entry 6800000000000001
get_swap_device: Bad swap file entry 6c00000000000001
get_swap_device: Bad swap file entry 7000000000000001
get_swap_device: Bad swap file entry 7400000000000001
======================================================
WARNING: possible circular locking dependency detected
5.2.0-rc2-next-20190531 #4 Not tainted
------------------------------------------------------
syz-executor395/17373 is trying to acquire lock:
000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at: inode_lock
include/linux/fs.h:778 [inline]
000000003d1a4a53 (&sb->s_type->i_mutex_key#10){+.+.}, at:
process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
but task is already holding lock:
00000000e0714fc5 (&mm->mmap_sem#2){++++}, at: do_mprotect_pkey+0x1f6/0xa30
mm/mprotect.c:485
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&mm->mmap_sem#2){++++}:
down_read+0x3f/0x1e0 kernel/locking/rwsem.c:24
get_user_pages_unlocked+0xfc/0x4a0 mm/gup.c:1174
__gup_longterm_unlocked mm/gup.c:2193 [inline]
get_user_pages_fast+0x43f/0x530 mm/gup.c:2245
iov_iter_get_pages+0x2c2/0xf80 lib/iov_iter.c:1287
dio_refill_pages fs/direct-io.c:171 [inline]
dio_get_page fs/direct-io.c:215 [inline]
do_direct_IO fs/direct-io.c:983 [inline]
do_blockdev_direct_IO+0x3f7b/0x8e00 fs/direct-io.c:1336
__blockdev_direct_IO+0xa1/0xca fs/direct-io.c:1422
ext4_direct_IO_write fs/ext4/inode.c:3782 [inline]
ext4_direct_IO+0xaa7/0x1bb0 fs/ext4/inode.c:3909
generic_file_direct_write+0x20a/0x4a0 mm/filemap.c:3110
__generic_file_write_iter+0x2ee/0x630 mm/filemap.c:3293
ext4_file_write_iter+0x332/0x1070 fs/ext4/file.c:266
call_write_iter include/linux/fs.h:1870 [inline]
new_sync_write+0x4d3/0x770 fs/read_write.c:483
__vfs_write+0xe1/0x110 fs/read_write.c:496
vfs_write+0x268/0x5d0 fs/read_write.c:558
ksys_write+0x14f/0x290 fs/read_write.c:611
__do_sys_write fs/read_write.c:623 [inline]
__se_sys_write fs/read_write.c:620 [inline]
__x64_sys_write+0x73/0xb0 fs/read_write.c:620
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #0 (&sb->s_type->i_mutex_key#10){+.+.}:
lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
down_write+0x38/0xa0 kernel/locking/rwsem.c:66
inode_lock include/linux/fs.h:778 [inline]
process_measurement+0x15ae/0x15e0
security/integrity/ima/ima_main.c:228
ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
security_file_mprotect+0xd5/0x100 security/security.c:1430
do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
__do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
__se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
__x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&mm->mmap_sem#2);
lock(&sb->s_type->i_mutex_key#10);
lock(&mm->mmap_sem#2);
lock(&sb->s_type->i_mutex_key#10);
*** DEADLOCK ***
1 lock held by syz-executor395/17373:
#0: 00000000e0714fc5 (&mm->mmap_sem#2){++++}, at:
do_mprotect_pkey+0x1f6/0xa30 mm/mprotect.c:485
stack backtrace:
CPU: 1 PID: 17373 Comm: syz-executor395 Not tainted 5.2.0-rc2-next-20190531
#4
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
print_circular_bug.cold+0x1cc/0x28f kernel/locking/lockdep.c:1566
check_prev_add kernel/locking/lockdep.c:2311 [inline]
check_prevs_add kernel/locking/lockdep.c:2419 [inline]
validate_chain kernel/locking/lockdep.c:2801 [inline]
__lock_acquire+0x3755/0x5490 kernel/locking/lockdep.c:3790
lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4300
down_write+0x38/0xa0 kernel/locking/rwsem.c:66
inode_lock include/linux/fs.h:778 [inline]
process_measurement+0x15ae/0x15e0 security/integrity/ima/ima_main.c:228
ima_file_mmap+0x11a/0x130 security/integrity/ima/ima_main.c:370
security_file_mprotect+0xd5/0x100 security/security.c:1430
do_mprotect_pkey+0x537/0xa30 mm/mprotect.c:550
__do_sys_pkey_mprotect mm/mprotect.c:590 [inline]
__se_sys_pkey_mprotect mm/mprotect.c:587 [inline]
__x64_sys_pkey_mprotect+0x97/0xf0 mm/mprotect.c:587
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440279
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffeec2f48d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000149
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440279
RDX: 000000000000000
^ permalink raw reply
* Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Sean Christopherson @ 2019-06-03 17:15 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: <960B34DE67B9E140824F1DCDEC400C0F654EC5FD@ORSMSX116.amr.corp.intel.com>
On Sun, Jun 02, 2019 at 12:29:35AM -0700, Xing, Cedric wrote:
> Hi Sean,
>
> > From: Christopherson, Sean J
> > Sent: Friday, May 31, 2019 4:32 PM
> >
> > 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.
> >
> > 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}.
> >
> > ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file system or from a
> > file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2 hardware
> > (regardless of kernel support) userspace could EADD from a noexec file using read-only
> > permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.
> >
> > ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD (or EXECMEM).
> >
> > This is very much an RFC series. It's only compile tested, likely has obvious bugs, the
> > SELinux patch could be completely harebrained, etc...
> > My goal at this point is to get feedback at a macro level, e.g. is the core concept
> > viable/acceptable, are there objection to hooking mprotect(), etc...
> >
> > Andy and Cedric, hopefully this aligns with your general expectations based on our last
> > discussion.
>
> I couldn't understand the real intentions of ALLOW_* flags until I saw them
> in code. I have to say C is more expressive than English in that regard :)
>
> 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).
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.
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...
> Open: Given a file of type struct file *, how to tell if it is an enclave (i.e. /dev/sgx/enclave)?
> * In hook security_mmap_file(), if @file is an enclave, make sure @prot can
> only be PROT_NONE. This is to force all protection changes to go through
> security_file_mprotect().
> * In the newly introduced hook security_enclave_load(), set WRITTEN for pages
> that are requested PROT_WRITE.
How would an LSM associate a page with a specific enclave? vma->vm_file
will point always point at /dev/sgx/enclave. vma->vm_mm is useless
because we're allowing multiple processes to map a single enclave, not to
mention that by mm would require holding a reference to the mm.
> * In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up
> and use WRITTEN flags for all pages within @vma, along with other global
> flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide
> on allowing/rejecting @prot.
vma->vm_file will always be /dev/sgx/enclave at this point, which means
LSMs don't have the necessary anchor back to the source file, e.g. to
enforce FILE__EXECUTE. The noexec file system case is also unaddressed.
> * In hook security_file_free(), if @file is an enclave, free storage
> allocated for WRITTEN flags.
^ permalink raw reply
* Re: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Stephen Smalley @ 2019-06-03 17:47 UTC (permalink / raw)
To: Xing, Cedric, Christopherson, Sean J, Jarkko Sakkinen
Cc: Andy Lutomirski, 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: <960B34DE67B9E140824F1DCDEC400C0F654EC5FD@ORSMSX116.amr.corp.intel.com>
On 6/2/19 3:29 AM, Xing, Cedric wrote:
> Hi Sean,
>
>> From: Christopherson, Sean J
>> Sent: Friday, May 31, 2019 4:32 PM
>>
>> 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.
>>
>> 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}.
>>
>> ALLOW_EXEC is used to deny hings like loading an enclave from a noexec file system or from a
>> file without EXECUTE permissions, e.g. without the ALLOW_EXEC concept, on SGX2 hardware
>> (regardless of kernel support) userspace could EADD from a noexec file using read-only
>> permissions, and later use mprotect() and ENCLU[EMODPE] to gain execute permissions.
>>
>> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce SELinux's EXECMOD (or EXECMEM).
>>
>> This is very much an RFC series. It's only compile tested, likely has obvious bugs, the
>> SELinux patch could be completely harebrained, etc...
>> My goal at this point is to get feedback at a macro level, e.g. is the core concept
>> viable/acceptable, are there objection to hooking mprotect(), etc...
>>
>> Andy and Cedric, hopefully this aligns with your general expectations based on our last
>> discussion.
>
> I couldn't understand the real intentions of ALLOW_* flags until I saw them in code. I have to say C is more expressive than English in that regard :)
>
> 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.
>
> 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.
> Open: Given a file of type struct file *, how to tell if it is an enclave (i.e. /dev/sgx/enclave)?
> * In hook security_mmap_file(), if @file is an enclave, make sure @prot can only be PROT_NONE. This is to force all protection changes to go through security_file_mprotect().
> * In the newly introduced hook security_enclave_load(), set WRITTEN for pages that are requested PROT_WRITE.
> * In hook security_file_mprotect(), if @vma->vm_file is an enclave, look up and use WRITTEN flags for all pages within @vma, along with other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux) to decide on
allowing/rejecting @prot.
At this point we have no knowledge of the source vma/file, right? So
what do we check FILE__EXECUTE and/or FILE__EXECMOD against?
vma->vm_file at this point is /dev/sgx/enclave, right?
> * In hook security_file_free(), if @file is an enclave, free storage allocated for WRITTEN flags.
>
> I'll try to make more detailed comments in my replies to individual patches sometime tomorrow.
>
>>
>> Lastly, I added a patch to allow userspace to add multiple pages in a single ioctl(). It's
>> obviously not directly related to the security stuff, but the idea tangentially came up during
>> earlier discussions and it's something I think the UAPI should provide (it's a tiny change).
>> Since I was modifying the UAPI anyways, I threw it in.
>>
>> Sean Christopherson (9):
>> x86/sgx: Remove unused local variable in sgx_encl_release()
>> x86/sgx: Do not naturally align MAP_FIXED address
>> x86/sgx: Allow userspace to add multiple pages in single ioctl()
>> mm: Introduce vm_ops->mprotect()
>> x86/sgx: Restrict mapping without an enclave page to PROT_NONE
>> x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
>> x86/sgx: Enforce noexec filesystem restriction for enclaves
>> LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
>> security/selinux: Add enclave_load() implementation
>>
>> arch/x86/include/uapi/asm/sgx.h | 30 ++++--
>> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 143 +++++++++++++++++--------
>> arch/x86/kernel/cpu/sgx/driver/main.c | 13 ++-
>> arch/x86/kernel/cpu/sgx/encl.c | 31 +++++-
>> arch/x86/kernel/cpu/sgx/encl.h | 4 +
>> include/linux/lsm_hooks.h | 16 +++
>> include/linux/mm.h | 2 +
>> include/linux/security.h | 2 +
>> mm/mprotect.c | 15 ++-
>> security/security.c | 8 ++
>> security/selinux/hooks.c | 85 +++++++++++++++
>> 11 files changed, 290 insertions(+), 59 deletions(-)
>>
>> --
>> 2.21.0
>
> -Cedric
>
^ permalink raw reply
* RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-06-03 18:02 UTC (permalink / raw)
To: Stephen Smalley, Christopherson, Sean J, Jarkko Sakkinen
Cc: Andy Lutomirski, 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: <e5e1dc77-eb56-bdab-8164-602ea544ea6e@tycho.nsa.gov>
> From: linux-sgx-owner@vger.kernel.org [mailto:linux-sgx-
> owner@vger.kernel.org] On Behalf Of Stephen Smalley
> Sent: Monday, June 03, 2019 10:47 AM
>
> On 6/2/19 3:29 AM, Xing, Cedric wrote:
> > Hi Sean,
> >
> >> From: Christopherson, Sean J
> >> Sent: Friday, May 31, 2019 4:32 PM
> >>
> >> 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.
> >>
> >> 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}.
> >>
> >> ALLOW_EXEC is used to deny hings like loading an enclave from a
> >> noexec file system or from a file without EXECUTE permissions, e.g.
> >> without the ALLOW_EXEC concept, on SGX2 hardware (regardless of
> >> kernel support) userspace could EADD from a noexec file using read-
> only permissions, and later use mprotect() and ENCLU[EMODPE] to gain
> execute permissions.
> >>
> >> ALLOW_WRITE is used in conjuction with ALLOW_EXEC to enforce
> SELinux's EXECMOD (or EXECMEM).
> >>
> >> This is very much an RFC series. It's only compile tested, likely
> >> has obvious bugs, the SELinux patch could be completely harebrained,
> etc...
> >> My goal at this point is to get feedback at a macro level, e.g. is
> >> the core concept viable/acceptable, are there objection to hooking
> mprotect(), etc...
> >>
> >> Andy and Cedric, hopefully this aligns with your general expectations
> >> based on our last discussion.
> >
> > I couldn't understand the real intentions of ALLOW_* flags until I saw
> > them in code. I have to say C is more expressive than English in that
> > regard :)
> >
> > 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.
> >
> > 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.
> > Open: Given a file of type struct file *, how to tell if it is an
> enclave (i.e. /dev/sgx/enclave)?
> > * In hook security_mmap_file(), if @file is an enclave, make sure
> @prot can only be PROT_NONE. This is to force all protection changes to
> go through security_file_mprotect().
> > * In the newly introduced hook security_enclave_load(), set WRITTEN
> for pages that are requested PROT_WRITE.
> > * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> > look up and use WRITTEN flags for all pages within @vma, along with
> > other global flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of
> > SELinux) to decide on
> allowing/rejecting @prot.
>
> At this point we have no knowledge of the source vma/file, right? So
> what do we check FILE__EXECUTE and/or FILE__EXECMOD against?
> vma->vm_file at this point is /dev/sgx/enclave, right?
My apology to the confusions here.
Yes, vma->vm_file is always /dev/sgx/enclave, but each open("/dev/sgx/enclave") returns a *new* file struct (let's denote it as @enclave_fd) that uniquely identifies one enclave instance, and the expectation is that @enclave_fd->f_security would be used by LSM to store enclave specific information, including ALLOW_* flags and whatever deemed appropriate by an LSM module.
In the case of SELinux, and if the choice is to use FILE__EXECMOD of .sigstruct file to authorize RW->RX at runtime, then SELinux could cache that flag in @enclave_fd->f_security upon security_enclave_init().
^ permalink raw reply
* RE: [RFC PATCH 0/9] security: x86/sgx: SGX vs. LSM
From: Xing, Cedric @ 2019-06-03 18:30 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: <20190603171549.GE13384@linux.intel.com>
> 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?
Anyway, my point is that it is unnecessary even if it works.
>
> 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.
>
> 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.
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. But spilling them into user mode is what I think is unacceptable.
>
> > Open: Given a file of type struct file *, how to tell if it is an
> enclave (i.e. /dev/sgx/enclave)?
> > * In hook security_mmap_file(), if @file is an enclave, make sure
> @prot can
> > only be PROT_NONE. This is to force all protection changes to go
> through
> > security_file_mprotect().
> > * In the newly introduced hook security_enclave_load(), set WRITTEN
> for pages
> > that are requested PROT_WRITE.
>
> How would an LSM associate a page with a specific enclave? vma->vm_file
> will point always point at /dev/sgx/enclave. vma->vm_mm is useless
> because we're allowing multiple processes to map a single enclave, not
> to mention that by mm would require holding a reference to the mm.
Each open("/dev/sgx/enclave") syscall creates a *new* instance of struct file to uniquely identify one enclave instance. What I mean is @vma->vm_file, not @vma->vm_file->f_path or @vma->vm_file->f_inode.
>
> > * In hook security_file_mprotect(), if @vma->vm_file is an enclave,
> look up
> > and use WRITTEN flags for all pages within @vma, along with other
> global
> > flags (e.g. PROCESS__EXECMEM/FILE__EXECMOD in the case of SELinux)
> to decide
> > on allowing/rejecting @prot.
>
> vma->vm_file will always be /dev/sgx/enclave at this point, which means
> LSMs don't have the necessary anchor back to the source file, e.g. to
> enforce FILE__EXECUTE. The noexec file system case is also unaddressed.
vma->vm_file identifies an enclave instance uniquely. FILE__EXECUTE is checked by security_enclave_load() using @source_vma->vm_file. Once a page has been EADD'ed, whether to allow RW->RX depends on .sigstruct file (more precisely, the file backing SIGSTRUCT), whose FILE__* attributes could be cached in vma->vm_file->f_security by security_enclave_init().
The noexec case should be addressed in IOC_ADD_PAGES by testing @source_vma->vm_flags & VM_MAYEXEC.
>
> > * In hook security_file_free(), if @file is an enclave, free storage
> > allocated for WRITTEN flags.
^ permalink raw reply
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Rob Landley @ 2019-06-03 18:32 UTC (permalink / raw)
To: Roberto Sassu, viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, bug-cpio, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, james.w.mcmechan,
niveditas98
In-Reply-To: <cf9d08ca-74c7-c945-5bf9-7c3495907d1e@huawei.com>
On 6/3/19 4:31 AM, Roberto Sassu wrote:
>> This patch set aims at solving the following use case: appraise files from
>> the initial ram disk. To do that, IMA checks the signature/hash from the
>> security.ima xattr. Unfortunately, this use case cannot be implemented
>> currently, as the CPIO format does not support xattrs.
>>
>> This proposal consists in including file metadata as additional files named
>> METADATA!!!, for each file added to the ram disk. The CPIO parser in the
>> kernel recognizes these special files from the file name, and calls the
>> appropriate parser to add metadata to the previously extracted file. It has
>> been proposed to use bit 17:16 of the file mode as a way to recognize files
>> with metadata, but both the kernel and the cpio tool declare the file mode
>> as unsigned short.
>
> Any opinion on this patch set?
>
> Thanks
>
> Roberto
Sorry, I've had the window open since you posted it but haven't gotten around to
it. I'll try to build it later today.
It does look interesting, and I have no objections to the basic approach. I
should be able to add support to toybox cpio over a weekend once I've got the
kernel doing it to test against.
Rob
^ permalink raw reply
* [PATCH v4] Allow to exclude specific file types in LoadPin
From: Ke Wu @ 2019-06-03 18:36 UTC (permalink / raw)
To: Kees Cook, Jonathan Corbet, James Morris, Serge E. Hallyn
Cc: linux-doc, linux-kernel, linux-security-module, Ke Wu
In-Reply-To: <20190529224350.6460-1-mikewu@google.com>
Linux kernel already provide MODULE_SIG and KEXEC_VERIFY_SIG to
make sure loaded kernel module and kernel image are trusted. This
patch adds a kernel command line option "loadpin.exclude" which
allows to exclude specific file types from LoadPin. This is useful
when people want to use different mechanisms to verify module and
kernel image while still use LoadPin to protect the integrity of
other files kernel loads.
Signed-off-by: Ke Wu <mikewu@google.com>
---
Changelog since v3:
- Undo patch v3 change.
- Use ignore_read_file_id rahther than kernel_read_file_str when
iterating to prevent out-of-bounds write.
Changelog since v2:
- Make size of exclude_read_files and ignore_read_file_id to be
equal to the size of kernel_read_file_str.
Changelog since v1:
- Mark ignore_read_file_id with __ro_after_init.
- Mark parse_exclude() with __init.
- Use ARRAY_SIZE(ignore_read_file_id) instead of READING_MAX_ID.
Documentation/admin-guide/LSM/LoadPin.rst | 10 ++++++
security/loadpin/loadpin.c | 44 +++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/Documentation/admin-guide/LSM/LoadPin.rst b/Documentation/admin-guide/LSM/LoadPin.rst
index 32070762d24c..716ad9b23c9a 100644
--- a/Documentation/admin-guide/LSM/LoadPin.rst
+++ b/Documentation/admin-guide/LSM/LoadPin.rst
@@ -19,3 +19,13 @@ block device backing the filesystem is not read-only, a sysctl is
created to toggle pinning: ``/proc/sys/kernel/loadpin/enabled``. (Having
a mutable filesystem means pinning is mutable too, but having the
sysctl allows for easy testing on systems with a mutable filesystem.)
+
+It's also possible to exclude specific file types from LoadPin using kernel
+command line option "``loadpin.exclude``". By default, all files are
+included, but they can be excluded using kernel command line option such
+as "``loadpin.exclude=kernel-module,kexec-image``". This allows to use
+different mechanisms such as ``CONFIG_MODULE_SIG`` and
+``CONFIG_KEXEC_VERIFY_SIG`` to verify kernel module and kernel image while
+still use LoadPin to protect the integrity of other files kernel loads. The
+full list of valid file types can be found in ``kernel_read_file_str``
+defined in ``include/linux/fs.h``.
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 055fb0a64169..ae59b812f4c8 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -45,6 +45,8 @@ static void report_load(const char *origin, struct file *file, char *operation)
}
static int enforce = IS_ENABLED(CONFIG_SECURITY_LOADPIN_ENFORCE);
+static char *exclude_read_files[READING_MAX_ID];
+static int ignore_read_file_id[READING_MAX_ID] __ro_after_init;
static struct super_block *pinned_root;
static DEFINE_SPINLOCK(pinned_root_spinlock);
@@ -129,6 +131,13 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
struct super_block *load_root;
const char *origin = kernel_read_file_id_str(id);
+ /* If the file id is excluded, ignore the pinning. */
+ if ((unsigned int)id < ARRAY_SIZE(ignore_read_file_id) &&
+ ignore_read_file_id[id]) {
+ report_load(origin, file, "pinning-excluded");
+ return 0;
+ }
+
/* This handles the older init_module API that has a NULL file. */
if (!file) {
if (!enforce) {
@@ -187,10 +196,43 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
};
+static void __init parse_exclude(void)
+{
+ int i, j;
+ char *cur;
+
+ /* Robustness check: size of kernel_read_file_str should be size of
+ * ignore_read_file_id + 1.
+ */
+ BUILD_BUG_ON(ARRAY_SIZE(kernel_read_file_str) <
+ ARRAY_SIZE(ignore_read_file_id));
+
+ for (i = 0; i < ARRAY_SIZE(exclude_read_files); i++) {
+ cur = exclude_read_files[i];
+ if (!cur)
+ break;
+ if (*cur == '\0')
+ continue;
+
+ for (j = 0; j < ARRAY_SIZE(ignore_read_file_id); j++) {
+ if (strcmp(cur, kernel_read_file_str[j]) == 0) {
+ pr_info("excluding: %s\n",
+ kernel_read_file_str[j]);
+ ignore_read_file_id[j] = 1;
+ /*
+ * Can not break, because one read_file_str
+ * may map to more than on read_file_id.
+ */
+ }
+ }
+ }
+}
+
static int __init loadpin_init(void)
{
pr_info("ready to pin (currently %senforcing)\n",
enforce ? "" : "not ");
+ parse_exclude();
security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
return 0;
}
@@ -203,3 +245,5 @@ DEFINE_LSM(loadpin) = {
/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
module_param(enforce, int, 0);
MODULE_PARM_DESC(enforce, "Enforce module/firmware pinning");
+module_param_array_named(exclude, exclude_read_files, charp, NULL, 0);
+MODULE_PARM_DESC(exclude, "Exclude pinning specific read file types");
--
2.22.0.rc1.311.g5d7573a151-goog
^ permalink raw reply related
* Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Stephen Smalley @ 2019-06-03 18:38 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, Andy Lutomirski, Cedric Xing, 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: <20190603144251.GB13384@linux.intel.com>
On 6/3/19 10:42 AM, Sean Christopherson wrote:
> On Mon, Jun 03, 2019 at 10:19:18AM -0400, Stephen Smalley wrote:
>> On 5/31/19 7:31 PM, Sean Christopherson wrote:
>>> enclave_load() is roughly analogous to the existing file_mprotect().
>>>
>>> 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, file_mprotect() does not provide any meaningful
>>> security for enclaves since an LSM can only deny/grant access to the
>>> EPC as a whole.
>>>
>>> security_enclave_load() is called when SGX is first loading an enclave
>>> page, i.e. copying a page from normal memory into the EPC. The notable
>>> difference from file_mprotect() is the allowed_prot parameter, which
>>> is essentially an SGX-specific version of a VMA's MAY_{READ,WRITE,EXEC}
>>> flags. The purpose of allowed_prot is to enable checks such as
>>> SELinux's FILE__EXECMOD permission without having to track and update
>>> VMAs across multiple mm structs, i.e. SGX can ensure userspace doesn't
>>> overstep its bounds simply by restricting an enclave VMA's protections
>>> by vetting what is maximally allowed during build time.
>>>
>>> An alternative to the allowed_prot approach would be to use an enclave's
>>> SIGSTRUCT (a smallish structure that can uniquely identify an enclave)
>>> as a proxy for the enclave. For example, SGX could take and hold a
>>> reference to the file containing the SIGSTRUCT (if it's in a file) and
>>> call security_enclave_load() during mprotect(). While the SIGSTRUCT
>>> approach would provide better precision, the actual value added was
>>> deemed to be negligible. On the other hand, pinning a file for the
>>> lifetime of the enclave is ugly, and essentially caching LSM policies
>>> in each page's allowed_prot avoids having to make an extra LSM upcall
>>> during mprotect().
>>>
>>> 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>
>>> ---
>>> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 14 +++++++++-----
>>> include/linux/lsm_hooks.h | 16 ++++++++++++++++
>>> include/linux/security.h | 2 ++
>>> security/security.c | 8 ++++++++
>>> 4 files changed, 35 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> index 5f71be7cbb01..260417ecbcff 100644
>>> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
>>> @@ -8,6 +8,7 @@
>>> #include <linux/highmem.h>
>>> #include <linux/ratelimit.h>
>>> #include <linux/sched/signal.h>
>>> +#include <linux/security.h>
>>> #include <linux/shmem_fs.h>
>>> #include <linux/slab.h>
>>> #include <linux/suspend.h>
>>> @@ -580,21 +581,24 @@ static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
>>> unsigned long *allowed_prot)
>>> {
>>> struct vm_area_struct *vma;
>>> + int ret = 0;
>>> - if (!(*allowed_prot & VM_EXEC))
>>> + if (!(*allowed_prot & VM_EXEC) && !IS_ENABLED(CONFIG_SECURITY))
>>> goto do_check;
>>> down_read(¤t->mm->mmap_sem);
>>> vma = find_vma(current->mm, src);
>>> if (!vma || (vma->vm_file && path_noexec(&vma->vm_file->f_path)))
>>> *allowed_prot &= ~VM_EXEC;
>>> +#ifdef CONFIG_SECURITY
>>> + ret = security_enclave_load(vma, prot, allowed_prot);
>>> +#endif
>>
>> Normally you'd define a static inline stub for the hook in the #else clause
>> for CONFIG_SECURITY in include/linux/security.h and avoid any ifdef here.
>
> Ah, right.
>
>> What ensures that the mapping referenced by src can't be changed to an
>> entirely different one (with a different vm_file) between the time of check
>> (here) and the time of use?
>
> Nothing. Holding mmap_sem across copy_from_user() would suffice, correct?
I don't believe you can do that; copy_from_user() could stall
indefinitely. Not sure how to do what you want here or if it requires
changing the interface.
>
>>> up_read(¤t->mm->mmap_sem);
>>> do_check:
>>> - if (prot & ~*allowed_prot)
>>> - return -EACCES;
>>> -
>>> - return 0;
>>> + if (!ret && (prot & ~*allowed_prot))
>>> + ret = -EACCES;
>>> + return ret;
>>> }
>>> static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long addr,
^ permalink raw reply
* Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Dave Hansen @ 2019-06-03 18:45 UTC (permalink / raw)
To: Stephen Smalley, Sean Christopherson
Cc: Jarkko Sakkinen, Andy Lutomirski, Cedric Xing, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, 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: <7fa39363-78ec-8bd3-50e3-b5d05cd4593c@tycho.nsa.gov>
...
>>> What ensures that the mapping referenced by src can't be changed
>>> to an entirely different one (with a different vm_file) between
>>> the time of check (here) and the time of use?
>>
>> Nothing. Holding mmap_sem across copy_from_user() would suffice,
>> correct?
>
> I don't believe you can do that; copy_from_user() could stall
> indefinitely. Not sure how to do what you want here or if it requires
> changing the interface.
Holding mmap_sem for *read* is OK since you can handle page faults
underneath it. Holding it for write is not.
But, holding it for read also locks out the writers which might be
messing with vm_file or other parts of the VMA.
Holding it for read for a long time is OK. It's obviously not ideal,
but it is something we do widely today.
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Sean Christopherson @ 2019-06-03 20:08 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: <960B34DE67B9E140824F1DCDEC400C0F654ECBBD@ORSMSX116.amr.corp.intel.com>
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.
> > +
> > + if (need_resched())
> > + cond_resched();
> > +
> > + ret = sgx_encl_add_page(encl, addp->addr + i*PAGE_SIZE,
> > + addp->src + i*PAGE_SIZE,
> > + &secinfo, addp->mrmask);
> > + }
> > return ret;
> > }
> >
> > @@ -823,8 +845,8 @@ long sgx_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
> > case SGX_IOC_ENCLAVE_CREATE:
> > handler = sgx_ioc_enclave_create;
> > break;
> > - case SGX_IOC_ENCLAVE_ADD_PAGE:
> > - handler = sgx_ioc_enclave_add_page;
> > + case SGX_IOC_ENCLAVE_ADD_PAGES:
> > + handler = sgx_ioc_enclave_add_pages;
> > break;
> > case SGX_IOC_ENCLAVE_INIT:
> > handler = sgx_ioc_enclave_init;
> > --
> > 2.21.0
>
^ permalink raw reply
* Re: [PATCH 04/58] LSM: Create an lsm_export data structure.
From: Casey Schaufler @ 2019-06-03 20:07 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds
In-Reply-To: <201906010822.03972E152B@keescook>
On 6/1/2019 8:23 AM, Kees Cook wrote:
> On Fri, May 31, 2019 at 04:09:26PM -0700, Casey Schaufler wrote:
>> When more than one security module is exporting data to
>> audit and networking sub-systems a single 32 bit integer
>> is no longer sufficient to represent the data. Add a
>> structure to be used instead.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> Thoughts below...
>
>> ---
>> include/linux/security.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 49f2685324b0..81f9f79f9a1e 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -76,6 +76,18 @@ enum lsm_event {
>> LSM_POLICY_CHANGE,
>> };
>>
>> +/* Data exported by the security modules */
>> +struct lsm_export {
>> + u32 selinux;
>> + u32 smack;
>> + u32 apparmor;
>> + u32 flags;
>> +};
> I think it might make more logical sent to leave flags at the start.
> While I don't expect to add more fields, I find it surprising that
> it is at the end. :)
Data structure randomization notwithstanding, it
doesn't matter to me, so I'll put the flags first.
This came about because I initially didn't think I'd
need flags, and they were add to the end when I determined
I did.
>
> -Kees
>
>> +#define LSM_EXPORT_NONE 0x00
>> +#define LSM_EXPORT_SELINUX 0x01
>> +#define LSM_EXPORT_SMACK 0x02
>> +#define LSM_EXPORT_APPARMOR 0x04
>> +
>> /* These functions are in security/commoncap.c */
>> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>> int cap, unsigned int opts);
>> --
>> 2.19.1
>>
^ permalink raw reply
* Re:[RFC 1/1] Add dm verity root hash pkcs7 sig validation.
From: jaskarankhurana @ 2019-06-03 20:08 UTC (permalink / raw)
To: linux-kernel, linux-security-module, gmazyland
Cc: agk, snitzer, dm-deval, jmorris, scottsh
In-Reply-To: <MN2PR21MB1200327819F4810E82AA6FF38A000@MN2PR21MB1200.namprd21.prod.outlook.com>
On Tue, 21 May 2019, Milan Broz wrote:
> On 20/05/2019 23:54, Jaskaran Khurana wrote:
>> Adds in-kernel pkcs7 signature checking for the roothash of
>> the dm-verity hash tree.
>>
>>
>> Adds DM_VERITY_VERIFY_ROOTHASH_SIG_FORCE: roothash signature *must* be
>> specified for all dm verity volumes and verification must succeed prior
>> to creation of device mapper block device.
>
> I am not sure this is a good idea. If I understand it correctly, this will
> block creating another dm-verity mappings without PKCS7 signature, and these
> are used in many other environments and applications that could possibly
> run on that system later.
>
> (But I have no idea how to solve it better though :-)
>
> ...
>
>> + /* Root hash signature is a optional parameter*/
>> + r = verity_verify_root_hash(root_hash_digest_to_validate,
>> + strlen(root_hash_digest_to_validate),
>> + verify_args.sig,
>> + verify_args.sig_size);
>> + if (r < 0) {
>> + ti->error = "Root hash verification failed";
>> + goto bad;
>> + }
>
> You are sending the PKCS7 signature as a (quite large) binary blob inside the mapping table.
>
> I am not sure if it is possible here (I guess so), but why not put this it kernel keyring
> and then just reference it from mapping table?
> (We use kernel keyring in libcryptsetup already for dm-crypt.)
>
> It will also solve an issue in userspace patch, when you are reading the signature
> file too late (devices can be suspended in that moment, so I would prefer to download
> sig file to keyring in advance, and then just reference it in mapping table).
>
> (I guess you will send merge request for veritysetup userspace part later.)
I have made the changes for passing the signature bytes using the keyring
and I had sent an updated patch for the same last week. I have given a
link to the veritysetup changes which I used to test this in the patch and I will
cleanup and send those for review next.
Please take a look and provide code review feedback for the kernel
changes.
>
> Milan
>
Regards,
Jaskaran
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Dave Hansen @ 2019-06-03 20:14 UTC (permalink / raw)
To: Sean Christopherson, Jarkko Sakkinen
Cc: Andy Lutomirski, Cedric Xing, Stephen Smalley, James Morris,
Serge E . Hallyn, LSM List, Paul Moore, Eric Paris, selinux,
Jethro Beekman, 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 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.
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? ;)
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-06-03 21:37 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, 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: <20190603205405.GE4894@linux.intel.com>
On Mon, Jun 3, 2019 at 1:54 PM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote:
> > > What is the "source file" i.e. the target of the check? Enclave file,
> > > sigstruct file, or /dev/sgx/enclave?
> >
> > Enclave file -- that is, the file backing the vma from which the data
> > is loaded.
>
> Wonder why KVM gets away without having this given that enclaves are
> lot alike VMs.
>
I would argue it's because access to /dev/kvm means you can execute
whatever you code you want in a VM. I don't see how this is
avoidable. On the other hand, it would be nice for SGX to not imply
this same sort of "execute anything" right, especially since, unlike
KVM, SGX is not a sandbox.
^ permalink raw reply
* Re: [PATCH 25/58] IMA: Clean out lsm_export scaffolding
From: Casey Schaufler @ 2019-06-03 21:40 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906011905.75B11DC86@keescook>
On 6/1/2019 7:06 PM, Kees Cook wrote:
> On Fri, May 31, 2019 at 04:09:47PM -0700, Casey Schaufler wrote:
>> +++ b/security/integrity/ima/ima_api.c
>> @@ -159,7 +159,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>> * ima_get_action - appraise & measure decision based on policy.
>> * @inode: pointer to inode to measure
>> * @cred: pointer to credentials structure to validate
>> - * @secid: secid of the task being validated
>> + * @l: LAM data of the task being validated
>> * @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
>> * MAY_APPEND)
>> * @func: caller identifier
> Call this "l" just hurts me. Why shouldn't it still be secid?
Changing the type while leaving the name alone, or
changed slightly (e.g. secids instead of secid) makes
tracking down unchanged uses much harder. How about
lsme, or export?
> Also typo: LAM -> LSM.
That too.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-06-03 21:23 UTC (permalink / raw)
To: Jarkko Sakkinen
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: <20190603205405.GE4894@linux.intel.com>
On Mon, Jun 03, 2019 at 11:54:05PM +0300, Jarkko Sakkinen wrote:
> On Thu, May 30, 2019 at 09:14:10AM -0700, Andy Lutomirski wrote:
> > > What is the "source file" i.e. the target of the check? Enclave file,
> > > sigstruct file, or /dev/sgx/enclave?
> >
> > Enclave file -- that is, the file backing the vma from which the data
> > is loaded.
>
> Wonder why KVM gets away without having this given that enclaves are
> lot alike VMs.
From a memory management perspective, VMs are not at all like enclaves.
An enclave is an extension of its host, i.e. runs in the same address.
This isn't strictly necessary, e.g. an enclave could run in a sandbox
process, but even then the enclave will be running with the kernel's
standard page tables.
A VM is a essentially an opaque blob of data that gets loaded into memory.
KVM builds a completely different set of page tables for the VM, the VM
has it's own file system (or perhaps doesn't have a file system at all),
etc... Ignoring Spectre and L1TF, the VM is contained to its own world.
There are a lot of ways for a userspace VMM to expose things beyond raw
memory, but doing so requires the appropriate permissions.
And practically speaking, all traditional VMs will effectively need RWX
memory, i.e. Qemu (or any other userspace VMM) would be required to have
EXECMEM permissions, which would be a net negative for security.
> > It's provided by userspace based on whether it thinks the data in
> > question is enclave code. source->vm_file is the file from which the
> > code is being loaded. I'm assuming that the user code will only set
> > excute_intent ==true if it actually wants to execute the code, so, if
> > there's a denial, it will be fatal. The normal case will be that the
> > request will be granted on the basis of EXECUTE.
>
> AFAIK user spaces tells that already with the SECINFO flags. I don't
> get why we need a duplicate parameter.
Please read through the RFC, I think it address a lot of your questions.
Hopefully that will help us avoid some thrash.
^ permalink raw reply
* Re: [PATCH 46/58] LSM: Use lsm_context in release_secctx hooks
From: Casey Schaufler @ 2019-06-03 21:57 UTC (permalink / raw)
To: Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
john.johansen, penguin-kernel, paul, sds, casey
In-Reply-To: <201906011920.C5CC2777@keescook>
On 6/1/2019 7:27 PM, Kees Cook wrote:
> On Fri, May 31, 2019 at 04:10:08PM -0700, Casey Schaufler wrote:
>> -void apparmor_release_secctx(char *secdata, u32 seclen)
>> +void apparmor_release_secctx(struct lsm_context *cp)
>> {
>> - kfree(secdata);
>> + kfree(cp->context);
>> }
> Maybe better to have common helper?
>
> void lsm_context_clear(struct lsm_context *cp)
> {
> kfree(cp->context);
> cp->context = NULL;
> cp->len = 0;
> }
The caller, security_release_secctx(), does a
lsm_context_init after the module specific code.
The helper wouldn't be generic, since Smack does
not do a kfree() on cp->context.
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Sean Christopherson @ 2019-06-03 20:37 UTC (permalink / raw)
To: Dave Hansen
Cc: Jarkko Sakkinen, Andy Lutomirski, Cedric Xing, Stephen Smalley,
James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux, Jethro Beekman, 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: <beb8ac7a-b580-8ff2-7467-fb2870fb8cf0@intel.com>
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;
__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-03 20:39 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: <20190603200804.GG13384@linux.intel.com>
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.
^ permalink raw reply
* Re: [RFC PATCH 3/9] x86/sgx: Allow userspace to add multiple pages in single ioctl()
From: Dave Hansen @ 2019-06-03 20:39 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, Andy Lutomirski, Cedric Xing, Stephen Smalley,
James Morris, Serge E . Hallyn, LSM List, Paul Moore, Eric Paris,
selinux, Jethro Beekman, 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: <20190603203712.GI13384@linux.intel.com>
On 6/3/19 1:37 PM, Sean Christopherson wrote:
> 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;
> }
Looks fine to me.
^ 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