* 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 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Sean Christopherson @ 2019-06-03 14:42 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: <d9ef6314-de91-ddcb-3d18-8155cd37e7be@tycho.nsa.gov>
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?
> > 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: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: James Bottomley @ 2019-06-03 14:31 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: <3667fbd4-b6ed-6a76-9ff4-84ec3c2dda12@huawei.com>
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.
James
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-06-03 14:29 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: <1559569401.5052.17.camel@HansenPartnership.com>
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?
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Chuck Lever @ 2019-06-03 14:24 UTC (permalink / raw)
To: James Bottomley, Roberto Sassu
Cc: Mimi Zohar, dmitry.kasatkin, Matthew Garrett, linux-integrity,
linux-security-module, linux-doc, Linux Kernel Mailing List,
silviu.vlasceanu, stable
In-Reply-To: <1559569401.5052.17.camel@HansenPartnership.com>
> On Jun 3, 2019, at 9:43 AM, James Bottomley <James.Bottomley@HansenPartnership.com> 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.
>
> So I can see your use case requires IMA+EVM, but requiring it would
> cause more complexity for my use case.
... and would not work at all for the current NFS IMA implementation.
--
Chuck Lever
^ permalink raw reply
* Re: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Stephen Smalley @ 2019-06-03 14:19 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-9-sean.j.christopherson@intel.com>
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.
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?
> 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,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 47f58cfb6a19..0562775424a0 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1446,6 +1446,14 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * Security hooks for Intel SGX enclaves.
> + *
> + * @enclave_load:
> + * On success, returns 0 and optionally adjusts @allowed_prot
> + * @vma: the source memory region of the enclave page being loaded.
> + * @prot: the initial protection of the enclave page.
> + * @allowed_prot: the maximum protections of the enclave page.
> + * Return 0 if permission is granted.
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1807,6 +1815,11 @@ union security_list_options {
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> + int (*enclave_load)(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
> +#endif /* CONFIG_INTEL_SGX */
> };
>
> struct security_hook_heads {
> @@ -2046,6 +2059,9 @@ struct security_hook_heads {
> struct hlist_head bpf_prog_alloc_security;
> struct hlist_head bpf_prog_free_security;
> #endif /* CONFIG_BPF_SYSCALL */
> +#ifdef CONFIG_INTEL_SGX
> + struct hlist_head enclave_load;
> +#endif /* CONFIG_INTEL_SGX */
> } __randomize_layout;
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..07ed6763571e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2359,3 +2359,11 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
> call_void_hook(bpf_prog_free_security, aux);
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot)
> +{
> + return call_int_hook(enclave_load, 0, vma, prot, allowed_prot);
> +}
> +#endif /* CONFIG_INTEL_SGX */
>
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: James Bottomley @ 2019-06-03 13:43 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: <e6b31aa9-0319-1805-bdfc-3ddde5884494@huawei.com>
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.
So I can see your use case requires IMA+EVM, but requiring it would
cause more complexity for my use case.
James
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Mimi Zohar @ 2019-06-03 12:48 UTC (permalink / raw)
To: Roberto Sassu, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <e6b31aa9-0319-1805-bdfc-3ddde5884494@huawei.com>
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 status
> >
> > 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 don't disagree with you that people should be using EVM to protect
IMA hashes. If you claim this is a bug in the design from the very
beginning, then there needs some explanation as to why it was
upstreamed as it was. My review of this patch provided that
context/background.
Mimi
>
>
> >> This patch mitigates the issue by selecting signature verification as the
> >> only method allowed for appraisal when EVM is not initialized. Since the
> >> new behavior might break user space, it must be turned on by adding the
> >> '-evm' suffix to the value of the ima_appraise= kernel option.
> >>
> >> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> >> Cc: stable@vger.kernel.org
>
^ permalink raw reply
* Re: [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
From: Roberto Sassu @ 2019-06-03 9:31 UTC (permalink / raw)
To: 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, rob,
james.w.mcmechan, niveditas98
In-Reply-To: <20190523121803.21638-1-roberto.sassu@huawei.com>
Any opinion on this patch set?
Thanks
Roberto
On 5/23/2019 2:18 PM, 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.
>
> The difference from v2, v3 (https://lkml.org/lkml/2019/5/9/230,
> https://lkml.org/lkml/2019/5/17/466) is that file metadata are stored in
> separate files instead of a single file. Given that files with metadata
> must immediately follow the files metadata will be added to, image
> generators have to be modified in this version.
>
> The difference from v1 (https://lkml.org/lkml/2018/11/22/1182) is that
> all files have the same name. The file metadata are added to is always the
> previous one, and the image generator in user space will make sure that
> files are in the correct sequence.
>
> The difference with another proposal
> (https://lore.kernel.org/patchwork/cover/888071/) is that xattrs can be
> included in an image without changing the image format. Files with metadata
> will appear as regular files. It will be task of the parser in the kernel
> to process them.
>
> This patch set extends the format of data defined in patch 9/15 of the last
> proposal. It adds header version and type, so that new formats can be
> defined and arbitrary metadata types can be processed.
>
> The changes introduced by this patch set don't cause any compatibility
> issue: kernels without the metadata parser simply extract the special files
> and don't process metadata; kernels with the metadata parser don't process
> metadata if the special files are not included in the image.
>
> From the kernel space perspective, backporting this functionality to older
> kernels should be very easy. It is sufficient to add two calls to the new
> function do_process_metadata() in do_copy(), and to check the file name in
> do_name(). From the user space perspective, unlike the previous version of
> the patch set, it is required to modify the image generators in order to
> include metadata as separate files.
>
> Changelog
>
> v3:
> - include file metadata as separate files named METADATA!!!
> - add the possibility to include in the ram disk arbitrary metadata types
>
> v2:
> - replace ksys_lsetxattr() with kern_path() and vfs_setxattr()
> (suggested by Jann Horn)
> - replace ksys_open()/ksys_read()/ksys_close() with
> filp_open()/kernel_read()/fput()
> (suggested by Jann Horn)
> - use path variable instead of name_buf in do_readxattrs()
> - set last byte of str to 0 in do_readxattrs()
> - call do_readxattrs() in do_name() before replacing an existing
> .xattr-list
> - pass pathname to do_setxattrs()
>
> v1:
> - move xattr unmarshaling to CPIO parser
>
>
> Mimi Zohar (1):
> initramfs: add file metadata
>
> Roberto Sassu (2):
> initramfs: read metadata from special file METADATA!!!
> gen_init_cpio: add support for file metadata
>
> include/linux/initramfs.h | 21 ++++++
> init/initramfs.c | 137 +++++++++++++++++++++++++++++++++++++-
> usr/Kconfig | 8 +++
> usr/Makefile | 4 +-
> usr/gen_init_cpio.c | 137 ++++++++++++++++++++++++++++++++++++--
> usr/gen_initramfs_list.sh | 10 ++-
> 6 files changed, 305 insertions(+), 12 deletions(-)
> create mode 100644 include/linux/initramfs.h
>
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-06-03 9:25 UTC (permalink / raw)
To: Mimi Zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <1559217621.4008.7.camel@linux.ibm.com>
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.
>> This patch mitigates the issue by selecting signature verification as the
>> only method allowed for appraisal when EVM is not initialized. Since the
>> new behavior might break user space, it must be turned on by adding the
>> '-evm' suffix to the value of the ima_appraise= kernel option.
>>
>> Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Cc: stable@vger.kernel.org
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 3 ++-
>> security/integrity/ima/ima_appraise.c | 8 ++++++++
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 138f6664b2e2..d84a2e612b93 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1585,7 +1585,8 @@
>> Set number of hash buckets for inode cache.
>>
>> ima_appraise= [IMA] appraise integrity measurements
>> - Format: { "off" | "enforce" | "fix" | "log" }
>> + Format: { "off" | "enforce" | "fix" | "log" |
>> + "enforce-evm" | "log-evm" }
>
> Is it necessary to define both "enforce-evm" and "log-evm"? Perhaps
> defining "require-evm" is sufficient.
ima_appraise= accepts as values modes of operation. I consider the -evm
suffix as a modifier of already defined modes.
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: [PATCH v5 2/3] mm: init: report memory auto-initialization features at boot time
From: Alexander Potapenko @ 2019-06-03 9:24 UTC (permalink / raw)
To: Andrew Morton, Kees Cook
Cc: 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: <20190531181832.e7c3888870ce9e50db9f69e6@linux-foundation.org>
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?
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Dr. Greg @ 2019-06-03 9:12 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, Stephen Smalley, Xing, Cedric, William Roberts,
Jarkko Sakkinen, 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
In-Reply-To: <20190530213601.GC27551@linux.intel.com>
On Thu, May 30, 2019 at 02:36:01PM -0700, Sean Christopherson wrote:
Good morning, I hope everyone had a pleasant weekend.
> Assuming MRENCLAVE generated by Graphene or any other hosting scheme
> are stable[1], then avoiding EXEC<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?
The cryptographic identity of the entity that signed the enclave and
generated the SIGSTRUCT.
At the risk of being the monotone in the choir, any relevant SGX
security controls require verifying the identity of whoever signed the
identity characteristics (SIGSTRUCT) of the image that initiates the
execution of an SGX TEE. Other then verifying the initial execution
image, the MRENCLAVE value isn't all that relevant.
This issue is further evidenced by the fact that sealing data to an
enclave uses the MRSIGNER variant of ENCLU[EGETKEY] key derivation.
The current work on LSM controls seems to focus on the identity of the
entity that is requesting the image to be loaded rather then who
actually signed, and presumably authored, the code. As I have
previously noted, with SGX2/EDMM, a platform owner may not even have
any visibility into the code that an SGX TEE may ultimately load and
execute.
Any security relevant LSM control in this space has to focus on
providing the platform owner the ability to take action based on the
contents of the SIGSTRUCT of the initiating image. In addition to the
identity of who is requesting the image to be loaded.
Have a good week.
Dr. Greg
As always,
Dr. G.W. Wettstein, Ph.D. Enjellic Systems Development, LLC.
4206 N. 19th Ave. Specializing in information infra-structure
Fargo, ND 58102 development.
PH: 701-281-1686
FAX: 701-281-3949 EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"Experience is something you don't get until just after you need it."
-- Olivier
^ permalink raw reply
* Re: [PATCH 2/2] ima: use the lsm policy update notifier
From: Janne Karhunen @ 2019-06-03 6:58 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Paul Moore, linux-integrity, linux-security-module
In-Reply-To: <7a0f17c6-5332-fd48-2727-1f1e2649d356@tycho.nsa.gov>
On Fri, May 31, 2019 at 9:35 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > + entry->lsm[i].rule = rule_new;
>
> Doesn't this still leak the old entry->lsm[i].rule?
Argh, clearly got a wrong understanding from different part of the
code. Will fix.
> Also, I don't think you can just mutate entry like this under RCU.
Yeah, it's definitely not the politically correct way of doing it.
Let's rework the entire list then, I will post another draft. It will
become somewhat more intrusive :-(
--
Janne
^ permalink raw reply
* RE: [RFC PATCH 7/9] x86/sgx: Enforce noexec filesystem restriction for enclaves
From: Xing, Cedric @ 2019-06-03 6:29 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-8-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> Do not allow an enclave page to be mapped with PROT_EXEC if the source page is backed by a
> file on a noexec file system.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kernel/cpu/sgx/driver/ioctl.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> index c30acd3fbbdd..5f71be7cbb01 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/ioctl.c
> @@ -576,6 +576,27 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, unsigned long
> addr,
> return ret;
> }
>
> +static int sgx_encl_page_protect(unsigned long src, unsigned long prot,
> + unsigned long *allowed_prot)
> +{
> + struct vm_area_struct *vma;
> +
> + if (!(*allowed_prot & VM_EXEC))
> + 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;
Testing (vma->vm_flags & VM_MAYEXEC) == 0 should be a better approach.
Moreover, it looks like the check is done per page, so say 100 pages would cause this test to run 100 times even if they are within the same VMA. Wouldn't that be a bit inefficient?
> + up_read(¤t->mm->mmap_sem);
> +
> +do_check:
> + if (prot & ~*allowed_prot)
> + return -EACCES;
> +
> + return 0;
> +}
^ permalink raw reply
* RE: [RFC PATCH 6/9] x86/sgx: Require userspace to provide allowed prots to ADD_PAGES
From: Xing, Cedric @ 2019-06-03 6:28 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-7-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> ...to support (the equivalent) of existing Linux Security Module functionality.
>
> Because SGX manually manages EPC memory, all enclave VMAs are backed by the same vm_file,
> i.e. /dev/sgx/enclave, so that SGX can implement the necessary hooks to move pages in/out
> of the EPC. And because EPC pages for any given enclave are fundamentally shared between
> processes, i.e.
> CoW semantics are not possible with EPC pages, /dev/sgx/enclave must always be MAP_SHARED.
> Lastly, all real world enclaves will need read, write and execute permissions to EPC pages.
> As a result, SGX does not play nice with existing LSM behavior as it is impossible to
> apply policies to enclaves with any reasonable granularity, e.g. an LSM can deny access to
> EPC altogether, but can't deny potentially dangerous behavior such as mapping pages RW->RW
> or RWX.
>
> To give LSMs enough information to implement their policies without having to resort to
> ugly things, e.g. holding a reference to the vm_file of each enclave page, require
> userspace to explicitly state the allowed protections for each page (region), i.e. take
> ALLOW_{READ,WRITE,EXEC} in the ADD_PAGES ioctl.
>
> The ALLOW_* flags will be passed to LSMs so that they can make informed decisions when the
> enclave is being built, i.e. when the source vm_file is available. For example, SELinux's
> EXECMOD permission can be required if an enclave is requesting both ALLOW_WRITE and
> ALLOW_EXEC.
>
> Update the mmap()/mprotect() hooks to enforce the ALLOW_* protections, a la the standard
> VM_MAY{READ,WRITE,EXEC} flags.
>
> The ALLOW_EXEC flag also has a second (important) use in that it can be used to prevent
> loading an enclave from a noexec file system, on
> SGX2 hardware (regardless of kernel support for SGX2), userspace could EADD from a noexec
> path using read-only permissions and later mprotect() and ENCLU[EMODPE] the page to gain
> execute permissions. By requiring ALLOW_EXEC up front, SGX will be able to enforce noexec
> paths when building the enclave.
ALLOW_* flags shall be kept internal to LSM.
This patch is completely unnecessary.
^ permalink raw reply
* RE: [RFC PATCH 5/9] x86/sgx: Restrict mapping without an enclave page to PROT_NONE
From: Xing, Cedric @ 2019-06-03 6:28 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-6-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> To support LSM integration, SGX will require userspace to explicitly specify the allowed
> protections for each page. The allowed protections will be supplied to and modified by
> LSMs (based on their policies).
> To prevent userspace from circumventing the allowed protections, do not allow
> PROT_{READ,WRITE,EXEC} mappings to an enclave without an associated enclave page (which
> will track the allowed protections).
This is unnecessary.
For mprotect(), LSM shall validate @prot against existing pages with applicable global flags (e.g. FILE__EXECMOD/PROCESS__EXECMEM in the case of SELinux).
For mmap(), SGX driver could invoke security_file_mprotect() explicitly to have LSM validate requested protection.
In the case where there's no page associated with an VMA, security_file_mprotect() shall still dictate whether to allow/deny the request. LSM internally is able to track existence/nonexistence of enclave pages. If there's no page, there's no conflict so the decision shall only depend on global flags (if any). Afterwards, #PF may trigger SGX driver to EAUG, in which case security_enclave_load() will be invoked and LSM could decide whether to approve/decline EAUG request.
^ permalink raw reply
* RE: [RFC PATCH 8/9] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Xing, Cedric @ 2019-06-03 6:28 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-9-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> diff --git a/include/linux/security.h b/include/linux/security.h index
> 659071c2e57c..2f7925eeef7e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -392,6 +392,8 @@ void security_inode_invalidate_secctx(struct inode *inode); int
> security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int
> security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); int
> security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + unsigned long *allowed_prot);
Per my comments to the cover letter, security_enclave_load() should have a signature similar to the following:
int security_enclave_load(struct file *enclave_fd, unsigned long linear_address, unsigned long nr_pages, int prot, struct vm_area_struct *source_vma);
@enclave_fd identifies the enclave to which new pages are being added.
@linear_address/@nr_pages specifies the linear range of pages being added.
@prot specifies the initial protection of those newly added pages. It is taken from the vma covering the target range.
@source_vma covers the source pages in the case of EADD. An LSM is expected to make sure security_file_mprotect(source_vma, prot, prot) would succeed before checking anything else, unless @source_vma is NULL, indicating pages are being EAUG'ed. In all cases, LSM is expected to "remember" @prot for all those pages to be checked in future security_file_mprotect() invocations.
> #else /* CONFIG_SECURITY */
^ permalink raw reply
* RE: [RFC PATCH 4/9] mm: Introduce vm_ops->mprotect()
From: Xing, Cedric @ 2019-06-03 6:27 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-5-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Friday, May 31, 2019 4:32 PM
>
> 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);
As I commented in my reply to the cover letter, SGX driver doesn't need to intercept mprotect() if ALLOW_* flags are not spilled into it.
> vm_fault_t (*fault)(struct vm_fault *vmf);
> vm_fault_t (*huge_fault)(struct vm_fault *vmf,
> enum page_entry_size pe_size);
^ 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 6:26 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: 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: <20190531233159.30992-4-sean.j.christopherson@intel.com>
> 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?
> +
> + 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
* [PATCH 58/58] AppArmor: Remove the exclusive flag
From: Casey Schaufler @ 2019-06-02 16:51 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
With the inclusion of the "display" process attribute
mechanism AppArmor no longer needs to be treated as an
"exclusive" security module. Remove the flag that indicates
it is exclusive.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/apparmor/lsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 771b0ae24a5f..a8b11a7f29fa 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1705,7 +1705,7 @@ static int __init apparmor_init(void)
DEFINE_LSM(apparmor) = {
.name = "apparmor",
- .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
+ .flags = LSM_FLAG_LEGACY_MAJOR,
.enabled = &apparmor_enabled,
.blobs = &apparmor_blob_sizes,
.init = apparmor_init,
--
2.19.1
^ permalink raw reply related
* [PATCH 57/58] LSM: Specify which LSM to display
From: Casey Schaufler @ 2019-06-02 16:51 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
Create a new entry "display" in /proc/.../attr for controlling
which LSM security information is displayed for a process.
The name of an active LSM that supplies hooks for human readable
data may be written to "display" to set the value. The name of
the LSM currently in use can be read from "display".
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
fs/proc/base.c | 1 +
security/security.c | 110 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 109 insertions(+), 2 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ddef482f1334..7bf70e041315 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
ATTR(NULL, "fscreate", 0666),
ATTR(NULL, "keycreate", 0666),
ATTR(NULL, "sockcreate", 0666),
+ ATTR(NULL, "display", 0666),
#ifdef CONFIG_SECURITY_SMACK
DIR("smack", 0555,
proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/security/security.c b/security/security.c
index b05265ec24f0..4af99077572d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -46,7 +46,9 @@ static struct kmem_cache *lsm_file_cache;
static struct kmem_cache *lsm_inode_cache;
char *lsm_names;
-static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
+static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
+ .lbs_task = sizeof(struct lsm_one_hooks),
+};
/* Boot-time LSM user choice */
static __initdata const char *chosen_lsm_order;
@@ -577,6 +579,7 @@ static int lsm_task_alloc(struct task_struct *task)
task->security = kzalloc(blob_sizes.lbs_task, GFP_KERNEL);
if (task->security == NULL)
return -ENOMEM;
+
return 0;
}
@@ -736,7 +739,10 @@ int lsm_superblock_alloc(struct super_block *sb)
#define call_one_int_hook(FUNC, IRC, ...) ({ \
int RC = IRC; \
- if (lsm_base_one.FUNC.FUNC) \
+ struct lsm_one_hooks *LOH = current->security; \
+ if (LOH->FUNC.FUNC) \
+ RC = LOH->FUNC.FUNC(__VA_ARGS__); \
+ else if (LOH->lsm == NULL && lsm_base_one.FUNC.FUNC) \
RC = lsm_base_one.FUNC.FUNC(__VA_ARGS__); \
RC; \
})
@@ -1569,13 +1575,22 @@ int security_file_open(struct file *file)
int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
{
+ struct lsm_one_hooks *odisplay = current->security;
+ struct lsm_one_hooks *ndisplay;
int rc = lsm_task_alloc(task);
if (rc)
return rc;
+
rc = call_int_hook(task_alloc, 0, task, clone_flags);
if (unlikely(rc))
security_task_free(task);
+ else if (odisplay) {
+ ndisplay = task->security;
+ if (ndisplay)
+ *ndisplay = *odisplay;
+ }
+
return rc;
}
@@ -1945,10 +1960,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
char **value)
{
struct security_hook_list *hp;
+ struct lsm_one_hooks *loh = current->security;
+ char *s;
+
+ if (!strcmp(name, "display")) {
+ if (loh->lsm)
+ s = loh->lsm;
+ else if (lsm_base_one.lsm)
+ s = lsm_base_one.lsm;
+ else
+ return -EINVAL;
+
+ *value = kstrdup(s, GFP_KERNEL);
+ if (*value)
+ return strlen(s);
+ return -ENOMEM;
+ }
hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
if (lsm != NULL && strcmp(lsm, hp->lsm))
continue;
+ if (lsm == NULL && loh->lsm && strcmp(loh->lsm, hp->lsm))
+ continue;
return hp->hook.getprocattr(p, name, value);
}
return -EINVAL;
@@ -1958,10 +1991,83 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
size_t size)
{
struct security_hook_list *hp;
+ struct lsm_one_hooks *loh = current->security;
+ bool found = false;
+ char *s;
+
+ /*
+ * End the passed name at a newline.
+ */
+ s = strnchr(value, size, '\n');
+ if (s)
+ *s = '\0';
+
+ if (!strcmp(name, "display")) {
+ union security_list_options secid_to_secctx;
+ union security_list_options secctx_to_secid;
+ union security_list_options socket_getpeersec_stream;
+
+ if (size == 0 || size >= 100)
+ return -EINVAL;
+
+ secid_to_secctx.secid_to_secctx = NULL;
+ hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
+ list) {
+ if (size >= strlen(hp->lsm) &&
+ !strncmp(value, hp->lsm, size)) {
+ secid_to_secctx = hp->hook;
+ found = true;
+ break;
+ }
+ }
+ secctx_to_secid.secctx_to_secid = NULL;
+ hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid,
+ list) {
+ if (size >= strlen(hp->lsm) &&
+ !strncmp(value, hp->lsm, size)) {
+ secctx_to_secid = hp->hook;
+ found = true;
+ break;
+ }
+ }
+ socket_getpeersec_stream.socket_getpeersec_stream = NULL;
+ hlist_for_each_entry(hp,
+ &security_hook_heads.socket_getpeersec_stream,
+ list) {
+ if (size >= strlen(hp->lsm) &&
+ !strncmp(value, hp->lsm, size)) {
+ socket_getpeersec_stream = hp->hook;
+ found = true;
+ break;
+ }
+ }
+ if (!found)
+ return -EINVAL;
+
+ /*
+ * The named lsm is active and supplies one or more
+ * of the relevant hooks. Switch to it.
+ */
+ s = kmemdup(value, size + 1, GFP_KERNEL);
+ if (s == NULL)
+ return -ENOMEM;
+ s[size] = '\0';
+
+ if (loh->lsm)
+ kfree(loh->lsm);
+ loh->lsm = s;
+ loh->secid_to_secctx = secid_to_secctx;
+ loh->secctx_to_secid = secctx_to_secid;
+ loh->socket_getpeersec_stream = socket_getpeersec_stream;
+
+ return size;
+ }
hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
if (lsm != NULL && strcmp(lsm, hp->lsm))
continue;
+ if (lsm == NULL && loh->lsm && strcmp(loh->lsm, hp->lsm))
+ continue;
return hp->hook.setprocattr(name, value, size);
}
return -EINVAL;
--
2.19.1
^ permalink raw reply related
* [PATCH 56/58] SELinux: Use blob offset in current_sid
From: Casey Schaufler @ 2019-06-02 16:50 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
Replace the use of current_security() with a call to current_cred()
so that the blob offset can be correctly applied.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/selinux/include/objsec.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 59a3b1cd5ba9..c9a88b7a96a7 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -31,6 +31,8 @@
#include "flask.h"
#include "avc.h"
+extern struct lsm_blob_sizes selinux_blob_sizes;
+
struct task_security_struct {
u32 osid; /* SID prior to last execve */
u32 sid; /* current SID */
@@ -45,7 +47,9 @@ struct task_security_struct {
*/
static inline u32 current_sid(void)
{
- const struct task_security_struct *tsec = current_security();
+ const struct task_security_struct *tsec;
+
+ tsec = current_cred()->security + selinux_blob_sizes.lbs_cred;
return tsec->sid;
}
@@ -174,7 +178,6 @@ struct bpf_security_struct {
u32 sid; /*SID of bpf obj creater*/
};
-extern struct lsm_blob_sizes selinux_blob_sizes;
static inline struct task_security_struct *selinux_cred(const struct cred *cred)
{
return cred->security + selinux_blob_sizes.lbs_cred;
--
2.19.1
^ permalink raw reply related
* [PATCH 53/58] kernfs: remove lsm_context scaffolding
From: Casey Schaufler @ 2019-06-02 16:50 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
Change the parameters to kernfs_node_setsecdata from a
data/length pair to a lsm_context struct as both the function
it calls and the function that calls it want that.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
fs/kernfs/inode.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 4c7da446d210..d6e25cd7bf21 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -135,20 +135,15 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
return error;
}
-static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
- u32 *secdata_len)
+static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs,
+ struct lsm_context *cp)
{
- void *old_secdata;
- size_t old_secdata_len;
+ struct lsm_context old_context;
- old_secdata = attrs->ia_context.context;
- old_secdata_len = attrs->ia_context.len;
+ old_context = attrs->ia_context;
+ attrs->ia_context = *cp;
+ *cp = old_context;
- attrs->ia_context.context = *secdata;
- attrs->ia_context.len = *secdata_len;
-
- *secdata = old_secdata;
- *secdata_len = old_secdata_len;
return 0;
}
@@ -363,7 +358,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
return error;
mutex_lock(&kernfs_mutex);
- error = kernfs_node_setsecdata(attrs, (void **)&lc.context, &lc.len);
+ error = kernfs_node_setsecdata(attrs, &lc);
mutex_unlock(&kernfs_mutex);
if (lc.context)
--
2.19.1
^ permalink raw reply related
* [PATCH 54/58] LSM: Remove unused macro
From: Casey Schaufler @ 2019-06-02 16:50 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
The call_one_void_hook macro is unused since the change
to how releasing a secctx was made. Remove it.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/security.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/security/security.c b/security/security.c
index 363647cf1ae8..ae43735575d6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -698,16 +698,6 @@ int lsm_superblock_alloc(struct super_block *sb)
P->hook.FUNC(__VA_ARGS__); \
} while (0)
-#define call_one_void_hook(FUNC, ...) \
- do { \
- struct security_hook_list *P; \
- \
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
- P->hook.FUNC(__VA_ARGS__); \
- break; \
- } \
- } while (0)
-
#define call_int_hook(FUNC, IRC, ...) ({ \
int RC = IRC; \
do { \
--
2.19.1
^ permalink raw reply related
* [PATCH 55/58] LSM: Special handling for secctx lsm hooks
From: Casey Schaufler @ 2019-06-02 16:50 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190602165101.25079-1-casey@schaufler-ca.com>
Create a special set of LSM hooks for the translation
to human readable security data.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 10 ++++++++++
security/security.c | 32 ++++++++++++++++++++++++--------
2 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 088985d10a69..d90d953c8ef4 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2032,6 +2032,16 @@ struct security_hook_list {
char *lsm;
} __randomize_layout;
+/*
+ * The set of hooks that may be selected for a specific module.
+ */
+struct lsm_one_hooks {
+ char *lsm;
+ union security_list_options secid_to_secctx;
+ union security_list_options secctx_to_secid;
+ union security_list_options socket_getpeersec_stream;
+};
+
/*
* Security blob size or offset data.
*/
diff --git a/security/security.c b/security/security.c
index ae43735575d6..b05265ec24f0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -420,6 +420,9 @@ static int lsm_append(char *new, char **result)
return 0;
}
+/* Base list of once-only hooks */
+struct lsm_one_hooks lsm_base_one;
+
/**
* security_add_hooks - Add a modules hooks to the hook lists.
* @hooks: the hooks to add
@@ -436,6 +439,25 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
for (i = 0; i < count; i++) {
hooks[i].lsm = lsm;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+
+ /*
+ * Check for the special hooks that are restricted to
+ * a single module to create the base set. Use the hooks
+ * from that module for the set, which may not be complete.
+ */
+ if (lsm_base_one.lsm && strcmp(lsm_base_one.lsm, hooks[i].lsm))
+ continue;
+ if (hooks[i].head == &security_hook_heads.secid_to_secctx)
+ lsm_base_one.secid_to_secctx = hooks[i].hook;
+ else if (hooks[i].head == &security_hook_heads.secctx_to_secid)
+ lsm_base_one.secctx_to_secid = hooks[i].hook;
+ else if (hooks[i].head ==
+ &security_hook_heads.socket_getpeersec_stream)
+ lsm_base_one.socket_getpeersec_stream = hooks[i].hook;
+ else
+ continue;
+ if (lsm_base_one.lsm == NULL)
+ lsm_base_one.lsm = kstrdup(hooks[i].lsm, GFP_KERNEL);
}
if (lsm_append(lsm, &lsm_names) < 0)
panic("%s - Cannot get early memory.\n", __func__);
@@ -714,14 +736,8 @@ int lsm_superblock_alloc(struct super_block *sb)
#define call_one_int_hook(FUNC, IRC, ...) ({ \
int RC = IRC; \
- do { \
- struct security_hook_list *P; \
- \
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
- RC = P->hook.FUNC(__VA_ARGS__); \
- break; \
- } \
- } while (0); \
+ if (lsm_base_one.FUNC.FUNC) \
+ RC = lsm_base_one.FUNC.FUNC(__VA_ARGS__); \
RC; \
})
--
2.19.1
^ permalink raw reply related
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