* 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
* Re: [PATCH v2 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Michal Hocko @ 2019-05-17 14:01 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, 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: <CAG_fn=Ve88z2ezFjV6CthufMUhJ-ePNMT2=3m6J3nHWh9iSgsg@mail.gmail.com>
On Fri 17-05-19 15:37:14, Alexander Potapenko wrote:
> On Fri, May 17, 2019 at 3:25 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Fri 17-05-19 15:18:19, Alexander Potapenko wrote:
> > > On Fri, May 17, 2019 at 2:59 PM Michal this flag Hocko
> > > <mhocko@kernel.org> wrote:
> > > >
> > > > [It would be great to keep people involved in the previous version in the
> > > > CC list]
> > > Yes, I've been trying to keep everyone in the loop, but your email
> > > fell through the cracks.
> > > Sorry about that.
> >
> > No problem
> >
> > > > On Tue 14-05-19 16:35:36, Alexander Potapenko wrote:
> > > > > When passed to an allocator (either pagealloc or SL[AOU]B),
> > > > > __GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
> > > > > init_on_alloc boot option is enabled. This can be useful in the cases
> > > > > newly allocated memory is going to be initialized by the caller right
> > > > > away.
> > > > >
> > > > > __GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
> > > > > where init_on_free implies init_on_alloc.
> > > > >
> > > > > __GFP_NO_AUTOINIT basically defeats the hardening against information
> > > > > leaks provided by init_on_alloc, so one should use it with caution.
> > > > >
> > > > > This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
> > > > > Doing so is safe, because the heap allocators initialize the pages they
> > > > > receive before passing memory to the callers.
> > > >
> > > > I still do not like the idea of a new gfp flag as explained in the
> > > > previous email. People will simply use it incorectly or arbitrarily.
> > > > We have that juicy experience from the past.
> > >
> > > Just to preserve some context, here's the previous email:
> > > https://patchwork.kernel.org/patch/10907595/
> > > (plus the patch removing GFP_TEMPORARY for the curious ones:
> > > https://lwn.net/Articles/729145/)
> >
> > Not only. GFP_REPEAT being another one and probably others I cannot
> > remember from the top of my head.
> >
> > > > 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?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Stephen Smalley @ 2019-05-17 13:53 UTC (permalink / raw)
To: Xing, Cedric, Andy Lutomirski
Cc: James Morris, Christopherson, Sean J, 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: <960B34DE67B9E140824F1DCDEC400C0F654E3FB9@ORSMSX116.amr.corp.intel.com>
On 5/16/19 6:23 PM, Xing, Cedric wrote:
> Hi Andy,
>
>>> SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.
>> How about the following?
>>> void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
>>> ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
>>>
>>> The idea here is SIGSTRUCT will still be passed in memory so it works
>> the same way when no LSM modules are loaded or basing its decision on
>> the .sigstruct file. Otherwise, an LSM module can figure out the backing
>> file (and offset within that file) by looking into the VMA covering
>> ss_pointer.
>>
>> I don’t love this approach. Application authors seem likely to use
>> read() instead of mmap(), and it’ll still work in many cares. It would
>> also complicate the kernel implementation, and looking at the inode
>> backing the vma that backs a pointer is at least rather unusual.
>> Instead, if the sigstruct isn’t on disk because it’s dynamic or came
>> from a network, the application can put it in a memfd.
>
> I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with. Of course it could be copied to a memfd but whatever "attributes" (e.g. path, or SELinux class/type) associated with the original file would be lost, so I'm not sure if that would work.
>
> I'm also with you that applications tend to use read() instead of mmap() for accessing files. But in our case that'd be necessary only if .sigstruct is a separate file (hence needs to be read separately). What if (and I guess most implementations would) the SIGSTRUCT is embedded in the same file as the enclave? mmap() is the more common practice when dealing with executable images, and in that case SIGSTRUCT will have already been mmap()'d.
>
> I'm with you again that it's kind of unprecedented to look at the backing inode. But I believe we should strive to allow as large variety of applications/usages as possible and I don't see any alternatives without losing flexibility.
>
>>>
>>>>
>>>> /* Actually map the thing */
>>>> mmap(enclave_fd RO section, PROT_READ, ...);
>>>> mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
>>>> mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
>>>>
>>>> /* This should fail unless EXECMOD is available, I think */
>>>> mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
>>>>
>>>> And the idea here is that, if the .enclave file isn't mapped
>>>> PROT_EXEC, then mmapping the RX section will also require EXECMEM or
>>>> EXECMOD.
>>>
>>> From security perspective, I think it reasonable to give EXECMEM and
>> EXECMOD to /dev/sgx/enclave because the actual permissions are guarded
>> by EPCM permissions, which are "inherited" from the source pages, whose
>> permissions have passed LSM checks.
>>
>> I disagree. If you deny a program EXECMOD, it’s not because you
>> distrust the program. It’s because you want to enforce good security
>> practices. (Or you’re Apple and want to disallow third-party JITs.)
>> A policy that accepts any sigstruct but requires that enclaves come
>> from disk and respect W^X seems entirely reasonable.
>>
>> I think that blocking EXECMOD has likely served two very real security
>> purposes. It helps force application and library developers to write
>> and compile their code in a way that doesn’t rely on dangerous tricks
>> like putting executable trampolines on the stack. It also makes it
>> essentially impossible for an exploit to run actual downloaded machine
>> code — if there is no way to run code that isn’t appropriately
>> labeled, then attackers are more limited in what they can do.
>
>>
>> I don’t think that SGX should become an exception to either of these.
>> Code should not have an excuse to use WX memory just because it’s in
>> an enclave. Similarly, an exploit should not be able to run an
>> attacker-supplied enclave as a way around a policy that would
>> otherwise prevent downloaded code from running.
>
> My apology for the confusion here.
>
> 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.
>
> I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.
>
> 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.
>
> -Cedric
>
^ permalink raw reply
* Re: [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Alexander Potapenko @ 2019-05-17 13:50 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: <CAG_fn=Vj6Jk_DY_-0+x6EpbsVh+abpEVcjycBhJxeMH3wuy9rw@mail.gmail.com>
On Fri, May 17, 2019 at 10:49 AM Alexander Potapenko <glider@google.com> 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.
> > everything off
> > Run times: 289.18 288.61 289.66 287.71 287.67
> > Min: 287.67 Max: 289.66 Mean: 288.57 Std Dev: 0.79
> > baseline
> >
> > 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%
I've collected more stats running QEMU with different numbers of cores.
The slowdown values of init_on_free compared to baseline are:
2 CPUs - 5.94% for wall time (20.08% for sys time)
6 CPUs - 7.43% for wall time (23.55% for sys time)
12 CPUs - 8.41% for wall time (24.25% for sys time)
24 CPUs - 9.49% for wall time (17.98% for sys time)
I'm building a defconfig of some fixed KMSAN tree with Clang, but that
shouldn't matter much.
> > > In the commit log it might be worth mentioning that this is only
> > > changing the init_on_alloc case (in case it's not already obvious to
> > > folks). Perhaps there needs to be a split of __GFP_NO_AUTOINIT into
> > > __GFP_NO_AUTO_ALLOC_INIT and __GFP_NO_AUTO_FREE_INIT? Right now
> > > __GFP_NO_AUTOINIT is only checked for init_on_alloc:
> >
> > I was obviously crazy here. :) GFP isn't present for free(), but a SLAB
> > flag works (as was done in PAX_MEMORY_SANITIZE). I'll send the patch I
> > used for the above timing test.
> >
> > --
> > 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
--
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 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Alexander Potapenko @ 2019-05-17 13:37 UTC (permalink / raw)
To: Michal Hocko
Cc: Kees Cook, 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: <20190517132542.GJ6836@dhcp22.suse.cz>
On Fri, May 17, 2019 at 3:25 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Fri 17-05-19 15:18:19, Alexander Potapenko wrote:
> > On Fri, May 17, 2019 at 2:59 PM Michal this flag Hocko
> > <mhocko@kernel.org> wrote:
> > >
> > > [It would be great to keep people involved in the previous version in the
> > > CC list]
> > Yes, I've been trying to keep everyone in the loop, but your email
> > fell through the cracks.
> > Sorry about that.
>
> No problem
>
> > > On Tue 14-05-19 16:35:36, Alexander Potapenko wrote:
> > > > When passed to an allocator (either pagealloc or SL[AOU]B),
> > > > __GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
> > > > init_on_alloc boot option is enabled. This can be useful in the cases
> > > > newly allocated memory is going to be initialized by the caller right
> > > > away.
> > > >
> > > > __GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
> > > > where init_on_free implies init_on_alloc.
> > > >
> > > > __GFP_NO_AUTOINIT basically defeats the hardening against information
> > > > leaks provided by init_on_alloc, so one should use it with caution.
> > > >
> > > > This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
> > > > Doing so is safe, because the heap allocators initialize the pages they
> > > > receive before passing memory to the callers.
> > >
> > > I still do not like the idea of a new gfp flag as explained in the
> > > previous email. People will simply use it incorectly or arbitrarily.
> > > We have that juicy experience from the past.
> >
> > Just to preserve some context, here's the previous email:
> > https://patchwork.kernel.org/patch/10907595/
> > (plus the patch removing GFP_TEMPORARY for the curious ones:
> > https://lwn.net/Articles/729145/)
>
> Not only. GFP_REPEAT being another one and probably others I cannot
> remember from the top of my head.
>
> > > 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 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.
> > However the benchmark results provided in this and the previous patch
> > don't show any noticeable difference - most certainly because the cost
> > of initializing the page is amortized.
>
> > The second one was to fine-tune hackbench, for which the slowdown
> > drops by a factor of 2.
> > But optimizing a mitigation for certain benchmarks is a questionable
> > measure, so maybe we could really go without it.
>
> Agreed. Over optimization based on an artificial workloads tend to be
> dubious IMHO.
>
> --
> 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 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Michal Hocko @ 2019-05-17 13:25 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Kees Cook, 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: <CAG_fn=VG6vrCdpEv0g73M-Au4wW07w8g0uydEiHA96QOfcCVhA@mail.gmail.com>
On Fri 17-05-19 15:18:19, Alexander Potapenko wrote:
> On Fri, May 17, 2019 at 2:59 PM Michal this flag Hocko
> <mhocko@kernel.org> wrote:
> >
> > [It would be great to keep people involved in the previous version in the
> > CC list]
> Yes, I've been trying to keep everyone in the loop, but your email
> fell through the cracks.
> Sorry about that.
No problem
> > On Tue 14-05-19 16:35:36, Alexander Potapenko wrote:
> > > When passed to an allocator (either pagealloc or SL[AOU]B),
> > > __GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
> > > init_on_alloc boot option is enabled. This can be useful in the cases
> > > newly allocated memory is going to be initialized by the caller right
> > > away.
> > >
> > > __GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
> > > where init_on_free implies init_on_alloc.
> > >
> > > __GFP_NO_AUTOINIT basically defeats the hardening against information
> > > leaks provided by init_on_alloc, so one should use it with caution.
> > >
> > > This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
> > > Doing so is safe, because the heap allocators initialize the pages they
> > > receive before passing memory to the callers.
> >
> > I still do not like the idea of a new gfp flag as explained in the
> > previous email. People will simply use it incorectly or arbitrarily.
> > We have that juicy experience from the past.
>
> Just to preserve some context, here's the previous email:
> https://patchwork.kernel.org/patch/10907595/
> (plus the patch removing GFP_TEMPORARY for the curious ones:
> https://lwn.net/Articles/729145/)
Not only. GFP_REPEAT being another one and probably others I cannot
remember from the top of my head.
> > 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.
> > 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?
> However the benchmark results provided in this and the previous patch
> don't show any noticeable difference - most certainly because the cost
> of initializing the page is amortized.
> The second one was to fine-tune hackbench, for which the slowdown
> drops by a factor of 2.
> But optimizing a mitigation for certain benchmarks is a questionable
> measure, so maybe we could really go without it.
Agreed. Over optimization based on an artificial workloads tend to be
dubious IMHO.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Alexander Potapenko @ 2019-05-17 13:18 UTC (permalink / raw)
To: Michal Hocko, 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, Souptick Joarder,
Matthew Wilcox, Linux Memory Management List,
linux-security-module
In-Reply-To: <20190517125916.GF1825@dhcp22.suse.cz>
On Fri, May 17, 2019 at 2:59 PM Michal this flag Hocko
<mhocko@kernel.org> wrote:
>
> [It would be great to keep people involved in the previous version in the
> CC list]
Yes, I've been trying to keep everyone in the loop, but your email
fell through the cracks.
Sorry about that.
> On Tue 14-05-19 16:35:36, Alexander Potapenko wrote:
> > When passed to an allocator (either pagealloc or SL[AOU]B),
> > __GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
> > init_on_alloc boot option is enabled. This can be useful in the cases
> > newly allocated memory is going to be initialized by the caller right
> > away.
> >
> > __GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
> > where init_on_free implies init_on_alloc.
> >
> > __GFP_NO_AUTOINIT basically defeats the hardening against information
> > leaks provided by init_on_alloc, so one should use it with caution.
> >
> > This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
> > Doing so is safe, because the heap allocators initialize the pages they
> > receive before passing memory to the callers.
>
> I still do not like the idea of a new gfp flag as explained in the
> previous email. People will simply use it incorectly or arbitrarily.
> We have that juicy experience from the past.
Just to preserve some context, here's the previous email:
https://patchwork.kernel.org/patch/10907595/
(plus the patch removing GFP_TEMPORARY for the curious ones:
https://lwn.net/Articles/729145/)
> 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?
> 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.
However the benchmark results provided in this and the previous patch
don't show any noticeable difference - most certainly because the cost
of initializing the page is amortized.
The second one was to fine-tune hackbench, for which the slowdown
drops by a factor of 2.
But optimizing a mitigation for certain benchmarks is a questionable
measure, so maybe we could really go without it.
Kees, what do you think?
> --
> 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 3/4] gfp: mm: introduce __GFP_NO_AUTOINIT
From: Michal Hocko @ 2019-05-17 12:59 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, Souptick Joarder,
Matthew Wilcox, linux-mm, linux-security-module
In-Reply-To: <20190514143537.10435-4-glider@google.com>
[It would be great to keep people involved in the previous version in the
CC list]
On Tue 14-05-19 16:35:36, Alexander Potapenko wrote:
> When passed to an allocator (either pagealloc or SL[AOU]B),
> __GFP_NO_AUTOINIT tells it to not initialize the requested memory if the
> init_on_alloc boot option is enabled. This can be useful in the cases
> newly allocated memory is going to be initialized by the caller right
> away.
>
> __GFP_NO_AUTOINIT doesn't affect init_on_free behavior, except for SLOB,
> where init_on_free implies init_on_alloc.
>
> __GFP_NO_AUTOINIT basically defeats the hardening against information
> leaks provided by init_on_alloc, so one should use it with caution.
>
> This patch also adds __GFP_NO_AUTOINIT to alloc_pages() calls in SL[AOU]B.
> Doing so is safe, because the heap allocators initialize the pages they
> receive before passing memory to the callers.
I still do not like the idea of a new gfp flag as explained in the
previous email. People will simply use it incorectly or arbitrarily.
We have that juicy experience from the past.
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.
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?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Alexander Potapenko @ 2019-05-17 8:49 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: <201905161714.A53D472D9@keescook>
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.
> everything off
> Run times: 289.18 288.61 289.66 287.71 287.67
> Min: 287.67 Max: 289.66 Mean: 288.57 Std Dev: 0.79
> baseline
>
> 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%
> > In the commit log it might be worth mentioning that this is only
> > changing the init_on_alloc case (in case it's not already obvious to
> > folks). Perhaps there needs to be a split of __GFP_NO_AUTOINIT into
> > __GFP_NO_AUTO_ALLOC_INIT and __GFP_NO_AUTO_FREE_INIT? Right now
> > __GFP_NO_AUTOINIT is only checked for init_on_alloc:
>
> I was obviously crazy here. :) GFP isn't present for free(), but a SLAB
> flag works (as was done in PAX_MEMORY_SANITIZE). I'll send the patch I
> used for the above timing test.
>
> --
> 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 5/4] mm: Introduce SLAB_NO_FREE_INIT and mark excluded caches
From: Alexander Potapenko @ 2019-05-17 8:34 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: <201905161746.16E885F@keescook>
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.
> This patch is modified from Brad Spengler/PaX Team's code in the
> last public patch of grsecurity/PaX based on my understanding of the
> code. Changes or omissions from the original code are mine and don't
> reflect the original grsecurity/PaX code.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> fs/buffer.c | 2 +-
> fs/dcache.c | 3 ++-
> include/linux/slab.h | 3 +++
> kernel/fork.c | 6 ++++--
> mm/rmap.c | 5 +++--
> mm/slab.h | 5 +++--
> net/core/skbuff.c | 6 ++++--
> 7 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 0faa41fb4c88..04a85bd4cf2e 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -3457,7 +3457,7 @@ void __init buffer_init(void)
> bh_cachep = kmem_cache_create("buffer_head",
> sizeof(struct buffer_head), 0,
> (SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
> - SLAB_MEM_SPREAD),
> + SLAB_MEM_SPREAD|SLAB_NO_FREE_INIT),
> NULL);
>
> /*
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 8136bda27a1f..323b039accba 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -3139,7 +3139,8 @@ void __init vfs_caches_init_early(void)
> void __init vfs_caches_init(void)
> {
> names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
> - SLAB_HWCACHE_ALIGN|SLAB_PANIC, 0, PATH_MAX, NULL);
> + SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NO_FREE_INIT, 0,
> + PATH_MAX, NULL);
>
> dcache_init();
> inode_init();
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 9449b19c5f10..7eba9ad8830d 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -92,6 +92,9 @@
> /* Avoid kmemleak tracing */
> #define SLAB_NOLEAKTRACE ((slab_flags_t __force)0x00800000U)
>
> +/* Exclude slab from zero-on-free when init_on_free is enabled */
> +#define SLAB_NO_FREE_INIT ((slab_flags_t __force)0x01000000U)
> +
> /* Fault injection mark */
> #ifdef CONFIG_FAILSLAB
> # define SLAB_FAILSLAB ((slab_flags_t __force)0x02000000U)
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b4cba953040a..9868585f5520 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2550,11 +2550,13 @@ void __init proc_caches_init(void)
>
> mm_cachep = kmem_cache_create_usercopy("mm_struct",
> mm_size, ARCH_MIN_MMSTRUCT_ALIGN,
> - SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
> + SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT|
> + SLAB_NO_FREE_INIT,
> offsetof(struct mm_struct, saved_auxv),
> sizeof_field(struct mm_struct, saved_auxv),
> NULL);
> - vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
> + vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT|
> + SLAB_NO_FREE_INIT);
> mmap_init();
> nsproxy_cache_init();
> }
> diff --git a/mm/rmap.c b/mm/rmap.c
> index e5dfe2ae6b0d..b7b8013eeb0a 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -432,10 +432,11 @@ static void anon_vma_ctor(void *data)
> void __init anon_vma_init(void)
> {
> anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct anon_vma),
> - 0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
> + 0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT|
> + SLAB_NO_FREE_INIT,
> anon_vma_ctor);
> anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,
> - SLAB_PANIC|SLAB_ACCOUNT);
> + SLAB_PANIC|SLAB_ACCOUNT|SLAB_NO_FREE_INIT);
> }
>
> /*
> diff --git a/mm/slab.h b/mm/slab.h
> index 24ae887359b8..f95b4f03c57b 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -129,7 +129,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
> /* Legal flag mask for kmem_cache_create(), for various configurations */
> #define SLAB_CORE_FLAGS (SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA | \
> SLAB_CACHE_DMA32 | SLAB_PANIC | \
> - SLAB_TYPESAFE_BY_RCU | SLAB_DEBUG_OBJECTS )
> + SLAB_TYPESAFE_BY_RCU | SLAB_DEBUG_OBJECTS | \
> + SLAB_NO_FREE_INIT)
>
> #if defined(CONFIG_DEBUG_SLAB)
> #define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
> @@ -535,7 +536,7 @@ static inline bool slab_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
> static inline bool slab_want_init_on_free(struct kmem_cache *c)
> {
> if (static_branch_unlikely(&init_on_free))
> - return !(c->ctor);
> + return !(c->ctor) && ((c->flags & SLAB_NO_FREE_INIT) == 0);
> else
> return false;
> }
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index e89be6282693..b65902d2c042 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3981,14 +3981,16 @@ void __init skb_init(void)
> skbuff_head_cache = kmem_cache_create_usercopy("skbuff_head_cache",
> sizeof(struct sk_buff),
> 0,
> - SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> + SLAB_HWCACHE_ALIGN|SLAB_PANIC|
> + SLAB_NO_FREE_INIT,
> offsetof(struct sk_buff, cb),
> sizeof_field(struct sk_buff, cb),
> NULL);
> skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
> sizeof(struct sk_buff_fclones),
> 0,
> - SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> + SLAB_HWCACHE_ALIGN|SLAB_PANIC|
> + SLAB_NO_FREE_INIT,
> NULL);
> skb_extensions_init();
> }
> --
> 2.17.1
>
>
> --
> 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: Kees Cook @ 2019-05-17 1:26 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, 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, 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?
> + else
> + return false;
> +}
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 1:21 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, James Morris, Christopherson, Sean J,
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: <960B34DE67B9E140824F1DCDEC400C0F654E40E5@ORSMSX116.amr.corp.intel.com>
On Thu, May 16, 2019 at 6:06 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> > From: Andy Lutomirski [mailto:luto@kernel.org]
> >
> > On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@intel.com>
> > wrote:
> > >
> > > Hi Andy,
> > >
> > > > > SIGSTRUCT isn't necessarily stored on disk so may not always have
> > a fd.
> > > > How about the following?
> > > > > void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> > > > > ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
> > > > >
> > > > > The idea here is SIGSTRUCT will still be passed in memory so it
> > > > > works
> > > > the same way when no LSM modules are loaded or basing its decision
> > > > on the .sigstruct file. Otherwise, an LSM module can figure out the
> > > > backing file (and offset within that file) by looking into the VMA
> > > > covering ss_pointer.
> > > >
> > > > I don’t love this approach. Application authors seem likely to use
> > > > read() instead of mmap(), and it’ll still work in many cares. It
> > > > would also complicate the kernel implementation, and looking at the
> > > > inode backing the vma that backs a pointer is at least rather
> > unusual.
> > > > Instead, if the sigstruct isn’t on disk because it’s dynamic or came
> > > > from a network, the application can put it in a memfd.
> > >
> > > I understand your concern here. But I guess we are making too much
> > assumption on how enclaves are structured/packaged. My concern is, what
> > if a SIGSTRUCT really has to be from memory? For example, an enclave
> > (along with its SIGSTRUCT) could be embedded inside a shared object (or
> > even the "main" executable) so it shows up in memory to begin with.
> >
> > Hmm. That's a fair point, although opening /proc/self/exe could be
> > somewhat of a workaround. It does suffer from a bit of an in-band
> > signaling problem, though, in that it's possible that some other random
> > bytes in the library resemble a SIGSTRUCT.
> >
> > > I was not saying enclaves were exempt to good security practices. What
> > I was trying to say was, EPC pages are *not* subject to the same attacks
> > as regular pages so I suspect there will be a desire to enforce
> > different policies on them, especially after new SGX2
> > features/applications become available. So I think it beneficial to
> > distinguish between regular vs. enclave virtual ranges. And to do that,
> > a new VM_SGX flag in VMA is probably a very simple/easy way. And with
> > that VM_SGX flag, we could add a new security_sgx_mprot() hook so that
> > LSM modules/policies could act differently.
> >
> > I'm not opposed to this, but I also don't think this needs to be in the
> > initial upstream driver. VM_SGX also isn't strictly necessary -- an LSM
> > could inspect the VMA to decide whether it's an SGX VMA if it really
> > wanted to.
>
> VM_SGX is just what I think is the easiest way for any module to tell enclave VMAs from all others. I agree totally with you that doesn't have to be in the initial release!
>
> >
> > That being said, do you have any specific behavior differences in mind
> > aside from the oddities involved in loading the enclave.
>
> The major thing is dynamically linked enclaves. Say if you want something like dlopen() inside an enclave, the driver would need to EAUG a page as RW initially, and then change to RX after it has been EACCEPTCOPY'ed by the enclave. So it's like a RW->RX transition and an LSM module/policy may want to allow it only if it's within an enclave range (ELRANGE), or deny it otherwise.
I'm not convinced. Given that the kernel has no way to tell that the
dynamically loaded code wasn't dynamically generated, I don't think it
makes sense to allow this in an enclave but disallow it outside an
enclave.
>
> >
> > >
> > > 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.
>
> This is exactly what I'm going after!
>
> But I have to apologize for this silly question because I don't know much about SELinux: Wouldn't it require changes to existing SELinux policies to *not* restrict mprotect() on /dev/sgx/enclave?
I'm assuming we'd make a small in-kernel change to SELinux to make it
work without policy changes, assuming the SELinux maintainers would be
okay with this.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-17 1:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, Christopherson, Sean J, 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>
> From: Andy Lutomirski [mailto:luto@kernel.org]
>
> On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@intel.com>
> wrote:
> >
> > Hi Andy,
> >
> > > > SIGSTRUCT isn't necessarily stored on disk so may not always have
> a fd.
> > > How about the following?
> > > > void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> > > > ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
> > > >
> > > > The idea here is SIGSTRUCT will still be passed in memory so it
> > > > works
> > > the same way when no LSM modules are loaded or basing its decision
> > > on the .sigstruct file. Otherwise, an LSM module can figure out the
> > > backing file (and offset within that file) by looking into the VMA
> > > covering ss_pointer.
> > >
> > > I don’t love this approach. Application authors seem likely to use
> > > read() instead of mmap(), and it’ll still work in many cares. It
> > > would also complicate the kernel implementation, and looking at the
> > > inode backing the vma that backs a pointer is at least rather
> unusual.
> > > Instead, if the sigstruct isn’t on disk because it’s dynamic or came
> > > from a network, the application can put it in a memfd.
> >
> > I understand your concern here. But I guess we are making too much
> assumption on how enclaves are structured/packaged. My concern is, what
> if a SIGSTRUCT really has to be from memory? For example, an enclave
> (along with its SIGSTRUCT) could be embedded inside a shared object (or
> even the "main" executable) so it shows up in memory to begin with.
>
> Hmm. That's a fair point, although opening /proc/self/exe could be
> somewhat of a workaround. It does suffer from a bit of an in-band
> signaling problem, though, in that it's possible that some other random
> bytes in the library resemble a SIGSTRUCT.
>
> > I was not saying enclaves were exempt to good security practices. What
> I was trying to say was, EPC pages are *not* subject to the same attacks
> as regular pages so I suspect there will be a desire to enforce
> different policies on them, especially after new SGX2
> features/applications become available. So I think it beneficial to
> distinguish between regular vs. enclave virtual ranges. And to do that,
> a new VM_SGX flag in VMA is probably a very simple/easy way. And with
> that VM_SGX flag, we could add a new security_sgx_mprot() hook so that
> LSM modules/policies could act differently.
>
> I'm not opposed to this, but I also don't think this needs to be in the
> initial upstream driver. VM_SGX also isn't strictly necessary -- an LSM
> could inspect the VMA to decide whether it's an SGX VMA if it really
> wanted to.
VM_SGX is just what I think is the easiest way for any module to tell enclave VMAs from all others. I agree totally with you that doesn't have to be in the initial release!
>
> That being said, do you have any specific behavior differences in mind
> aside from the oddities involved in loading the enclave.
The major thing is dynamically linked enclaves. Say if you want something like dlopen() inside an enclave, the driver would need to EAUG a page as RW initially, and then change to RX after it has been EACCEPTCOPY'ed by the enclave. So it's like a RW->RX transition and an LSM module/policy may want to allow it only if it's within an enclave range (ELRANGE), or deny it otherwise.
>
> >
> > 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.
This is exactly what I'm going after!
But I have to apologize for this silly question because I don't know much about SELinux: Wouldn't it require changes to existing SELinux policies to *not* restrict mprotect() on /dev/sgx/enclave?
-Cedric
^ permalink raw reply
* [PATCH 5/4] mm: Introduce SLAB_NO_FREE_INIT and mark excluded caches
From: Kees Cook @ 2019-05-17 0:50 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, 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-5-glider@google.com>
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.
This patch is modified from Brad Spengler/PaX Team's code in the
last public patch of grsecurity/PaX based on my understanding of the
code. Changes or omissions from the original code are mine and don't
reflect the original grsecurity/PaX code.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/buffer.c | 2 +-
fs/dcache.c | 3 ++-
include/linux/slab.h | 3 +++
kernel/fork.c | 6 ++++--
mm/rmap.c | 5 +++--
mm/slab.h | 5 +++--
net/core/skbuff.c | 6 ++++--
7 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 0faa41fb4c88..04a85bd4cf2e 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -3457,7 +3457,7 @@ void __init buffer_init(void)
bh_cachep = kmem_cache_create("buffer_head",
sizeof(struct buffer_head), 0,
(SLAB_RECLAIM_ACCOUNT|SLAB_PANIC|
- SLAB_MEM_SPREAD),
+ SLAB_MEM_SPREAD|SLAB_NO_FREE_INIT),
NULL);
/*
diff --git a/fs/dcache.c b/fs/dcache.c
index 8136bda27a1f..323b039accba 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3139,7 +3139,8 @@ void __init vfs_caches_init_early(void)
void __init vfs_caches_init(void)
{
names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC, 0, PATH_MAX, NULL);
+ SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_NO_FREE_INIT, 0,
+ PATH_MAX, NULL);
dcache_init();
inode_init();
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 9449b19c5f10..7eba9ad8830d 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -92,6 +92,9 @@
/* Avoid kmemleak tracing */
#define SLAB_NOLEAKTRACE ((slab_flags_t __force)0x00800000U)
+/* Exclude slab from zero-on-free when init_on_free is enabled */
+#define SLAB_NO_FREE_INIT ((slab_flags_t __force)0x01000000U)
+
/* Fault injection mark */
#ifdef CONFIG_FAILSLAB
# define SLAB_FAILSLAB ((slab_flags_t __force)0x02000000U)
diff --git a/kernel/fork.c b/kernel/fork.c
index b4cba953040a..9868585f5520 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2550,11 +2550,13 @@ void __init proc_caches_init(void)
mm_cachep = kmem_cache_create_usercopy("mm_struct",
mm_size, ARCH_MIN_MMSTRUCT_ALIGN,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT,
+ SLAB_HWCACHE_ALIGN|SLAB_PANIC|SLAB_ACCOUNT|
+ SLAB_NO_FREE_INIT,
offsetof(struct mm_struct, saved_auxv),
sizeof_field(struct mm_struct, saved_auxv),
NULL);
- vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT);
+ vm_area_cachep = KMEM_CACHE(vm_area_struct, SLAB_PANIC|SLAB_ACCOUNT|
+ SLAB_NO_FREE_INIT);
mmap_init();
nsproxy_cache_init();
}
diff --git a/mm/rmap.c b/mm/rmap.c
index e5dfe2ae6b0d..b7b8013eeb0a 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -432,10 +432,11 @@ static void anon_vma_ctor(void *data)
void __init anon_vma_init(void)
{
anon_vma_cachep = kmem_cache_create("anon_vma", sizeof(struct anon_vma),
- 0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT,
+ 0, SLAB_TYPESAFE_BY_RCU|SLAB_PANIC|SLAB_ACCOUNT|
+ SLAB_NO_FREE_INIT,
anon_vma_ctor);
anon_vma_chain_cachep = KMEM_CACHE(anon_vma_chain,
- SLAB_PANIC|SLAB_ACCOUNT);
+ SLAB_PANIC|SLAB_ACCOUNT|SLAB_NO_FREE_INIT);
}
/*
diff --git a/mm/slab.h b/mm/slab.h
index 24ae887359b8..f95b4f03c57b 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -129,7 +129,8 @@ static inline slab_flags_t kmem_cache_flags(unsigned int object_size,
/* Legal flag mask for kmem_cache_create(), for various configurations */
#define SLAB_CORE_FLAGS (SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA | \
SLAB_CACHE_DMA32 | SLAB_PANIC | \
- SLAB_TYPESAFE_BY_RCU | SLAB_DEBUG_OBJECTS )
+ SLAB_TYPESAFE_BY_RCU | SLAB_DEBUG_OBJECTS | \
+ SLAB_NO_FREE_INIT)
#if defined(CONFIG_DEBUG_SLAB)
#define SLAB_DEBUG_FLAGS (SLAB_RED_ZONE | SLAB_POISON | SLAB_STORE_USER)
@@ -535,7 +536,7 @@ static inline bool slab_want_init_on_alloc(gfp_t flags, struct kmem_cache *c)
static inline bool slab_want_init_on_free(struct kmem_cache *c)
{
if (static_branch_unlikely(&init_on_free))
- return !(c->ctor);
+ return !(c->ctor) && ((c->flags & SLAB_NO_FREE_INIT) == 0);
else
return false;
}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e89be6282693..b65902d2c042 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3981,14 +3981,16 @@ void __init skb_init(void)
skbuff_head_cache = kmem_cache_create_usercopy("skbuff_head_cache",
sizeof(struct sk_buff),
0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+ SLAB_HWCACHE_ALIGN|SLAB_PANIC|
+ SLAB_NO_FREE_INIT,
offsetof(struct sk_buff, cb),
sizeof_field(struct sk_buff, cb),
NULL);
skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
sizeof(struct sk_buff_fclones),
0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+ SLAB_HWCACHE_ALIGN|SLAB_PANIC|
+ SLAB_NO_FREE_INIT,
NULL);
skb_extensions_init();
}
--
2.17.1
--
Kees Cook
^ permalink raw reply related
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 0:35 UTC (permalink / raw)
To: Xing, Cedric
Cc: Andy Lutomirski, James Morris, Christopherson, Sean J,
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: <960B34DE67B9E140824F1DCDEC400C0F654E3FB9@ORSMSX116.amr.corp.intel.com>
On Thu, May 16, 2019 at 3:23 PM Xing, Cedric <cedric.xing@intel.com> wrote:
>
> Hi Andy,
>
> > > SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.
> > How about the following?
> > > void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> > > ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
> > >
> > > The idea here is SIGSTRUCT will still be passed in memory so it works
> > the same way when no LSM modules are loaded or basing its decision on
> > the .sigstruct file. Otherwise, an LSM module can figure out the backing
> > file (and offset within that file) by looking into the VMA covering
> > ss_pointer.
> >
> > I don’t love this approach. Application authors seem likely to use
> > read() instead of mmap(), and it’ll still work in many cares. It would
> > also complicate the kernel implementation, and looking at the inode
> > backing the vma that backs a pointer is at least rather unusual.
> > Instead, if the sigstruct isn’t on disk because it’s dynamic or came
> > from a network, the application can put it in a memfd.
>
> I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with.
Hmm. That's a fair point, although opening /proc/self/exe could be
somewhat of a workaround. It does suffer from a bit of an in-band
signaling problem, though, in that it's possible that some other
random bytes in the library resemble a SIGSTRUCT.
> I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.
I'm not opposed to this, but I also don't think this needs to be in
the initial upstream driver. VM_SGX also isn't strictly necessary --
an LSM could inspect the VMA to decide whether it's an SGX VMA if it
really wanted to.
That being said, do you have any specific behavior differences in mind
aside from the oddities involved in loading the enclave.
>
> 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.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-17 0:26 UTC (permalink / raw)
To: Sean Christopherson
Cc: Andy Lutomirski, 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: <20190517000331.GD11204@linux.intel.com>
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?
>
> 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? What
happens if there are different regions with different permissions on
the same page? SGX has 256-byte granularity right?
>
> 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. But user
code can also mmap() the enclave again. What is supposed to happen in
that case?
^ 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 0:26 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, 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: <201905160923.BD3E530EFC@keescook>
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:
everything off
Run times: 289.18 288.61 289.66 287.71 287.67
Min: 287.67 Max: 289.66 Mean: 288.57 Std Dev: 0.79
baseline
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.
> In the commit log it might be worth mentioning that this is only
> changing the init_on_alloc case (in case it's not already obvious to
> folks). Perhaps there needs to be a split of __GFP_NO_AUTOINIT into
> __GFP_NO_AUTO_ALLOC_INIT and __GFP_NO_AUTO_FREE_INIT? Right now
> __GFP_NO_AUTOINIT is only checked for init_on_alloc:
I was obviously crazy here. :) GFP isn't present for free(), but a SLAB
flag works (as was done in PAX_MEMORY_SANITIZE). I'll send the patch I
used for the above timing test.
--
Kees Cook
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-17 0:03 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: <CALCETrXf8mSK45h7sTK5Wf+pXLVn=Bjsc_RLpgO-h-qdzBRo5Q@mail.gmail.com>
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? 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).
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.
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.
The SGX ioctl() would need to take mmap_sem for write, but we can mitigate
that issue by changing the ioctl() to take a range of memory instead of a
single page. That'd also provide "EADD batching" that folks have
requested.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-16 23:29 UTC (permalink / raw)
To: Christopherson, Sean J, Andy Lutomirski
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux@vger.kernel.org,
Jethro Beekman, Hansen, Dave, Thomas Gleixner, Dr. Greg,
Linus Torvalds, LKML, X86 ML, linux-sgx@vger.kernel.org,
Andrew Morton, nhorman@redhat.com, npmccallum@redhat.com,
Ayoun, Serge, Katz-zamir, Shay, Huang, Haitao, Andy Shevchenko,
Svahn, Kai, Borislav Petkov, Josh Triplett, Huang, Kai,
David Rientjes
In-Reply-To: <20190516224550.GC11204@linux.intel.com>
> > > There is a problem here though. Usually the enclave itself is just a
> > > loader that then loads the application from outside source and
> > > creates the executable pages from the content.
> > >
> > > A great example of this is Graphene that bootstraps unmodified Linux
> > > applications to an enclave:
> > >
> > > https://github.com/oscarlab/graphene
> > >
> >
> > ISTM you should need EXECMEM or similar to run Graphene, then.
>
> Agreed, Graphene is effectively running arbitrary enclave code. I'm
> guessing there is nothing that prevents extending/reworking Graphene to
> allow generating the enclave ahead of time so as to avoid populating the
> guts of the enclave at runtime, i.e. it's likely possible to run an
> unmodified application in an enclave without EXECMEM if that's something
> Graphene or its users really care about.
Inefficient use of memory is a problem of running Graphene on SGX1, from at least 2 aspects: 1) heaps/stacks have to be pre-allocated but only a small portion of those pages will be actually used; and 2) dynamic linking is commonly used in *unmodified* applications and all dependent libraries have to be loaded, but only a subset of those pages will actually be used - e.g. most applications use only a small set of functions in libc.so but the whole library still has to be loaded. Hence a practical/efficient solution will require/involve EDMM features available in SGX2. I guess we shall look a bit further into future in order to address this problem properly. And I think it necessary to distinguish enclave virtual ranges from regular ones (probably at VMA level) before we could have a practical solution.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Sean Christopherson @ 2019-05-16 22:45 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, LSM List,
Paul Moore, Stephen Smalley, Eric Paris, selinux, 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: <CALCETrVx1hgY67mP+73w5rT+eY+APcfS0YJ+XwtTLNz3CbVNMA@mail.gmail.com>
On Thu, May 16, 2019 at 02:02:58PM -0700, Andy Lutomirski wrote:
> > On May 15, 2019, at 10:16 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > There is a problem here though. Usually the enclave itself is just a
> > loader that then loads the application from outside source and creates
> > the executable pages from the content.
> >
> > A great example of this is Graphene that bootstraps unmodified Linux
> > applications to an enclave:
> >
> > https://github.com/oscarlab/graphene
> >
>
> ISTM you should need EXECMEM or similar to run Graphene, then.
Agreed, Graphene is effectively running arbitrary enclave code. I'm
guessing there is nothing that prevents extending/reworking Graphene to
allow generating the enclave ahead of time so as to avoid populating the
guts of the enclave at runtime, i.e. it's likely possible to run an
unmodified application in an enclave without EXECMEM if that's something
Graphene or its users really care about.
^ permalink raw reply
* RE: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Xing, Cedric @ 2019-05-16 22:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: James Morris, Christopherson, Sean J, 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: <CALCETrUfmyQ7ivNzQic0FyPXe1fmAnoK093jnz0i8DRn2LvdSA@mail.gmail.com>
Hi Andy,
> > SIGSTRUCT isn't necessarily stored on disk so may not always have a fd.
> How about the following?
> > void *ss_pointer = mmap(sigstruct_fd, PROT_READ,...);
> > ioctl(enclave_fd, SGX_INIT_THE_ENCLAVE, ss_pointer);
> >
> > The idea here is SIGSTRUCT will still be passed in memory so it works
> the same way when no LSM modules are loaded or basing its decision on
> the .sigstruct file. Otherwise, an LSM module can figure out the backing
> file (and offset within that file) by looking into the VMA covering
> ss_pointer.
>
> I don’t love this approach. Application authors seem likely to use
> read() instead of mmap(), and it’ll still work in many cares. It would
> also complicate the kernel implementation, and looking at the inode
> backing the vma that backs a pointer is at least rather unusual.
> Instead, if the sigstruct isn’t on disk because it’s dynamic or came
> from a network, the application can put it in a memfd.
I understand your concern here. But I guess we are making too much assumption on how enclaves are structured/packaged. My concern is, what if a SIGSTRUCT really has to be from memory? For example, an enclave (along with its SIGSTRUCT) could be embedded inside a shared object (or even the "main" executable) so it shows up in memory to begin with. Of course it could be copied to a memfd but whatever "attributes" (e.g. path, or SELinux class/type) associated with the original file would be lost, so I'm not sure if that would work.
I'm also with you that applications tend to use read() instead of mmap() for accessing files. But in our case that'd be necessary only if .sigstruct is a separate file (hence needs to be read separately). What if (and I guess most implementations would) the SIGSTRUCT is embedded in the same file as the enclave? mmap() is the more common practice when dealing with executable images, and in that case SIGSTRUCT will have already been mmap()'d.
I'm with you again that it's kind of unprecedented to look at the backing inode. But I believe we should strive to allow as large variety of applications/usages as possible and I don't see any alternatives without losing flexibility.
> >
> >>
> >> /* Actually map the thing */
> >> mmap(enclave_fd RO section, PROT_READ, ...);
> >> mmap(enclave_fd RW section, PROT_READ | PROT_WRITE, ...);
> >> mmap(enclave_fd RX section, PROT_READ | PROT_EXEC, ...);
> >>
> >> /* This should fail unless EXECMOD is available, I think */
> >> mmap(enclave_fd RWX section, PROT_READ | PROT_WRITE | PROT_EXEC);
> >>
> >> And the idea here is that, if the .enclave file isn't mapped
> >> PROT_EXEC, then mmapping the RX section will also require EXECMEM or
> >> EXECMOD.
> >
> > From security perspective, I think it reasonable to give EXECMEM and
> EXECMOD to /dev/sgx/enclave because the actual permissions are guarded
> by EPCM permissions, which are "inherited" from the source pages, whose
> permissions have passed LSM checks.
>
> I disagree. If you deny a program EXECMOD, it’s not because you
> distrust the program. It’s because you want to enforce good security
> practices. (Or you’re Apple and want to disallow third-party JITs.)
> A policy that accepts any sigstruct but requires that enclaves come
> from disk and respect W^X seems entirely reasonable.
>
> I think that blocking EXECMOD has likely served two very real security
> purposes. It helps force application and library developers to write
> and compile their code in a way that doesn’t rely on dangerous tricks
> like putting executable trampolines on the stack. It also makes it
> essentially impossible for an exploit to run actual downloaded machine
> code — if there is no way to run code that isn’t appropriately
> labeled, then attackers are more limited in what they can do.
>
> I don’t think that SGX should become an exception to either of these.
> Code should not have an excuse to use WX memory just because it’s in
> an enclave. Similarly, an exploit should not be able to run an
> attacker-supplied enclave as a way around a policy that would
> otherwise prevent downloaded code from running.
My apology for the confusion here.
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?
I was not saying enclaves were exempt to good security practices. What I was trying to say was, EPC pages are *not* subject to the same attacks as regular pages so I suspect there will be a desire to enforce different policies on them, especially after new SGX2 features/applications become available. So I think it beneficial to distinguish between regular vs. enclave virtual ranges. And to do that, a new VM_SGX flag in VMA is probably a very simple/easy way. And with that VM_SGX flag, we could add a new security_sgx_mprot() hook so that LSM modules/policies could act differently.
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.
-Cedric
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-16 21:02 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Andy Lutomirski, Sean Christopherson, James Morris,
Serge E. Hallyn, LSM List, Paul Moore, Stephen Smalley,
Eric Paris, selinux, 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: <20190516051622.GC6388@linux.intel.com>
> On May 15, 2019, at 10:16 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
>> On Wed, May 15, 2019 at 11:27:04AM -0700, Andy Lutomirski wrote:
>> Hi, LSM and SELinux people-
>>
>> We're trying to figure out how SGX fits in with LSMs. For background,
>> an SGX library is functionally a bit like a DSO, except that it's
>> nominally resistant to attack from outside and the process of loading
>> it is complicated. To load an enclave, a program can open
>> /dev/sgx/enclave, do some ioctls to load the code and data segments
>> into the enclave, call a special ioctl to "initialize" the enclave,
>> and then call into the enclave (using special CPU instructions).
>>
>> One nastiness is that there is not actually a universally agreed upon,
>> documented file format for enclaves. Windows has an undocumented
>> format, and there are probably a few others out there. No one really
>> wants to teach the kernel to parse enclave files.
>>
>> There are two issues with how this interacts with LSMs:
>>
>> 1) LSMs might want to be able to whitelist, blacklist, or otherwise
>> restrict what enclaves can run at all. The current proposal that
>> everyone seems to dislike the least is to have a .sigstruct file on
>> disk that contains a hash and signature of the enclave in a
>> CPU-defined format. To initialize an enclave, a program will pass an
>> fd to this file, and a new LSM hook can be called to allow or disallow
>> the operation. In a SELinux context, the idea is that policy could
>> require the .sigstruct file to be labeled with a type like
>> sgx_sigstruct_t, and only enclaves that have a matching .sigstruct
>> with such a label could run.
>
> Similarly if we could take data for the enclave from fd and enforce
> it with sgx_enclave_t label.
That certainly *could* be done, and I guess the decision could be left
to the LSMs, but I'm not convinced this adds value. What security use
case does this cover that isn't already covered by requiring EXECUTE
(e.g. lib_t) on the enclave file and some new SIGSTRUCT right on the
.sigstruct?
>
>> 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.
>
> With the fd based model for source I'd mark SECINFO.W pages as unsafe
> to execute and then check unsafe bit before applying lets say EMODT
> or EMODPR.
>
> There is a problem here though. Usually the enclave itself is just a
> loader that then loads the application from outside source and creates
> the executable pages from the content.
>
> A great example of this is Graphene that bootstraps unmodified Linux
> applications to an enclave:
>
> https://github.com/oscarlab/graphene
>
ISTM you should need EXECMEM or similar to run Graphene, then.
^ permalink raw reply
* Re: SGX vs LSM (Re: [PATCH v20 00/28] Intel SGX1 support)
From: Andy Lutomirski @ 2019-05-16 21:00 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Sean Christopherson, 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: <alpine.LRH.2.21.1905161716460.23647@namei.org>
> On May 16, 2019, at 12:24 AM, James Morris <jmorris@namei.org> wrote:
>
>> On Wed, 15 May 2019, Andy Lutomirski wrote:
>>
>>> On Wed, May 15, 2019 at 3:46 PM James Morris <jmorris@namei.org> wrote:
>>>
>>> You could try user.sigstruct, which does not require any privs.
>>>
>>
>> I don't think I understand your proposal. What file would this
>> attribute be on? What would consume it?
>
> It would be on the enclave file, so you keep the sigstruct bound to it,
> rather than needing a separate file to manage. It would simplify any LSM
> policy check.
>
> It would be consumed by (I guess) the SGX_INIT_THE_ENCLAVE ioctl in your
> example, instead of having a 2nd fd.
>
>
Okay, I think I see what you’re suggesting. I don’t think it works
well, though, since loading the data from the enclave file will almost
always be done in multiple chunks, and it’s not clear when the kernel
should look for the xattr or what to do if the xattr changes part way
through.
^ 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-16 17:03 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=VsJmyuEUYy16R_M5Hu2CX-PJkz9Kw4rdy9XUCAYHwV5g@mail.gmail.com>
On Thu, May 16, 2019 at 06:42:37PM +0200, Alexander Potapenko wrote:
> I suspect the slowdown of init_on_free is bigger than that of
> PAX_SANITIZE_MEMORY, as we've set the goal to have fully zeroed memory
> at alloc time.
> If we want a mode that only wipes the user data upon free() but
> doesn't eliminate all uninit memory, then we can make it faster.
Yeah, I sent a separate email that discusses this a bit more.
I think the goals of init_on_alloc and init_on_free are likely a bit
different. Given init_on_alloc's much more cache-friendly performance,
I think that it's likely the right way forward for getting to fully zeroed
memory at alloc time. (Though I note that it already includes exclusions:
such tradeoffs won't be unique to init_on_free.)
init_on_free appears to give us similar coverage (but also reduces the
lifetime of unused data), but isn't cache-friendly so it looks to need
a lot more tuning/trade-offs. (Not that we shouldn't include it! It'll
just need a bit more care to be reasonable.)
> > +void __init report_meminit(void)
> > +{
> > + const char *stack;
> > +
> > + if (IS_ENABLED(CONFIG_INIT_STACK_ALL))
> > + stack = "all";
> > + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL))
> > + stack = "byref_all";
> > + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF))
> > + stack = "byref";
> > + else if (IS_ENABLED(CONFIG_GCC_PLUGIN_STRUCTLEAK_USER))
> > + stack = "__user";
> > + else
> > + stack = "off";
> > +
> > + /* Report memory auto-initialization states for this boot. */
> > + pr_info("mem auto-init: stack:%s, heap alloc:%s, heap free:%s\n",
> > + stack, want_init_on_alloc(GFP_KERNEL) ? "on" : "off",
> > + want_init_on_free() ? "on" : "off");
> > +}
> >
> > To get a boot line like:
> >
> > mem auto-init: stack:off, heap alloc:off, heap free:on
> For stack there's no binary on/off, as you can potentially build half
> of the kernel with stack instrumentation and another half without it.
> We could make the instrumentation insert a static global flag into
> each translation unit, but this won't give us any interesting info.
Well, yes, that's technically true, but I think reporting the kernel
config here would make sense. If someone intentionally bypasses the
stack auto-init for portions of the kernel, we can't meaningfully report
it here. There will be exceptions for stack auto-init and heap auto-init.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2 4/4] net: apply __GFP_NO_AUTOINIT to AF_UNIX sk_buff allocations
From: Kees Cook @ 2019-05-16 16:53 UTC (permalink / raw)
To: Alexander Potapenko
Cc: akpm, cl, 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-5-glider@google.com>
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%)
In the commit log it might be worth mentioning that this is only
changing the init_on_alloc case (in case it's not already obvious to
folks). Perhaps there needs to be a split of __GFP_NO_AUTOINIT into
__GFP_NO_AUTO_ALLOC_INIT and __GFP_NO_AUTO_FREE_INIT? Right now __GFP_NO_AUTOINIT is only checked for init_on_alloc:
static inline bool want_init_on_alloc(gfp_t flags)
{
if (static_branch_unlikely(&init_on_alloc))
return !(flags & __GFP_NO_AUTOINIT);
return flags & __GFP_ZERO;
}
...
static inline bool want_init_on_free(void)
{
return static_branch_unlikely(&init_on_free);
}
On a related note, it might be nice to add an exclusion list to
the kmem_cache_create() cases, since it seems likely that further
tuning will be needed there. For example, with the init_on_free-similar
PAX_MEMORY_SANITIZE changes in the last public release of PaX/grsecurity,
the following were excluded from wipe-on-free:
buffer_head
names_cache
mm_struct
vm_area_struct
anon_vma
anon_vma_chain
skbuff_head_cache
skbuff_fclone_cache
Adding these and others (with details on why they were selected),
might improve init_on_free performance further without trading too
much coverage.
Having a kernel param with a comma-separated list of cache names and
the logic to add __GFP_NO_AUTOINIT at creation time would be a nice
(and cheap!) debug feature to let folks tune things for their specific
workloads, if they choose to. (And it could maybe also know what "none"
meant, to actually remove the built-in exclusions, similar to what
PaX's "pax_sanitize_slab=full" does.)
--
Kees Cook
^ 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