Re: [PATCH] mm: security: introduce CONFIG_INIT_HEAP_ALL

[Alexander Potapenko <glider@google.com>]

On Wed, Apr 17, 2019 at 1:03 PM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Apr 16, 2019 at 6:30 PM Christopher Lameter <cl@linux.com> wrote:
> >
> > On Tue, 16 Apr 2019, Alexander Potapenko wrote:
> >
> > > > Hmmm... But we already have debugging options that poison objects and
> > > > pages?
> > > Laura Abbott mentioned in one of the previous threads
> > > (https://marc.info/?l=kernel-hardening&m=155474181528491&w=2) that:
> > >
> > > """
> > > I've looked at doing something similar in the past (failing to find
> > > the thread this morning...) and while this will work, it has pretty
> > > serious performance issues. It's not actually the poisoning which
> > > is expensive but that turning on debugging removes the cpu slab
> > > which has significant performance penalties.
> >
> > Ok you could rework that logic to be able to keep the per cpu slabs?
> I'll look into that. There's a lot going on with checking those
> poisoned bytes, although we don't need that for hardening.
>
> What do you think about the proposed approach to page initialization?
> We could separate that part from slab poisoning.
>
> > Also if you do the zeroing then you need to do it in the hotpath. And this
> > patch introduces new instructions to that hotpath for checking and
> > executing the zeroing.
> Right now the patch doesn't slow down the default case when
> CONFIG_INIT_HEAP_ALL=n, as GFP_INIT_ALWAYS_ON is 0.
> In the case heap initialization is enabled we could probably omit the
> gfp_flags check, as it'll be always zero in the case there's a
> constructor or RCU flag is set.
> So we'll have two branches instead of one in the case CONFIG_INIT_HEAP_ALL=y.
>
Ok, I think we could do the same without extra branches.
Right now I'm working on a patch that uses static branches in the
function that checks GFP flags:

static inline bool want_init_memory(gfp_t flags)
{
        if (static_branch_unlikely(&init_allocations))
                return true;
        return flags & __GFP_ZERO;
}

and does the following in slab_alloc_node():

        if (unlikely(want_init_memory(gfpflags)) && object)
                s->poison_fn(s, object);
, where s->poison_fn is either memset(object, 0, s->object_size) for
normal SLAB caches or a no-op for SLAB caches that have ctors
(I _think_ I don't have to special-case SLAB_TYPESAFE_BY_RCU).

With init_allocations disabled this doesn't affect the kernel
performance (hackbench shows negative slowdown within the standard
deviation). Most certainly the indirect call is performed not too
often.
With init_allocations enabled this yields ~7% slowdown on hackbench. I
believe most of that is caused by double initialization, which we can
eliminate by passing an extra GFP flag to the page allocator.

>
> --
> 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