* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 17:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <20190517172953.GC15006@linux.intel.com>
On 5/17/19 1:29 PM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:
>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>> I think we may want to change the SGX API to alloc an anon inode for each
>>>> enclave instead of hanging every enclave off of the /dev/sgx/enclave
>>>> inode.
>>>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
>>>> VMAs
>>>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>>>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>>> 64-bit only at this point, so SELinux will always have default_noexec).
>>>
>>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE
>>> and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable,
>>> only if the page is also writable or previously modified. The intent is
>>> to prevent arbitrary code execution without EXECMEM (or
>>> FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created
>>> without EXECMEM as long as the EPC page mapping is only ever mapped RX and
>>> its initial contents came from an unmodified file mapping that was
>>> PROT_EXEC (and hence already checked via FILE__EXECUTE).
>
> The idea is that by providing an SGX ioctl() to propagate VMA permissions
> from a source VMA, EXECMEM wouldn't be required to make an EPC page
> executable. E.g. userspace establishes an enclave in non-EPC memory from
> an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl()
> to copy the contents and permissions into EPC memory.
>
>> Also, just to be clear, there is nothing inherently better about checking
>> EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
>> /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
>> reason. Using anon inodes also unfortunately disables SELinux inode-based
>> checking since we no longer have any useful inode information, so you'd lose
>> out on SELinux ioctl whitelisting on those enclave inodes if that matters.
>
> The problem is that all enclaves are associated with a single inode, i.e.
> /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to
> provide ioctls() and to allow mmap()'ing EPC memory. In no way is it
> associated with the content that actually gets loaded into EPC memory.
>
> The actual file that contains the enclave's contents (assuming the enclave
> came from a file) is a separate regular file that the SGX subsystem never
> sees.
>
> AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> PROCESS__EXECMEM achieves per-process granularity.
>
No, FILE__WRITE and FILE__EXECUTE are a check between a process and a
file, so you can ensure that only whitelisted processes are allowed both
to /dev/sgx/enclave.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 17:43 UTC (permalink / raw)
To: Sean Christopherson
Cc: Stephen Smalley, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, 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: <20190517172953.GC15006@linux.intel.com>
> On May 17, 2019, at 10:29 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>
>> On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:
>>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>> I think we may want to change the SGX API to alloc an anon inode for each
>>>> enclave instead of hanging every enclave off of the /dev/sgx/enclave
>>>> inode.
>>>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
>>>> VMAs
>>>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>>>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>>> 64-bit only at this point, so SELinux will always have default_noexec).
>>>
>>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE
>>> and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable,
>>> only if the page is also writable or previously modified. The intent is
>>> to prevent arbitrary code execution without EXECMEM (or
>>> FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created
>>> without EXECMEM as long as the EPC page mapping is only ever mapped RX and
>>> its initial contents came from an unmodified file mapping that was
>>> PROT_EXEC (and hence already checked via FILE__EXECUTE).
>
> The idea is that by providing an SGX ioctl() to propagate VMA permissions
> from a source VMA, EXECMEM wouldn't be required to make an EPC page
> executable. E.g. userspace establishes an enclave in non-EPC memory from
> an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl()
> to copy the contents and permissions into EPC memory.
>
>> Also, just to be clear, there is nothing inherently better about checking
>> EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
>> /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
>> reason. Using anon inodes also unfortunately disables SELinux inode-based
>> checking since we no longer have any useful inode information, so you'd lose
>> out on SELinux ioctl whitelisting on those enclave inodes if that matters.
>
> The problem is that all enclaves are associated with a single inode, i.e.
> /dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to
> provide ioctls() and to allow mmap()'ing EPC memory. In no way is it
> associated with the content that actually gets loaded into EPC memory.
>
> The actual file that contains the enclave's contents (assuming the enclave
> came from a file) is a separate regular file that the SGX subsystem never
> sees.
>
> AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
> *any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
> PROCESS__EXECMEM achieves per-process granularity.
How does anon_inode make any difference? Anon_inode is not the same thing as anon_vma.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 17:29 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <80013cca-f1c2-f4d5-7558-8f4e752ada76@tycho.nsa.gov>
On Fri, May 17, 2019 at 12:37:40PM -0400, Stephen Smalley wrote:
> On 5/17/19 12:20 PM, Stephen Smalley wrote:
> >On 5/17/19 11:09 AM, Sean Christopherson wrote:
> >>I think we may want to change the SGX API to alloc an anon inode for each
> >>enclave instead of hanging every enclave off of the /dev/sgx/enclave
> >>inode.
> >>Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
> >>will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
> >>VMAs
> >>to RWX. Backing each enclave with an anon inode will make SELinux treat
> >>EPC memory like anonymous mappings, which is what we want (I think), e.g.
> >>making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
> >>64-bit only at this point, so SELinux will always have default_noexec).
> >
> >I don't think we want to require EXECMEM (or equivalently both FILE__WRITE
> >and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable,
> >only if the page is also writable or previously modified. The intent is
> >to prevent arbitrary code execution without EXECMEM (or
> >FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created
> >without EXECMEM as long as the EPC page mapping is only ever mapped RX and
> >its initial contents came from an unmodified file mapping that was
> >PROT_EXEC (and hence already checked via FILE__EXECUTE).
The idea is that by providing an SGX ioctl() to propagate VMA permissions
from a source VMA, EXECMEM wouldn't be required to make an EPC page
executable. E.g. userspace establishes an enclave in non-EPC memory from
an unmodified file (with FILE__EXECUTE perms), and the uses the SGX ioctl()
to copy the contents and permissions into EPC memory.
> Also, just to be clear, there is nothing inherently better about checking
> EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the
> /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that
> reason. Using anon inodes also unfortunately disables SELinux inode-based
> checking since we no longer have any useful inode information, so you'd lose
> out on SELinux ioctl whitelisting on those enclave inodes if that matters.
The problem is that all enclaves are associated with a single inode, i.e.
/dev/sgx/enclave. /dev/sgx/enclave is a char device whose purpose is to
provide ioctls() and to allow mmap()'ing EPC memory. In no way is it
associated with the content that actually gets loaded into EPC memory.
The actual file that contains the enclave's contents (assuming the enclave
came from a file) is a separate regular file that the SGX subsystem never
sees.
AIUI, having FILE__WRITE and FILE__EXECUTE on /dev/sgx/enclave would allow
*any* enclave/process to map EPC as RWX. Moving to anon inodes and thus
PROCESS__EXECMEM achieves per-process granularity.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 17:12 UTC (permalink / raw)
To: Stephen Smalley
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, 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: <80013cca-f1c2-f4d5-7558-8f4e752ada76@tycho.nsa.gov>
> On May 17, 2019, at 9:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> On 5/17/19 12:20 PM, Stephen Smalley wrote:
>>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>>> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>>>>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>>>> I thought EXECMOD applied to files (and memory mappings backed by them) but
>>>>> I was probably wrong. It sounds like EXECMOD applies to the whole process so
>>>>> would allow all pages within a process's address space to be modified then
>>>>> executed, regardless the backing files. Am I correct this time?
>>>>
>>>> No, you were correct the first time I think; EXECMOD is used to control
>>>> whether a process can make executable a private file mapping that has
>>>> previously been modified (e.g. text relocation); it is a special case to
>>>> support text relocations without having to allow full EXECMEM (i.e. execute
>>>> arbitrary memory).
>>>>
>>>> SELinux checks relevant to W^X include:
>>>>
>>>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
>>>> PROT_WRITE, since we know the content has to have been written at some
>>>> point) or a private file mapping that is also PROT_WRITE.
>>>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>>>> previously modified, typically for text relocations,
>>>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>>>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>>>
>>>> (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
>>>> this discussion)
>>>>
>>>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>>>> process, EXECMOD by the process to any file, and the combination of both
>>>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>>>
>>>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
>>>> anonymous inode, then I would expect that only the FILE__WRITE and
>>>> FILE__EXECUTE checks are relevant.
>>>
>>> Yep, I was just typing this up in a different thread:
>>>
>>> I think we may want to change the SGX API to alloc an anon inode for each
>>> enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
>>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
>>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>>> 64-bit only at this point, so SELinux will always have default_noexec).
>> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if the page is also writable or previously modified. The intent is to prevent arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created without EXECMEM as long as the EPC page mapping is only ever mapped RX and its initial contents came from an unmodified file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
>
> Also, just to be clear, there is nothing inherently better about checking EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE to the /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes for that reason. Using anon inodes also unfortunately disables SELinux inode-based checking since we no longer have any useful inode information, so you'd lose out on SELinux ioctl whitelisting on those enclave inodes if that matters.
How can that work? Unless the API changes fairly radically, users fundamentally need to both write and execute the enclave. Some of it will be written only from already executable pages, and some privilege should be needed to execute any enclave page that was not loaded like this.
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-05-17 17:11 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Andrew Morton, Christoph Lameter,
Kernel Hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <201905170928.A8F3BEC1B1@keescook>
On Fri 17-05-19 09:36:36, Kees Cook wrote:
> On Fri, May 17, 2019 at 04:20:48PM +0200, Michal Hocko wrote:
> > On Fri 17-05-19 16:11:32, Alexander Potapenko wrote:
> > > On Fri, May 17, 2019 at 4:04 PM Michal Hocko <mhocko@kernel.org> wrote:
> > > >
> > > > On Tue 14-05-19 16:35:34, Alexander Potapenko wrote:
> > > > > The new options are needed to prevent possible information leaks and
> > > > > make control-flow bugs that depend on uninitialized values more
> > > > > deterministic.
> > > > >
> > > > > init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> > > > > objects with zeroes. Initialization is done at allocation time at the
> > > > > places where checks for __GFP_ZERO are performed.
> > > > >
> > > > > init_on_free=1 makes the kernel initialize freed pages and heap objects
> > > > > with zeroes upon their deletion. This helps to ensure sensitive data
> > > > > doesn't leak via use-after-free accesses.
> > > >
> > > > Why do we need both? The later is more robust because even free memory
> > > > cannot be sniffed and the overhead might be shifted from the allocation
> > > > context (e.g. to RCU) but why cannot we stick to a single model?
> > > init_on_free appears to be slower because of cache effects. It's
> > > several % in the best case vs. <1% for init_on_alloc.
> >
> > This doesn't really explain why we need both.
>
> There are a couple reasons. The first is that once we have hardware with
> memory tagging (e.g. arm64's MTE) we'll need both on_alloc and on_free
> hooks to do change the tags. With MTE, zeroing comes for "free" with
> tagging (though tagging is as slow as zeroing, so it's really the tagging
> that is free...), so we'll need to re-use the init_on_free infrastructure.
I am not sure I follow, but ...
>
> The second reason is for very paranoid use-cases where in-memory
> data lifetime is desired to be minimized. There are various arguments
> for/against the realism of the associated threat models, but given that
> we'll need the infrastructre for MTE anyway, and there are people who
> want wipe-on-free behavior no matter what the performance cost, it seems
> reasonable to include it in this series.
>
> All that said, init_on_alloc looks desirable enough that distros will
> likely build with it enabled by default (I hope), and the very paranoid
> users will switch to (or additionally enable) init_on_free for their
> systems.
... this should all be part of the changelog.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Michal Hocko @ 2019-05-17 17:11 UTC (permalink / raw)
To: Kees Cook
Cc: Alexander Potapenko, Andrew Morton, Christoph Lameter,
Kernel Hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
Souptick Joarder, Matthew Wilcox, Linux Memory Management List,
linux-security-module
In-Reply-To: <201905170925.6FD47DDFFF@keescook>
On Fri 17-05-19 09:27:54, Kees Cook wrote:
> On Fri, May 17, 2019 at 04:01:08PM +0200, Michal Hocko wrote:
> > On Fri 17-05-19 15:37:14, Alexander Potapenko wrote:
> > > > > > Freeing a memory is an opt-in feature and the slab allocator can already
> > > > > > tell many (with constructor or GFP_ZERO) do not need it.
> > > > > Sorry, I didn't understand this piece. Could you please elaborate?
> > > >
> > > > The allocator can assume that caches with a constructor will initialize
> > > > the object so additional zeroying is not needed. GFP_ZERO should be self
> > > > explanatory.
> > > Ah, I see. We already do that, see the want_init_on_alloc()
> > > implementation here: https://patchwork.kernel.org/patch/10943087/
> > > > > > So can we go without this gfp thing and see whether somebody actually
> > > > > > finds a performance problem with the feature enabled and think about
> > > > > > what can we do about it rather than add this maint. nightmare from the
> > > > > > very beginning?
> > > > >
> > > > > There were two reasons to introduce this flag initially.
> > > > > The first was double initialization of pages allocated for SLUB.
> > > >
> > > > Could you elaborate please?
> > > When the kernel allocates an object from SLUB, and SLUB happens to be
> > > short on free pages, it requests some from the page allocator.
> > > Those pages are initialized by the page allocator
> >
> > ... when the feature is enabled ...
> >
> > > and split into objects. Finally SLUB initializes one of the available
> > > objects and returns it back to the kernel.
> > > Therefore the object is initialized twice for the first time (when it
> > > comes directly from the page allocator).
> > > This cost is however amortized by SLUB reusing the object after it's been freed.
> >
> > OK, I see what you mean now. Is there any way to special case the page
> > allocation for this feature? E.g. your implementation tries to make this
> > zeroying special but why cannot you simply do this
> >
> >
> > struct page *
> > ____alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, int preferred_nid,
> > nodemask_t *nodemask)
> > {
> > //current implementation
> > }
> >
> > struct page *
> > __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, int preferred_nid,
> > nodemask_t *nodemask)
> > {
> > if (your_feature_enabled)
> > gfp_mask |= __GFP_ZERO;
> > return ____alloc_pages_nodemask(gfp_mask, order, preferred_nid,
> > nodemask);
> > }
> >
> > and use ____alloc_pages_nodemask from the slab or other internal
> > allocators?
>
> If an additional allocator function is preferred over a new GFP flag, then
> I don't see any reason not to do this. (Though adding more "__"s seems
> a bit unfriendly to code-documentation.) What might be better naming?
The naminig is the last thing I would be worried about. Let's focus on
the most simplistic implementation first. And means, can we really make
it as simple as above? At least on the page allocator level.
> This would mean that the skb changes later in the series would use the
> "no auto init" version of the allocator too, then.
No, this would be an internal function to MM. I would really like to
optimize once there are numbers from _real_ workloads to base those
optimizations.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH v3 2/2] initramfs: introduce do_readxattrs()
From: Roberto Sassu @ 2019-05-17 16:55 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
In-Reply-To: <20190517165519.11507-1-roberto.sassu@huawei.com>
This patch adds support for an alternative method to add xattrs to files in
the rootfs filesystem. Instead of extracting them directly from the ram
disk image, they are extracted from a regular file called .xattr-list, that
can be added by any ram disk generator available today. The file format is:
<file #N data len (ASCII, 10 chars)><file #N path>\0
<xattr #N data len (ASCII, 8 chars)><xattr #N name>\0<xattr #N value>
.xattr-list can be generated by executing:
$ getfattr --absolute-names -d -h -R -e hex -m - \
<file list> | xattr.awk -b > ${initdir}/.xattr-list
where the content of the xattr.awk script is:
#! /usr/bin/awk -f
{
if (!length($0)) {
printf("%.10x%s\0", len, file);
for (x in xattr) {
printf("%.8x%s\0", xattr_len[x], x);
for (i = 0; i < length(xattr[x]) / 2; i++) {
printf("%c", strtonum("0x"substr(xattr[x], i * 2 + 1, 2)));
}
}
i = 0;
delete xattr;
delete xattr_len;
next;
};
if (i == 0) {
file=$3;
len=length(file) + 8 + 1;
}
if (i > 0) {
split($0, a, "=");
xattr[a[1]]=substr(a[2], 3);
xattr_len[a[1]]=length(a[1]) + 1 + 8 + length(xattr[a[1]]) / 2;
len+=xattr_len[a[1]];
};
i++;
}
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
init/initramfs.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/init/initramfs.c b/init/initramfs.c
index 0c6dd1d5d3f6..6ec018c6279a 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -13,6 +13,8 @@
#include <linux/namei.h>
#include <linux/xattr.h>
+#define XATTR_LIST_FILENAME ".xattr-list"
+
static ssize_t __init xwrite(int fd, const char *p, size_t count)
{
ssize_t out = 0;
@@ -382,6 +384,97 @@ static int __init __maybe_unused do_setxattrs(char *pathname)
return 0;
}
+struct path_hdr {
+ char p_size[10]; /* total size including p_size field */
+ char p_data[]; /* <path>\0<xattrs> */
+};
+
+static int __init do_readxattrs(void)
+{
+ struct path_hdr hdr;
+ char *path = NULL;
+ char str[sizeof(hdr.p_size) + 1];
+ unsigned long file_entry_size;
+ size_t size, path_size, total_size;
+ struct kstat st;
+ struct file *file;
+ loff_t pos;
+ int ret;
+
+ ret = vfs_lstat(XATTR_LIST_FILENAME, &st);
+ if (ret < 0)
+ return ret;
+
+ total_size = st.size;
+
+ file = filp_open(XATTR_LIST_FILENAME, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ pos = file->f_pos;
+
+ while (total_size) {
+ size = kernel_read(file, (char *)&hdr, sizeof(hdr), &pos);
+ if (size != sizeof(hdr)) {
+ ret = -EIO;
+ goto out;
+ }
+
+ total_size -= size;
+
+ str[sizeof(hdr.p_size)] = 0;
+ memcpy(str, hdr.p_size, sizeof(hdr.p_size));
+ ret = kstrtoul(str, 16, &file_entry_size);
+ if (ret < 0)
+ goto out;
+
+ file_entry_size -= sizeof(sizeof(hdr.p_size));
+ if (file_entry_size > total_size) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ path = vmalloc(file_entry_size);
+ if (!path) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ size = kernel_read(file, path, file_entry_size, &pos);
+ if (size != file_entry_size) {
+ ret = -EIO;
+ goto out_free;
+ }
+
+ total_size -= size;
+
+ path_size = strnlen(path, file_entry_size);
+ if (path_size == file_entry_size) {
+ ret = -EINVAL;
+ goto out_free;
+ }
+
+ xattr_buf = path + path_size + 1;
+ xattr_len = file_entry_size - path_size - 1;
+
+ ret = do_setxattrs(path);
+ vfree(path);
+ path = NULL;
+
+ if (ret < 0)
+ break;
+ }
+out_free:
+ vfree(path);
+out:
+ fput(file);
+
+ if (ret < 0)
+ error("Unable to parse xattrs");
+
+ return ret;
+}
+
static __initdata int wfd;
static int __init do_name(void)
@@ -391,6 +484,11 @@ static int __init do_name(void)
if (strcmp(collected, "TRAILER!!!") == 0) {
free_hash();
return 0;
+ } else if (strcmp(collected, XATTR_LIST_FILENAME) == 0) {
+ struct kstat st;
+
+ if (!vfs_lstat(collected, &st))
+ do_readxattrs();
}
clean_path(collected, mode);
if (S_ISREG(mode)) {
@@ -562,6 +660,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
buf += my_inptr;
len -= my_inptr;
}
+ do_readxattrs();
dir_utime();
kfree(name_buf);
kfree(symlink_buf);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/2] initramfs: set extended attributes
From: Roberto Sassu @ 2019-05-17 16:55 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
In-Reply-To: <20190517165519.11507-1-roberto.sassu@huawei.com>
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
This patch adds xattrs to a file, with name and value taken from a supplied
buffer. The data format is:
<xattr #N data len (ASCII, 8 chars)><xattr #N name>\0<xattr #N value>
[kamensky: fixed restoring of xattrs for symbolic links by using
sys_lsetxattr() instead of sys_setxattr()]
[sassu: removed state management, kept only do_setxattrs(), replaced
sys_lsetxattr() with vfs_setxattr(), added check for
xattr_entry_size, added check for hdr->c_size, replaced strlen()
with strnlen(); moved do_setxattrs() before do_name()]
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Victor Kamensky <kamensky@cisco.com>
Signed-off-by: Taras Kondratiuk <takondra@cisco.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
init/initramfs.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c
index 435a428c2af1..0c6dd1d5d3f6 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -10,6 +10,8 @@
#include <linux/syscalls.h>
#include <linux/utime.h>
#include <linux/file.h>
+#include <linux/namei.h>
+#include <linux/xattr.h>
static ssize_t __init xwrite(int fd, const char *p, size_t count)
{
@@ -146,7 +148,8 @@ static __initdata time64_t mtime;
static __initdata unsigned long ino, major, minor, nlink;
static __initdata umode_t mode;
-static __initdata unsigned long body_len, name_len;
+static __initdata u32 name_len, xattr_len;
+static __initdata u64 body_len;
static __initdata uid_t uid;
static __initdata gid_t gid;
static __initdata unsigned rdev;
@@ -218,7 +221,7 @@ static void __init read_into(char *buf, unsigned size, enum state next)
}
}
-static __initdata char *header_buf, *symlink_buf, *name_buf;
+static __initdata char *header_buf, *symlink_buf, *name_buf, *xattr_buf;
static int __init do_start(void)
{
@@ -315,6 +318,70 @@ static int __init maybe_link(void)
return 0;
}
+struct xattr_hdr {
+ char c_size[8]; /* total size including c_size field */
+ char c_data[]; /* <name>\0<value> */
+};
+
+static int __init __maybe_unused do_setxattrs(char *pathname)
+{
+ char *buf = xattr_buf;
+ char *bufend = buf + xattr_len;
+ struct xattr_hdr *hdr;
+ char str[sizeof(hdr->c_size) + 1];
+ struct path path;
+
+ if (!xattr_len)
+ return 0;
+
+ str[sizeof(hdr->c_size)] = 0;
+
+ while (buf < bufend) {
+ char *xattr_name, *xattr_value;
+ unsigned long xattr_entry_size;
+ unsigned long xattr_name_size, xattr_value_size;
+ int ret;
+
+ if (buf + sizeof(hdr->c_size) > bufend) {
+ error("malformed xattrs");
+ break;
+ }
+
+ hdr = (struct xattr_hdr *)buf;
+ memcpy(str, hdr->c_size, sizeof(hdr->c_size));
+ ret = kstrtoul(str, 16, &xattr_entry_size);
+ buf += xattr_entry_size;
+ if (ret || buf > bufend || !xattr_entry_size) {
+ error("malformed xattrs");
+ break;
+ }
+
+ xattr_name = hdr->c_data;
+ xattr_name_size = strnlen(xattr_name,
+ xattr_entry_size - sizeof(hdr->c_size));
+ if (xattr_name_size == xattr_entry_size - sizeof(hdr->c_size)) {
+ error("malformed xattrs");
+ break;
+ }
+
+ xattr_value = xattr_name + xattr_name_size + 1;
+ xattr_value_size = buf - xattr_value;
+
+ ret = kern_path(pathname, 0, &path);
+ if (!ret) {
+ ret = vfs_setxattr(path.dentry, xattr_name, xattr_value,
+ xattr_value_size, 0);
+
+ path_put(&path);
+ }
+
+ pr_debug("%s: %s size: %lu val: %s (ret: %d)\n", pathname,
+ xattr_name, xattr_value_size, xattr_value, ret);
+ }
+
+ return 0;
+}
+
static __initdata int wfd;
static int __init do_name(void)
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/2] initramfs: add support for xattrs in the initial ram disk
From: Roberto Sassu @ 2019-05-17 16:55 UTC (permalink / raw)
To: viro
Cc: linux-security-module, linux-integrity, initramfs, linux-api,
linux-fsdevel, linux-kernel, zohar, silviu.vlasceanu,
dmitry.kasatkin, takondra, kamensky, hpa, arnd, rob,
james.w.mcmechan, niveditas98, Roberto Sassu
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 marshaling pathnames and xattrs in a file called
.xattr-list. They are unmarshaled by the CPIO parser after all files have
been extracted, or before the next ram disk is processed.
The difference from v1 (https://lkml.org/lkml/2018/11/22/1182) is that all
xattrs are stored in a single file and not per file (solves the file name
limitation issue, as it is not necessary to add a suffix to files
containing xattrs).
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, as opposed to
defining a new one. As seen from the discussion, if a new format has to be
defined, it should fix the issues of the existing format, which requires
more time.
To fulfill both requirements, adding support for xattrs in a short time and
defining a new image format properly, this patch set takes an incremental
approach: it introduces a parser of xattrs that can be used either if
xattrs are in a regular file or directly added to the image (this patch set
reuses patch 9/15 of the existing proposal); in addition, it introduces a
wrapper of the xattr parser, to read xattrs from a file.
The changes introduced by this patch set don't cause any compatibility
issue: kernels without the xattr parser simply extracts .xattr-list and
don't unmarshal xattrs; kernels with the xattr parser don't unmarshal
xattrs if .xattr-list is not found 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_readxattrs(). From the user space perspective, no change is
required for the use case. A new dracut module (module-setup.sh) will
execute:
getfattr --absolute-names -d -h -R -e hex -m security.ima \
<file list> | xattr.awk -b > ${initdir}/.xattr-list
where xattr.awk is the script that marshals xattrs (see patch 3/3). The
same can be done with the initramfs-tools ram disk generator.
Changelog
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()
Mimi Zohar (1):
initramfs: set extended attributes
Roberto Sassu (1):
initramfs: introduce do_readxattrs()
init/initramfs.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 168 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 16:37 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <ca807220-47e2-5ec2-982c-4fb4a72439c6@tycho.nsa.gov>
On 5/17/19 12:20 PM, Stephen Smalley wrote:
> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>>> I thought EXECMOD applied to files (and memory mappings backed by
>>>> them) but
>>>> I was probably wrong. It sounds like EXECMOD applies to the whole
>>>> process so
>>>> would allow all pages within a process's address space to be
>>>> modified then
>>>> executed, regardless the backing files. Am I correct this time?
>>>
>>> No, you were correct the first time I think; EXECMOD is used to control
>>> whether a process can make executable a private file mapping that has
>>> previously been modified (e.g. text relocation); it is a special case to
>>> support text relocations without having to allow full EXECMEM (i.e.
>>> execute
>>> arbitrary memory).
>>>
>>> SELinux checks relevant to W^X include:
>>>
>>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
>>> PROT_WRITE, since we know the content has to have been written at some
>>> point) or a private file mapping that is also PROT_WRITE.
>>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>>> previously modified, typically for text relocations,
>>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>>
>>> (ignoring EXECSTACK and EXECHEAP here since they aren't really
>>> relevant to
>>> this discussion)
>>>
>>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>>> process, EXECMOD by the process to any file, and the combination of both
>>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>>
>>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
>>> anonymous inode, then I would expect that only the FILE__WRITE and
>>> FILE__EXECUTE checks are relevant.
>>
>> Yep, I was just typing this up in a different thread:
>>
>> I think we may want to change the SGX API to alloc an anon inode for each
>> enclave instead of hanging every enclave off of the /dev/sgx/enclave
>> inode.
>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave
>> VMAs
>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>> 64-bit only at this point, so SELinux will always have default_noexec).
>
> I don't think we want to require EXECMEM (or equivalently both
> FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC
> page executable, only if the page is also writable or previously
> modified. The intent is to prevent arbitrary code execution without
> EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to
> be created without EXECMEM as long as the EPC page mapping is only ever
> mapped RX and its initial contents came from an unmodified file mapping
> that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
Also, just to be clear, there is nothing inherently better about
checking EXECMEM instead of checking both FILE__WRITE and FILE__EXECUTE
to the /dev/sgx/enclave inode, so I wouldn't switch to using anon inodes
for that reason. Using anon inodes also unfortunately disables SELinux
inode-based checking since we no longer have any useful inode
information, so you'd lose out on SELinux ioctl whitelisting on those
enclave inodes if that matters.
^ permalink raw reply
* Re: [PATCH v2 2/4] lib: introduce test_meminit module
From: Kees Cook @ 2019-05-17 16:37 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Jann Horn, Linux Memory Management List,
linux-security-module
In-Reply-To: <CAG_fn=VVZ1FBygbAeTbdo2U2d2Zga6Z7wVitkqZB0YffCKYzag@mail.gmail.com>
On Fri, May 17, 2019 at 05:51:17PM +0200, Alexander Potapenko wrote:
> On Thu, May 16, 2019 at 3:02 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, May 14, 2019 at 04:35:35PM +0200, Alexander Potapenko wrote:
> > > Add tests for heap and pagealloc initialization.
> > > These can be used to check init_on_alloc and init_on_free implementations
> > > as well as other approaches to initialization.
> >
> > This is nice! Easy way to test the results. It might be helpful to show
> > here what to expect when loading this module:
>
> Do you want me to add the expected output to the patch description?
Yes, I think it's worth having, as a way to show people what to expect
when running the test, without having to actually enable, build, and
run it themselves.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Kees Cook @ 2019-05-17 16:36 UTC (permalink / raw)
To: Michal Hocko
Cc: Alexander Potapenko, Andrew Morton, Christoph Lameter,
Kernel Hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <20190517142048.GM6836@dhcp22.suse.cz>
On Fri, May 17, 2019 at 04:20:48PM +0200, Michal Hocko wrote:
> On Fri 17-05-19 16:11:32, Alexander Potapenko wrote:
> > On Fri, May 17, 2019 at 4:04 PM Michal Hocko <mhocko@kernel.org> wrote:
> > >
> > > On Tue 14-05-19 16:35:34, Alexander Potapenko wrote:
> > > > The new options are needed to prevent possible information leaks and
> > > > make control-flow bugs that depend on uninitialized values more
> > > > deterministic.
> > > >
> > > > init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> > > > objects with zeroes. Initialization is done at allocation time at the
> > > > places where checks for __GFP_ZERO are performed.
> > > >
> > > > init_on_free=1 makes the kernel initialize freed pages and heap objects
> > > > with zeroes upon their deletion. This helps to ensure sensitive data
> > > > doesn't leak via use-after-free accesses.
> > >
> > > Why do we need both? The later is more robust because even free memory
> > > cannot be sniffed and the overhead might be shifted from the allocation
> > > context (e.g. to RCU) but why cannot we stick to a single model?
> > init_on_free appears to be slower because of cache effects. It's
> > several % in the best case vs. <1% for init_on_alloc.
>
> This doesn't really explain why we need both.
There are a couple reasons. The first is that once we have hardware with
memory tagging (e.g. arm64's MTE) we'll need both on_alloc and on_free
hooks to do change the tags. With MTE, zeroing comes for "free" with
tagging (though tagging is as slow as zeroing, so it's really the tagging
that is free...), so we'll need to re-use the init_on_free infrastructure.
The second reason is for very paranoid use-cases where in-memory
data lifetime is desired to be minimized. There are various arguments
for/against the realism of the associated threat models, but given that
we'll need the infrastructre for MTE anyway, and there are people who
want wipe-on-free behavior no matter what the performance cost, it seems
reasonable to include it in this series.
All that said, init_on_alloc looks desirable enough that distros will
likely build with it enabled by default (I hope), and the very paranoid
users will switch to (or additionally enable) init_on_free for their
systems.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Kees Cook @ 2019-05-17 16:27 UTC (permalink / raw)
To: Michal Hocko
Cc: Alexander Potapenko, Andrew Morton, Christoph Lameter,
Kernel Hardening, Masahiro Yamada, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland,
Souptick Joarder, Matthew Wilcox, Linux Memory Management List,
linux-security-module
In-Reply-To: <20190517140108.GK6836@dhcp22.suse.cz>
On Fri, May 17, 2019 at 04:01:08PM +0200, Michal Hocko wrote:
> On Fri 17-05-19 15:37:14, Alexander Potapenko wrote:
> > > > > Freeing a memory is an opt-in feature and the slab allocator can already
> > > > > tell many (with constructor or GFP_ZERO) do not need it.
> > > > Sorry, I didn't understand this piece. Could you please elaborate?
> > >
> > > The allocator can assume that caches with a constructor will initialize
> > > the object so additional zeroying is not needed. GFP_ZERO should be self
> > > explanatory.
> > Ah, I see. We already do that, see the want_init_on_alloc()
> > implementation here: https://patchwork.kernel.org/patch/10943087/
> > > > > So can we go without this gfp thing and see whether somebody actually
> > > > > finds a performance problem with the feature enabled and think about
> > > > > what can we do about it rather than add this maint. nightmare from the
> > > > > very beginning?
> > > >
> > > > There were two reasons to introduce this flag initially.
> > > > The first was double initialization of pages allocated for SLUB.
> > >
> > > Could you elaborate please?
> > When the kernel allocates an object from SLUB, and SLUB happens to be
> > short on free pages, it requests some from the page allocator.
> > Those pages are initialized by the page allocator
>
> ... when the feature is enabled ...
>
> > and split into objects. Finally SLUB initializes one of the available
> > objects and returns it back to the kernel.
> > Therefore the object is initialized twice for the first time (when it
> > comes directly from the page allocator).
> > This cost is however amortized by SLUB reusing the object after it's been freed.
>
> OK, I see what you mean now. Is there any way to special case the page
> allocation for this feature? E.g. your implementation tries to make this
> zeroying special but why cannot you simply do this
>
>
> struct page *
> ____alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, int preferred_nid,
> nodemask_t *nodemask)
> {
> //current implementation
> }
>
> struct page *
> __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, int preferred_nid,
> nodemask_t *nodemask)
> {
> if (your_feature_enabled)
> gfp_mask |= __GFP_ZERO;
> return ____alloc_pages_nodemask(gfp_mask, order, preferred_nid,
> nodemask);
> }
>
> and use ____alloc_pages_nodemask from the slab or other internal
> allocators?
If an additional allocator function is preferred over a new GFP flag, then
I don't see any reason not to do this. (Though adding more "__"s seems
a bit unfriendly to code-documentation.) What might be better naming?
This would mean that the skb changes later in the series would use the
"no auto init" version of the allocator too, then.
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 16:20 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <20190517150948.GA15632@linux.intel.com>
On 5/17/19 11:09 AM, Sean Christopherson wrote:
> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>> I thought EXECMOD applied to files (and memory mappings backed by them) but
>>> I was probably wrong. It sounds like EXECMOD applies to the whole process so
>>> would allow all pages within a process's address space to be modified then
>>> executed, regardless the backing files. Am I correct this time?
>>
>> No, you were correct the first time I think; EXECMOD is used to control
>> whether a process can make executable a private file mapping that has
>> previously been modified (e.g. text relocation); it is a special case to
>> support text relocations without having to allow full EXECMEM (i.e. execute
>> arbitrary memory).
>>
>> SELinux checks relevant to W^X include:
>>
>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
>> PROT_WRITE, since we know the content has to have been written at some
>> point) or a private file mapping that is also PROT_WRITE.
>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>> previously modified, typically for text relocations,
>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>
>> (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
>> this discussion)
>>
>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>> process, EXECMOD by the process to any file, and the combination of both
>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>
>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
>> anonymous inode, then I would expect that only the FILE__WRITE and
>> FILE__EXECUTE checks are relevant.
>
> Yep, I was just typing this up in a different thread:
>
> I think we may want to change the SGX API to alloc an anon inode for each
> enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
> to RWX. Backing each enclave with an anon inode will make SELinux treat
> EPC memory like anonymous mappings, which is what we want (I think), e.g.
> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
> 64-bit only at this point, so SELinux will always have default_noexec).
I don't think we want to require EXECMEM (or equivalently both
FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC
page executable, only if the page is also writable or previously
modified. The intent is to prevent arbitrary code execution without
EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to
be created without EXECMEM as long as the EPC page mapping is only ever
mapped RX and its initial contents came from an unmodified file mapping
that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 16:24 UTC (permalink / raw)
To: Stephen Smalley
Cc: Sean Christopherson, Xing, Cedric, Andy Lutomirski, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Eric Paris,
selinux@vger.kernel.org, Jarkko Sakkinen, 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: <ca807220-47e2-5ec2-982c-4fb4a72439c6@tycho.nsa.gov>
> On May 17, 2019, at 9:20 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> On 5/17/19 11:09 AM, Sean Christopherson wrote:
>>> On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
>>>> On 5/16/19 6:23 PM, Xing, Cedric wrote:
>>>> I thought EXECMOD applied to files (and memory mappings backed by them) but
>>>> I was probably wrong. It sounds like EXECMOD applies to the whole process so
>>>> would allow all pages within a process's address space to be modified then
>>>> executed, regardless the backing files. Am I correct this time?
>>>
>>> No, you were correct the first time I think; EXECMOD is used to control
>>> whether a process can make executable a private file mapping that has
>>> previously been modified (e.g. text relocation); it is a special case to
>>> support text relocations without having to allow full EXECMEM (i.e. execute
>>> arbitrary memory).
>>>
>>> SELinux checks relevant to W^X include:
>>>
>>> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
>>> PROT_WRITE, since we know the content has to have been written at some
>>> point) or a private file mapping that is also PROT_WRITE.
>>> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
>>> previously modified, typically for text relocations,
>>> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
>>> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>>>
>>> (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
>>> this discussion)
>>>
>>> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
>>> process, EXECMOD by the process to any file, and the combination of both
>>> FILE__WRITE and FILE__EXECUTE by the process to any file.
>>>
>>> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
>>> anonymous inode, then I would expect that only the FILE__WRITE and
>>> FILE__EXECUTE checks are relevant.
>> Yep, I was just typing this up in a different thread:
>> I think we may want to change the SGX API to alloc an anon inode for each
>> enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
>> Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
>> will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
>> to RWX. Backing each enclave with an anon inode will make SELinux treat
>> EPC memory like anonymous mappings, which is what we want (I think), e.g.
>> making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
>> 64-bit only at this point, so SELinux will always have default_noexec).
>
> I don't think we want to require EXECMEM (or equivalently both FILE__WRITE and FILE__EXECUTE to /dev/sgx/enclave) for making any EPC page executable, only if the page is also writable or previously modified. The intent is to prevent arbitrary code execution without EXECMEM (or FILE__WRITE|FILE__EXECUTE), while still allowing enclaves to be created without EXECMEM as long as the EPC page mapping is only ever mapped RX and its initial contents came from an unmodified file mapping that was PROT_EXEC (and hence already checked via FILE__EXECUTE).
That agrees with my thoughts. Actually plumbing everything together so this works could be a bit interesting. I assume it’ll need a special case in SELinux or maybe a new vm_op.
^ permalink raw reply
* Re: [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Kees Cook @ 2019-05-17 16:13 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <CAG_fn=Vj6Jk_DY_-0+x6EpbsVh+abpEVcjycBhJxeMH3wuy9rw@mail.gmail.com>
On Fri, May 17, 2019 at 10:49:03AM +0200, Alexander Potapenko wrote:
> On Fri, May 17, 2019 at 2:26 AM Kees Cook <keescook@chromium.org> wrote:
> > On Thu, May 16, 2019 at 09:53:01AM -0700, Kees Cook wrote:
> > > On Tue, May 14, 2019 at 04:35:37PM +0200, Alexander Potapenko wrote:
> > > > Add sock_alloc_send_pskb_noinit(), which is similar to
> > > > sock_alloc_send_pskb(), but allocates with __GFP_NO_AUTOINIT.
> > > > This helps reduce the slowdown on hackbench in the init_on_alloc mode
> > > > from 6.84% to 3.45%.
> > >
> > > Out of curiosity, why the creation of the new function over adding a
> > > gfp flag argument to sock_alloc_send_pskb() and updating callers? (There
> > > are only 6 callers, and this change already updates 2 of those.)
> > >
> > > > Slowdown for the initialization features compared to init_on_free=0,
> > > > init_on_alloc=0:
> > > >
> > > > hackbench, init_on_free=1: +7.71% sys time (st.err 0.45%)
> > > > hackbench, init_on_alloc=1: +3.45% sys time (st.err 0.86%)
> >
> > So I've run some of my own wall-clock timings of kernel builds (which
> > should be an pretty big "worst case" situation, and I see much smaller
> > performance changes:
> How many cores were you using? I suspect the numbers may vary a bit
> depending on that.
I was using 4.
> > init_on_alloc=1
> > Run times: 289.72 286.95 287.87 287.34 287.35
> > Min: 286.95 Max: 289.72 Mean: 287.85 Std Dev: 0.98
> > 0.25% faster (within the std dev noise)
> >
> > init_on_free=1
> > Run times: 303.26 301.44 301.19 301.55 301.39
> > Min: 301.19 Max: 303.26 Mean: 301.77 Std Dev: 0.75
> > 4.57% slower
> >
> > init_on_free=1 with the PAX_MEMORY_SANITIZE slabs excluded:
> > Run times: 299.19 299.85 298.95 298.23 298.64
> > Min: 298.23 Max: 299.85 Mean: 298.97 Std Dev: 0.55
> > 3.60% slower
> >
> > So the tuning certainly improved things by 1%. My perf numbers don't
> > show the 24% hit you were seeing at all, though.
> Note that 24% is the _sys_ time slowdown. The wall time slowdown seen
> in this case was 8.34%
Ah! Gotcha. Yeah, seems the impact for init_on_free is pretty
variable. The init_on_alloc appears close to free, though.
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 16:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Xing, Cedric, James Morris, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <CALCETrXmyau8Gq-wKHZ5FdNGF+mqd7a+q+HAVR2sqvXA6av9BA@mail.gmail.com>
On Thu, May 16, 2019 at 05:35:16PM -0700, Andy Lutomirski wrote:
> On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@intel.com> wrote:
> > And if you are with me on that bigger picture, the next question is: what
> > should be the default behavior of security_sgx_mprot() for
> > existing/non-SGX-aware LSM modules/policies? I'd say a reasonable default
> > is to allow R, RW and RX, but not anything else. It'd suffice to get rid of
> > EXECMEM/EXECMOD requirements on enclave applications. For SGX1, EPCM
> > permissions are immutable so it really doesn't matter what
> > security_sgx_mprot() does. For SGX2 and beyond, there's still time and new
> > SGX-aware LSM modules/policies will probably have emerged by then.
>
> I hadn't thought about the SGX1 vs SGX2 difference. If the driver
> initially only wants to support SGX1, then I guess we really could get
> away with constraining the EPC flags based on the source page
> permission and not restricting mprotect() and mmap() permissions on
> /dev/sgx/enclave at all.
No, SGX1 vs SGX2 support in the kernel is irrelevant. Well, unless the
driver simply refuses to load on SGX2 hardware, but I don't think anyone
wants to go that route. There is no enabling or attribute bit required
to execute ENCLU[EMODPE], e.g. an enclave can effect RW->RWX in the EPCM
on SGX2 hardware regardless of what the kernel is doing.
IMO the kernel should ignore the EPCM from an LSM perspective.
^ permalink raw reply
* Re: [PATCH 5/4] mm: Introduce SLAB_NO_FREE_INIT and mark excluded caches
From: Kees Cook @ 2019-05-17 15:59 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <CAG_fn=W41zDac9DN9qVB_EwJG89f2cNBQYNyove4oO3dwe6d5Q@mail.gmail.com>
On Fri, May 17, 2019 at 10:34:26AM +0200, Alexander Potapenko wrote:
> On Fri, May 17, 2019 at 2:50 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > In order to improve the init_on_free performance, some frequently
> > freed caches with less sensitive contents can be excluded from the
> > init_on_free behavior.
> Did you see any notable performance improvement with this patch?
> A similar one gave me only 1-2% on the parallel Linux build.
Yup, that's in the other thread. I saw similar. But 1-2% on a 5% hit is
a lot. ;)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 2/4] lib: introduce test_meminit module
From: Alexander Potapenko @ 2019-05-17 15:51 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Jann Horn, Linux Memory Management List,
linux-security-module
In-Reply-To: <201905151752.2BD430A@keescook>
On Thu, May 16, 2019 at 3:02 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 14, 2019 at 04:35:35PM +0200, Alexander Potapenko wrote:
> > Add tests for heap and pagealloc initialization.
> > These can be used to check init_on_alloc and init_on_free implementations
> > as well as other approaches to initialization.
>
> This is nice! Easy way to test the results. It might be helpful to show
> here what to expect when loading this module:
Do you want me to add the expected output to the patch description?
> with either init_on_alloc=1 or init_on_free=1, I happily see:
>
> test_meminit: all 10 tests in test_pages passed
> test_meminit: all 40 tests in test_kvmalloc passed
> test_meminit: all 20 tests in test_kmemcache passed
> test_meminit: all 70 tests passed!
>
> and without:
>
> test_meminit: test_pages failed 10 out of 10 times
> test_meminit: test_kvmalloc failed 40 out of 40 times
> test_meminit: test_kmemcache failed 10 out of 20 times
> test_meminit: failures: 60 out of 70
>
>
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Kees Cook <keescook@chromium.org>
>
> note below...
>
> > [...]
> > diff --git a/lib/test_meminit.c b/lib/test_meminit.c
> > new file mode 100644
> > index 000000000000..67d759498030
> > --- /dev/null
> > +++ b/lib/test_meminit.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > [...]
> > +module_init(test_meminit_init);
>
> I get a warning at build about missing the license:
>
> WARNING: modpost: missing MODULE_LICENSE() in lib/test_meminit.o
>
> So, following the SPDX line, just add:
>
> MODULE_LICENSE("GPL");
Will do, thanks!
> --
> Kees Cook
--
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: Sean Christopherson @ 2019-05-17 15:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, Serge E. Hallyn, LSM List, Paul Moore,
Stephen Smalley, Eric Paris, selinux, Jarkko Sakkinen,
Jethro Beekman, Xing, Cedric, Hansen, Dave, Thomas Gleixner,
Dr. Greg, Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <CALCETrWxw7xALE0kmiYBzomaSMAeXEVq-7rX7xeqPtDPeDQiCA@mail.gmail.com>
On Thu, May 16, 2019 at 05:26:15PM -0700, Andy Lutomirski wrote:
> On Thu, May 16, 2019 at 5:03 PM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
> > > Here's a very vague proposal that's kind of like what I've been
> > > thinking over the past few days. The SGX inode could track, for each
> > > page, a "safe-to-execute" bit. When you first open /dev/sgx/enclave,
> > > you get a blank enclave and all pages are safe-to-execute. When you
> > > do the ioctl to load context (which could be code, data, or anything
> > > else), the kernel will check whether the *source* VMA is executable
> > > and, if not, mark the page of the enclave being loaded as unsafe.
> > > Once the enclave is initialized, the driver will clear the
> > > safe-to-execute bit for any page that is successfully mapped writably.
> > >
> > > The intent is that a page of the enclave is safe-to-execute if that
> > > page was populated from executable memory and not modified since then.
> > > LSMs could then enforce a policy that you can map an enclave page RX
> > > if the page is safe-to-execute, you can map any page you want for
> > > write if there are no executable mappings, and you can only map a page
> > > for write and execute simultaneously if you can EXECMOD permission.
> > > This should allow an enclave to be loaded by userspace from a file
> > > with EXECUTE rights.
> >
> > I'm still confused as to why you want to track execute permissions on the
> > enclave pages and add SGX-specific LSM hooks. Is there anything that
> > prevents userspace from building the enclave like any other DSO and then
> > copying it into enclave memory?
>
> It's entirely possible that I'm the one missing something. But here's
> why I think this:
>
> > I feel like I'm missing something.
> >
> > 1. Userspace loads enclave into regular memory, e.g. like a normal DSO.
> > All mmap(), mprotect(), etc... calls are subject to all existing
> > LSM policies.
> >
> > 2. Userspace opens /dev/sgx/enclave to instantiate a new enclave.
> >
> > 3. Userspace uses mmap() to allocate virtual memory for its enclave,
> > again subject to all existing LSM policies (sane userspaces map it RO
> > since the permissions eventually get tossed anyways).
>
> Is userspace actually requred to mmap() the enclave prior to EADDing things?
It was a requirement prior to the API rework in v20, i.e. unless someone
was really quick on the draw after the v20 update all existing userspace
implementations mmap() the enclave before ECREATE. Requiring a valid
enclave VMA for EADD shoudn't be too onerous.
> > 4. SGX subsystem refuses to service page faults for enclaves that have
> > not yet been initialized, e.g. signals SIGBUS or SIGSEGV.
> >
> > 5. Userspace invokes SGX ioctl() to copy enclave from regulary VMA to
> > enclave VMA.
> >
> > 6. SGX ioctl() propagates VMA protection-related flags from source VMA
> > to enclave VMA, e.g. invokes mprotect_fixup(). Enclave VMA(s) may
> > be split as part of this process.
>
> Does this also call the LSM? If so, what is it expected to do?
Nope. My reasoning behind skipping LSM checks is that the LSMs have
already ok'd the source VMAs, similar to how dup_mmap() doesn't redo LSM
checks.
> What happens if there are different regions with different permissions on
> the same page? SGX has 256-byte granularity right?
No, EPC pages have 4k granularity.
The EPC is divided into EPC pages. An EPC page is 4KB in size and always
aligned on a 4KB boundary
EEXTEND is the only aspect of SGX that works on 256-byte chunks, and that
goofiness is primarily to keep the latency of EEXTEND low enough so that
the instruction doesn't have to be interruptible, a la EINIT.
> >
> > 7. At all times, mprotect() calls on the enclave VMA are subject to
> > existing LSM policies, i.e. it's not special cased for enclaves.
>
> I don't think the normal behavior actually works here. An enclave is
> always MAP_SHARED, so (with SELinux) mprotecting() to X or RX requires
> EXECUTE and mprotecting() to RWX requires extra permissions.
Requiring extra permissions is good though, right? My thinking is to make
the EADD "VMA copy" the happy/easy path, while using mprotect() to convert
EPC memory to executable would require PROCESS__EXECMEM (assuming we back
enclaves with anon inodes instead of /dev/sgx/enclave).
> But user code can also mmap() the enclave again. What is supposed to
> happen in that case?
Hmm, it can't effectively re-mmap() the enclave as executable since
entering the enclave requires using the correct virtual address range,
i.e. EENTER would fail. It could, I think, do munmap()->mmap() to change
the permissions. We could handle that case fairly easily by invoking
security_file_mprotect() in SGX's mmap() hook if any pages have been added
to the enclave, i.e. treat mmap() like mprotect().
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 15:09 UTC (permalink / raw)
To: Stephen Smalley
Cc: Xing, Cedric, Andy Lutomirski, James Morris, Serge E. Hallyn,
LSM List, Paul Moore, Eric Paris, selinux@vger.kernel.org,
Jarkko Sakkinen, 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: <6a97c099-2f42-672e-a258-95bc09152363@tycho.nsa.gov>
On Fri, May 17, 2019 at 09:53:06AM -0400, Stephen Smalley wrote:
> On 5/16/19 6:23 PM, Xing, Cedric wrote:
> >I thought EXECMOD applied to files (and memory mappings backed by them) but
> >I was probably wrong. It sounds like EXECMOD applies to the whole process so
> >would allow all pages within a process's address space to be modified then
> >executed, regardless the backing files. Am I correct this time?
>
> No, you were correct the first time I think; EXECMOD is used to control
> whether a process can make executable a private file mapping that has
> previously been modified (e.g. text relocation); it is a special case to
> support text relocations without having to allow full EXECMEM (i.e. execute
> arbitrary memory).
>
> SELinux checks relevant to W^X include:
>
> - EXECMEM: mmap/mprotect PROT_EXEC an anonymous mapping (regardless of
> PROT_WRITE, since we know the content has to have been written at some
> point) or a private file mapping that is also PROT_WRITE.
> - EXECMOD: mprotect PROT_EXEC a private file mapping that has been
> previously modified, typically for text relocations,
> - FILE__WRITE: mmap/mprotect PROT_WRITE a shared file mapping,
> - FILE__EXECUTE: mmap/mprotect PROT_EXEC a file mapping.
>
> (ignoring EXECSTACK and EXECHEAP here since they aren't really relevant to
> this discussion)
>
> So if you want to ensure W^X, then you wouldn't allow EXECMEM for the
> process, EXECMOD by the process to any file, and the combination of both
> FILE__WRITE and FILE__EXECUTE by the process to any file.
>
> If the /dev/sgx/enclave mappings are MAP_SHARED and you aren't using an
> anonymous inode, then I would expect that only the FILE__WRITE and
> FILE__EXECUTE checks are relevant.
Yep, I was just typing this up in a different thread:
I think we may want to change the SGX API to alloc an anon inode for each
enclave instead of hanging every enclave off of the /dev/sgx/enclave inode.
Because /dev/sgx/enclave is NOT private, SELinux's file_map_prot_check()
will only require FILE__WRITE and FILE__EXECUTE to mprotect() enclave VMAs
to RWX. Backing each enclave with an anon inode will make SELinux treat
EPC memory like anonymous mappings, which is what we want (I think), e.g.
making *any* EPC page executable will require PROCESS__EXECMEM (SGX is
64-bit only at this point, so SELinux will always have default_noexec).
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-17 14:38 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Christoph Lameter, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <201905161824.63B0DF0E@keescook>
On Fri, May 17, 2019 at 3:26 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 14, 2019 at 04:35:34PM +0200, Alexander Potapenko wrote:
> > [...]
> > diff --git a/mm/slab.h b/mm/slab.h
> > index 43ac818b8592..24ae887359b8 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -524,4 +524,20 @@ static inline int cache_random_seq_create(struct kmem_cache *cachep,
> > [...]
> > +static inline bool slab_want_init_on_free(struct kmem_cache *c)
> > +{
> > + if (static_branch_unlikely(&init_on_free))
> > + return !(c->ctor);
>
> BTW, why is this checking for c->ctor here? Shouldn't it not matter for
> the free case?
It does matter, see e.g. the handling of __OBJECT_POISON in slub.c
If we just return true here, the kernel crashes.
> > + else
> > + return false;
> > +}
>
> --
> Kees Cook
--
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: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-05-17 14:20 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <CAG_fn=W4k=mijnUpF98Hu6P8bFMHU81FHs4Swm+xv1k0wOGFFQ@mail.gmail.com>
On Fri 17-05-19 16:11:32, Alexander Potapenko wrote:
> On Fri, May 17, 2019 at 4:04 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Tue 14-05-19 16:35:34, Alexander Potapenko wrote:
> > > The new options are needed to prevent possible information leaks and
> > > make control-flow bugs that depend on uninitialized values more
> > > deterministic.
> > >
> > > init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> > > objects with zeroes. Initialization is done at allocation time at the
> > > places where checks for __GFP_ZERO are performed.
> > >
> > > init_on_free=1 makes the kernel initialize freed pages and heap objects
> > > with zeroes upon their deletion. This helps to ensure sensitive data
> > > doesn't leak via use-after-free accesses.
> >
> > Why do we need both? The later is more robust because even free memory
> > cannot be sniffed and the overhead might be shifted from the allocation
> > context (e.g. to RCU) but why cannot we stick to a single model?
> init_on_free appears to be slower because of cache effects. It's
> several % in the best case vs. <1% for init_on_alloc.
This doesn't really explain why we need both.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-05-17 14:11 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Kernel Hardening,
Masahiro Yamada, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland,
Linux Memory Management List, linux-security-module
In-Reply-To: <20190517140446.GA8846@dhcp22.suse.cz>
On Fri, May 17, 2019 at 4:04 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Tue 14-05-19 16:35:34, Alexander Potapenko wrote:
> > The new options are needed to prevent possible information leaks and
> > make control-flow bugs that depend on uninitialized values more
> > deterministic.
> >
> > init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> > objects with zeroes. Initialization is done at allocation time at the
> > places where checks for __GFP_ZERO are performed.
> >
> > init_on_free=1 makes the kernel initialize freed pages and heap objects
> > with zeroes upon their deletion. This helps to ensure sensitive data
> > doesn't leak via use-after-free accesses.
>
> Why do we need both? The later is more robust because even free memory
> cannot be sniffed and the overhead might be shifted from the allocation
> context (e.g. to RCU) but why cannot we stick to a single model?
init_on_free appears to be slower because of cache effects. It's
several % in the best case vs. <1% for init_on_alloc.
> --
> Michal Hocko
> SUSE Labs
--
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: [PATCH v2 1/4] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-05-17 14:04 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, keescook, kernel-hardening, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, linux-mm,
linux-security-module
In-Reply-To: <20190514143537.10435-2-glider@google.com>
On Tue 14-05-19 16:35:34, Alexander Potapenko wrote:
> The new options are needed to prevent possible information leaks and
> make control-flow bugs that depend on uninitialized values more
> deterministic.
>
> init_on_alloc=1 makes the kernel initialize newly allocated pages and heap
> objects with zeroes. Initialization is done at allocation time at the
> places where checks for __GFP_ZERO are performed.
>
> init_on_free=1 makes the kernel initialize freed pages and heap objects
> with zeroes upon their deletion. This helps to ensure sensitive data
> doesn't leak via use-after-free accesses.
Why do we need both? The later is more robust because even free memory
cannot be sniffed and the overhead might be shifted from the allocation
context (e.g. to RCU) but why cannot we stick to a single model?
--
Michal Hocko
SUSE Labs
^ 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