* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-06-21 9:11 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=UFj0Lzy3FgMV_JBKtxCiwE03HVxnR8=f9a7=4nrUFXSw@mail.gmail.com>
On Fri 21-06-19 10:57:35, Alexander Potapenko wrote:
> On Fri, Jun 21, 2019 at 9:09 AM Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index fd5c95ff9251..2f75dd0d0d81 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
> > > arch_kexec_post_alloc_pages(page_address(pages), count,
> > > gfp_mask);
> > >
> > > - if (gfp_mask & __GFP_ZERO)
> > > + if (want_init_on_alloc(gfp_mask))
> > > for (i = 0; i < count; i++)
> > > clear_highpage(pages + i);
> > > }
> >
> > I am not really sure I follow here. Why do we want to handle
> > want_init_on_alloc here? The allocated memory comes from the page
> > allocator and so it will get zeroed there. arch_kexec_post_alloc_pages
> > might touch the content there but is there any actual risk of any kind
> > of leak?
> You're right, we don't want to initialize this memory if init_on_alloc is on.
> We need something along the lines of:
> if (!static_branch_unlikely(&init_on_alloc))
> if (gfp_mask & __GFP_ZERO)
> // clear the pages
>
> Another option would be to disable initialization in alloc_pages() using a flag.
Or we can simply not care and keen the code the way it is. First of all
it seems that nobody actually does use __GFP_ZERO unless I have missed
soemthing
- kimage_alloc_pages(KEXEC_CONTROL_MEMORY_GFP, order); # GFP_KERNEL | __GFP_NORETRY
- kimage_alloc_pages(gfp_mask, 0);
- kimage_alloc_page(image, GFP_KERNEL, KIMAGE_NO_DEST);
- kimage_alloc_page(image, GFP_HIGHUSER, maddr);
but even if we actually had a user do we care about double intialization
for something kexec related? It is not any hot path AFAIR.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-06-21 9:18 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190621091159.GD3429@dhcp22.suse.cz>
On Fri, Jun 21, 2019 at 11:12 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Fri 21-06-19 10:57:35, Alexander Potapenko wrote:
> > On Fri, Jun 21, 2019 at 9:09 AM Michal Hocko <mhocko@kernel.org> wrote:
> [...]
> > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > > index fd5c95ff9251..2f75dd0d0d81 100644
> > > > --- a/kernel/kexec_core.c
> > > > +++ b/kernel/kexec_core.c
> > > > @@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
> > > > arch_kexec_post_alloc_pages(page_address(pages), count,
> > > > gfp_mask);
> > > >
> > > > - if (gfp_mask & __GFP_ZERO)
> > > > + if (want_init_on_alloc(gfp_mask))
> > > > for (i = 0; i < count; i++)
> > > > clear_highpage(pages + i);
> > > > }
> > >
> > > I am not really sure I follow here. Why do we want to handle
> > > want_init_on_alloc here? The allocated memory comes from the page
> > > allocator and so it will get zeroed there. arch_kexec_post_alloc_pages
> > > might touch the content there but is there any actual risk of any kind
> > > of leak?
> > You're right, we don't want to initialize this memory if init_on_alloc is on.
> > We need something along the lines of:
> > if (!static_branch_unlikely(&init_on_alloc))
> > if (gfp_mask & __GFP_ZERO)
> > // clear the pages
> >
> > Another option would be to disable initialization in alloc_pages() using a flag.
>
> Or we can simply not care and keen the code the way it is. First of all
> it seems that nobody actually does use __GFP_ZERO unless I have missed
> soemthing
> - kimage_alloc_pages(KEXEC_CONTROL_MEMORY_GFP, order); # GFP_KERNEL | __GFP_NORETRY
> - kimage_alloc_pages(gfp_mask, 0);
> - kimage_alloc_page(image, GFP_KERNEL, KIMAGE_NO_DEST);
> - kimage_alloc_page(image, GFP_HIGHUSER, maddr);
>
> but even if we actually had a user do we care about double intialization
> for something kexec related? It is not any hot path AFAIR.
Yes, sounds good. Spraying the code with too many checks for
init_on_alloc doesn't really look nice.
> --
> 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] structleak: disable BYREF_ALL in combination with KASAN_STACK
From: Arnd Bergmann @ 2019-06-21 9:43 UTC (permalink / raw)
To: Kees Cook
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev,
Alexander Popov, Ard Biesheuvel, James Morris, Serge E. Hallyn,
Masahiro Yamada, LSM List, Linux Kernel Mailing List
In-Reply-To: <201906201034.9E44D8A2A8@keescook>
On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > leads to much larger kernel stack usage, as seen from the warnings
> > about functions that now exceed the 2048 byte limit:
>
> Is the preference that this go into v5.2 (there's not much time left),
> or should this be v5.3? (You didn't mark it as Cc: stable?)
Having it in 5.2 would be great. I had not done much build testing in the last
months, so I didn't actually realize that your patch was merged a while ago
rather than only in linux-next.
BTW, I have now run into a small number of files that are still affected
by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
to come up with patches for those as well, we can probably do it in a way
that also improves the affected drivers. I'll put you on Cc when I
find another one.
Arnd
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Qian Cai @ 2019-06-21 12:36 UTC (permalink / raw)
To: Alexander Potapenko, Andrew Morton, Christoph Lameter, Kees Cook
Cc: Masahiro Yamada, Michal Hocko, James Morris, Serge E. Hallyn,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Laura Abbott, Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
linux-mm, linux-security-module, kernel-hardening
In-Reply-To: <20190617151050.92663-2-glider@google.com>
On Mon, 2019-06-17 at 17:10 +0200, Alexander Potapenko wrote:
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d66bc8abe0af..50a3b104a491 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -136,6 +136,48 @@ unsigned long totalcma_pages __read_mostly;
>
> int percpu_pagelist_fraction;
> gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
> +#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> +DEFINE_STATIC_KEY_TRUE(init_on_alloc);
> +#else
> +DEFINE_STATIC_KEY_FALSE(init_on_alloc);
> +#endif
> +#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
> +DEFINE_STATIC_KEY_TRUE(init_on_free);
> +#else
> +DEFINE_STATIC_KEY_FALSE(init_on_free);
> +#endif
> +
There is a problem here running kernels built with clang,
[ 0.000000] static_key_disable(): static key 'init_on_free+0x0/0x4' used
before call to jump_label_init()
[ 0.000000] WARNING: CPU: 0 PID: 0 at ./include/linux/jump_label.h:314
early_init_on_free+0x1c0/0x200
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.2.0-rc5-next-20190620+
#11
[ 0.000000] pstate: 60000089 (nZCv daIf -PAN -UAO)
[ 0.000000] pc : early_init_on_free+0x1c0/0x200
[ 0.000000] lr : early_init_on_free+0x1c0/0x200
[ 0.000000] sp : ffff100012c07df0
[ 0.000000] x29: ffff100012c07e20 x28: ffff1000110a01ec
[ 0.000000] x27: 0000000000000001 x26: ffff100011716f88
[ 0.000000] x25: ffff100010d367ae x24: ffff100010d367a5
[ 0.000000] x23: ffff100010d36afd x22: ffff100011716758
[ 0.000000] x21: 0000000000000000 x20: 0000000000000000
[ 0.000000] x19: 0000000000000000 x18: 000000000000002e
[ 0.000000] x17: 000000000000000f x16: 0000000000000040
[ 0.000000] x15: 0000000000000000 x14: 6d756a206f74206c
[ 0.000000] x13: 6c61632065726f66 x12: 6562206465737520
[ 0.000000] x11: 0000000000000000 x10: 0000000000000000
[ 0.000000] x9 : 0000000000000000 x8 : 0000000000000000
[ 0.000000] x7 : 73203a2928656c62 x6 : ffff1000144367ad
[ 0.000000] x5 : ffff100012c07b28 x4 : 000000000000000f
[ 0.000000] x3 : ffff1000101b36ec x2 : 0000000000000001
[ 0.000000] x1 : 0000000000000001 x0 : 000000000000005d
[ 0.000000] Call trace:
[ 0.000000] early_init_on_free+0x1c0/0x200
[ 0.000000] do_early_param+0xd0/0x104
[ 0.000000] parse_args+0x204/0x54c
[ 0.000000] parse_early_param+0x70/0x8c
[ 0.000000] setup_arch+0xa8/0x268
[ 0.000000] start_kernel+0x80/0x588
[ 0.000000] random: get_random_bytes called from __warn+0x164/0x208 with
crng_init=0
> diff --git a/mm/slub.c b/mm/slub.c
> index cd04dbd2b5d0..9c4a8b9a955c 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1279,6 +1279,12 @@ static int __init setup_slub_debug(char *str)
> if (*str == ',')
> slub_debug_slabs = str + 1;
> out:
> + if ((static_branch_unlikely(&init_on_alloc) ||
> + static_branch_unlikely(&init_on_free)) &&
> + (slub_debug & SLAB_POISON)) {
> + pr_warn("disabling SLAB_POISON: can't be used together with
> memory auto-initialization\n");
> + slub_debug &= ~SLAB_POISON;
> + }
> return 1;
> }
I don't think it is good idea to disable SLAB_POISON here as if people have
decided to enable SLUB_DEBUG later already, they probably care more to make sure
those additional checks with SLAB_POISON are still running to catch memory
corruption.
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-06-21 13:31 UTC (permalink / raw)
To: Qian Cai
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
Michal Hocko, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <1561120576.5154.35.camel@lca.pw>
On Fri, Jun 21, 2019 at 2:36 PM Qian Cai <cai@lca.pw> wrote:
>
> On Mon, 2019-06-17 at 17:10 +0200, Alexander Potapenko wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index d66bc8abe0af..50a3b104a491 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -136,6 +136,48 @@ unsigned long totalcma_pages __read_mostly;
> >
> > int percpu_pagelist_fraction;
> > gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
> > +#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> > +DEFINE_STATIC_KEY_TRUE(init_on_alloc);
> > +#else
> > +DEFINE_STATIC_KEY_FALSE(init_on_alloc);
> > +#endif
> > +#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
> > +DEFINE_STATIC_KEY_TRUE(init_on_free);
> > +#else
> > +DEFINE_STATIC_KEY_FALSE(init_on_free);
> > +#endif
> > +
>
> There is a problem here running kernels built with clang,
>
> [ 0.000000] static_key_disable(): static key 'init_on_free+0x0/0x4' used
> before call to jump_label_init()
> [ 0.000000] WARNING: CPU: 0 PID: 0 at ./include/linux/jump_label.h:314
> early_init_on_free+0x1c0/0x200
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.2.0-rc5-next-20190620+
> #11
> [ 0.000000] pstate: 60000089 (nZCv daIf -PAN -UAO)
> [ 0.000000] pc : early_init_on_free+0x1c0/0x200
> [ 0.000000] lr : early_init_on_free+0x1c0/0x200
> [ 0.000000] sp : ffff100012c07df0
> [ 0.000000] x29: ffff100012c07e20 x28: ffff1000110a01ec
> [ 0.000000] x27: 0000000000000001 x26: ffff100011716f88
> [ 0.000000] x25: ffff100010d367ae x24: ffff100010d367a5
> [ 0.000000] x23: ffff100010d36afd x22: ffff100011716758
> [ 0.000000] x21: 0000000000000000 x20: 0000000000000000
> [ 0.000000] x19: 0000000000000000 x18: 000000000000002e
> [ 0.000000] x17: 000000000000000f x16: 0000000000000040
> [ 0.000000] x15: 0000000000000000 x14: 6d756a206f74206c
> [ 0.000000] x13: 6c61632065726f66 x12: 6562206465737520
> [ 0.000000] x11: 0000000000000000 x10: 0000000000000000
> [ 0.000000] x9 : 0000000000000000 x8 : 0000000000000000
> [ 0.000000] x7 : 73203a2928656c62 x6 : ffff1000144367ad
> [ 0.000000] x5 : ffff100012c07b28 x4 : 000000000000000f
> [ 0.000000] x3 : ffff1000101b36ec x2 : 0000000000000001
> [ 0.000000] x1 : 0000000000000001 x0 : 000000000000005d
> [ 0.000000] Call trace:
> [ 0.000000] early_init_on_free+0x1c0/0x200
> [ 0.000000] do_early_param+0xd0/0x104
> [ 0.000000] parse_args+0x204/0x54c
> [ 0.000000] parse_early_param+0x70/0x8c
> [ 0.000000] setup_arch+0xa8/0x268
> [ 0.000000] start_kernel+0x80/0x588
> [ 0.000000] random: get_random_bytes called from __warn+0x164/0x208 with
> crng_init=0
>
> > diff --git a/mm/slub.c b/mm/slub.c
> > index cd04dbd2b5d0..9c4a8b9a955c 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1279,6 +1279,12 @@ static int __init setup_slub_debug(char *str)
> > if (*str == ',')
> > slub_debug_slabs = str + 1;
> > out:
> > + if ((static_branch_unlikely(&init_on_alloc) ||
> > + static_branch_unlikely(&init_on_free)) &&
> > + (slub_debug & SLAB_POISON)) {
> > + pr_warn("disabling SLAB_POISON: can't be used together with
> > memory auto-initialization\n");
> > + slub_debug &= ~SLAB_POISON;
> > + }
> > return 1;
> > }
>
> I don't think it is good idea to disable SLAB_POISON here as if people have
> decided to enable SLUB_DEBUG later already, they probably care more to make sure
> those additional checks with SLAB_POISON are still running to catch memory
> corruption.
The problem is that freed buffers can't be both poisoned and zeroed at
the same time.
Do you think we need to disable memory initialization in that case instead?
--
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] structleak: disable BYREF_ALL in combination with KASAN_STACK
From: Ard Biesheuvel @ 2019-06-21 13:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
kasan-dev, Alexander Popov, James Morris, Serge E. Hallyn,
Masahiro Yamada, LSM List, Linux Kernel Mailing List
In-Reply-To: <CAK8P3a2uFcaGMSHRdg4NECHJwgAyhtMuYDv3U=z2UdBSL5U0Lw@mail.gmail.com>
On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > leads to much larger kernel stack usage, as seen from the warnings
> > > about functions that now exceed the 2048 byte limit:
> >
> > Is the preference that this go into v5.2 (there's not much time left),
> > or should this be v5.3? (You didn't mark it as Cc: stable?)
>
> Having it in 5.2 would be great. I had not done much build testing in the last
> months, so I didn't actually realize that your patch was merged a while ago
> rather than only in linux-next.
>
> BTW, I have now run into a small number of files that are still affected
> by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> to come up with patches for those as well, we can probably do it in a way
> that also improves the affected drivers. I'll put you on Cc when I
> find another one.
>
There is something fundamentally wrong here, though. BYREF_ALL only
initializes variables that have their address taken, which does not
explain why the size of the stack frame should increase (since in
order to have an address in the first place, the variable must already
have a stack slot assigned)
So I suspect that BYREF_ALL is defeating some optimizations where.
e.g., the call involving the address of the variable is optimized
away, but the the initialization remains, thus forcing the variable to
be allocated in the stack frame even though the initializer is the
only thing that references it.
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Qian Cai @ 2019-06-21 13:36 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
Michal Hocko, James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=XKK5+nC5LErJ+zo7dt3N-cO7zToz=bN2R891dMG_rncA@mail.gmail.com>
On Fri, 2019-06-21 at 15:31 +0200, Alexander Potapenko wrote:
> On Fri, Jun 21, 2019 at 2:36 PM Qian Cai <cai@lca.pw> wrote:
> >
> > On Mon, 2019-06-17 at 17:10 +0200, Alexander Potapenko wrote:
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index d66bc8abe0af..50a3b104a491 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -136,6 +136,48 @@ unsigned long totalcma_pages __read_mostly;
> > >
> > > int percpu_pagelist_fraction;
> > > gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
> > > +#ifdef CONFIG_INIT_ON_ALLOC_DEFAULT_ON
> > > +DEFINE_STATIC_KEY_TRUE(init_on_alloc);
> > > +#else
> > > +DEFINE_STATIC_KEY_FALSE(init_on_alloc);
> > > +#endif
> > > +#ifdef CONFIG_INIT_ON_FREE_DEFAULT_ON
> > > +DEFINE_STATIC_KEY_TRUE(init_on_free);
> > > +#else
> > > +DEFINE_STATIC_KEY_FALSE(init_on_free);
> > > +#endif
> > > +
> >
> > There is a problem here running kernels built with clang,
> >
> > [ 0.000000] static_key_disable(): static key 'init_on_free+0x0/0x4' used
> > before call to jump_label_init()
> > [ 0.000000] WARNING: CPU: 0 PID: 0 at ./include/linux/jump_label.h:314
> > early_init_on_free+0x1c0/0x200
> > [ 0.000000] Modules linked in:
> > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.2.0-rc5-next-
> > 20190620+
> > #11
> > [ 0.000000] pstate: 60000089 (nZCv daIf -PAN -UAO)
> > [ 0.000000] pc : early_init_on_free+0x1c0/0x200
> > [ 0.000000] lr : early_init_on_free+0x1c0/0x200
> > [ 0.000000] sp : ffff100012c07df0
> > [ 0.000000] x29: ffff100012c07e20 x28: ffff1000110a01ec
> > [ 0.000000] x27: 0000000000000001 x26: ffff100011716f88
> > [ 0.000000] x25: ffff100010d367ae x24: ffff100010d367a5
> > [ 0.000000] x23: ffff100010d36afd x22: ffff100011716758
> > [ 0.000000] x21: 0000000000000000 x20: 0000000000000000
> > [ 0.000000] x19: 0000000000000000 x18: 000000000000002e
> > [ 0.000000] x17: 000000000000000f x16: 0000000000000040
> > [ 0.000000] x15: 0000000000000000 x14: 6d756a206f74206c
> > [ 0.000000] x13: 6c61632065726f66 x12: 6562206465737520
> > [ 0.000000] x11: 0000000000000000 x10: 0000000000000000
> > [ 0.000000] x9 : 0000000000000000 x8 : 0000000000000000
> > [ 0.000000] x7 : 73203a2928656c62 x6 : ffff1000144367ad
> > [ 0.000000] x5 : ffff100012c07b28 x4 : 000000000000000f
> > [ 0.000000] x3 : ffff1000101b36ec x2 : 0000000000000001
> > [ 0.000000] x1 : 0000000000000001 x0 : 000000000000005d
> > [ 0.000000] Call trace:
> > [ 0.000000] early_init_on_free+0x1c0/0x200
> > [ 0.000000] do_early_param+0xd0/0x104
> > [ 0.000000] parse_args+0x204/0x54c
> > [ 0.000000] parse_early_param+0x70/0x8c
> > [ 0.000000] setup_arch+0xa8/0x268
> > [ 0.000000] start_kernel+0x80/0x588
> > [ 0.000000] random: get_random_bytes called from __warn+0x164/0x208 with
> > crng_init=0
> >
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index cd04dbd2b5d0..9c4a8b9a955c 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -1279,6 +1279,12 @@ static int __init setup_slub_debug(char *str)
> > > if (*str == ',')
> > > slub_debug_slabs = str + 1;
> > > out:
> > > + if ((static_branch_unlikely(&init_on_alloc) ||
> > > + static_branch_unlikely(&init_on_free)) &&
> > > + (slub_debug & SLAB_POISON)) {
> > > + pr_warn("disabling SLAB_POISON: can't be used together with
> > > memory auto-initialization\n");
> > > + slub_debug &= ~SLAB_POISON;
> > > + }
> > > return 1;
> > > }
> >
> > I don't think it is good idea to disable SLAB_POISON here as if people have
> > decided to enable SLUB_DEBUG later already, they probably care more to make
> > sure
> > those additional checks with SLAB_POISON are still running to catch memory
> > corruption.
>
> The problem is that freed buffers can't be both poisoned and zeroed at
> the same time.
> Do you think we need to disable memory initialization in that case instead?
Yes, disable init_on_free|alloc and keep SLAB_POISON sounds good to me.
^ permalink raw reply
* Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK
From: Arnd Bergmann @ 2019-06-21 13:44 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Kees Cook, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
kasan-dev, Alexander Popov, James Morris, Serge E. Hallyn,
Masahiro Yamada, LSM List, Linux Kernel Mailing List
In-Reply-To: <CAKv+Gu-A_OWUQ_neUAprmQOotPA=LoUGQHvFkZ2tqQAg=us1jA@mail.gmail.com>
On Fri, Jun 21, 2019 at 3:32 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > > leads to much larger kernel stack usage, as seen from the warnings
> > > > about functions that now exceed the 2048 byte limit:
> > >
> > > Is the preference that this go into v5.2 (there's not much time left),
> > > or should this be v5.3? (You didn't mark it as Cc: stable?)
> >
> > Having it in 5.2 would be great. I had not done much build testing in the last
> > months, so I didn't actually realize that your patch was merged a while ago
> > rather than only in linux-next.
> >
> > BTW, I have now run into a small number of files that are still affected
> > by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> > to come up with patches for those as well, we can probably do it in a way
> > that also improves the affected drivers. I'll put you on Cc when I
> > find another one.
> >
>
> There is something fundamentally wrong here, though. BYREF_ALL only
> initializes variables that have their address taken, which does not
> explain why the size of the stack frame should increase (since in
> order to have an address in the first place, the variable must already
> have a stack slot assigned)
>
> So I suspect that BYREF_ALL is defeating some optimizations where.
> e.g., the call involving the address of the variable is optimized
> away, but the the initialization remains, thus forcing the variable to
> be allocated in the stack frame even though the initializer is the
> only thing that references it.
One pattern I have seen here is temporary variables from macros or
inline functions whose lifetime now extends over the entire function
rather than just the basic block in which they are defined, see e.g.
lpfc_debug_dump_qe() being inlined multiple times into
lpfc_debug_dump_all_queues(). Each instance of the local
"char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
one now, where the behavior without the structleak plugin is that
they don't.
Arnd
^ permalink raw reply
* Re: [PATCH] structleak: disable BYREF_ALL in combination with KASAN_STACK
From: Ard Biesheuvel @ 2019-06-21 13:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kees Cook, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
kasan-dev, Alexander Popov, James Morris, Serge E. Hallyn,
Masahiro Yamada, LSM List, Linux Kernel Mailing List
In-Reply-To: <CAK8P3a2d3H-pdiLX_8aA4LNLOVTSyPW_jvwZQkv0Ey3SJS87Bg@mail.gmail.com>
On Fri, 21 Jun 2019 at 15:44, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Jun 21, 2019 at 3:32 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On Fri, 21 Jun 2019 at 11:44, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thu, Jun 20, 2019 at 7:36 PM Kees Cook <keescook@chromium.org> wrote:
> > > > On Tue, Jun 18, 2019 at 11:47:13AM +0200, Arnd Bergmann wrote:
> > > > > The combination of KASAN_STACK and GCC_PLUGIN_STRUCTLEAK_BYREF_ALL
> > > > > leads to much larger kernel stack usage, as seen from the warnings
> > > > > about functions that now exceed the 2048 byte limit:
> > > >
> > > > Is the preference that this go into v5.2 (there's not much time left),
> > > > or should this be v5.3? (You didn't mark it as Cc: stable?)
> > >
> > > Having it in 5.2 would be great. I had not done much build testing in the last
> > > months, so I didn't actually realize that your patch was merged a while ago
> > > rather than only in linux-next.
> > >
> > > BTW, I have now run into a small number of files that are still affected
> > > by a stack overflow warning from STRUCTLEAK_BYREF_ALL. I'm trying
> > > to come up with patches for those as well, we can probably do it in a way
> > > that also improves the affected drivers. I'll put you on Cc when I
> > > find another one.
> > >
> >
> > There is something fundamentally wrong here, though. BYREF_ALL only
> > initializes variables that have their address taken, which does not
> > explain why the size of the stack frame should increase (since in
> > order to have an address in the first place, the variable must already
> > have a stack slot assigned)
> >
> > So I suspect that BYREF_ALL is defeating some optimizations where.
> > e.g., the call involving the address of the variable is optimized
> > away, but the the initialization remains, thus forcing the variable to
> > be allocated in the stack frame even though the initializer is the
> > only thing that references it.
>
> One pattern I have seen here is temporary variables from macros or
> inline functions whose lifetime now extends over the entire function
> rather than just the basic block in which they are defined, see e.g.
> lpfc_debug_dump_qe() being inlined multiple times into
> lpfc_debug_dump_all_queues(). Each instance of the local
> "char line_buf[LPFC_LBUF_SZ];" seems to add on to the previous
> one now, where the behavior without the structleak plugin is that
> they don't.
>
Right, that seems to be due to the fact that this code
/* split the first bb where we can put the forced initializers */
gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
if (!single_pred_p(bb)) {
split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
}
puts all the initializers at the beginning of the function rather than
inside the scope of the definition.
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-06-21 14:10 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=UFj0Lzy3FgMV_JBKtxCiwE03HVxnR8=f9a7=4nrUFXSw@mail.gmail.com>
On Fri, Jun 21, 2019 at 10:57 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Fri, Jun 21, 2019 at 9:09 AM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Mon 17-06-19 17:10:49, 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.
> > >
> > > Both init_on_alloc=1 and init_on_free=1 guarantee that the allocator
> > > returns zeroed memory. The two exceptions are slab caches with
> > > constructors and SLAB_TYPESAFE_BY_RCU flag. Those are never
> > > zero-initialized to preserve their semantics.
> > >
> > > Both init_on_alloc and init_on_free default to zero, but those defaults
> > > can be overridden with CONFIG_INIT_ON_ALLOC_DEFAULT_ON and
> > > CONFIG_INIT_ON_FREE_DEFAULT_ON.
> > >
> > > Slowdown for the new features compared to init_on_free=0,
> > > init_on_alloc=0:
> > >
> > > hackbench, init_on_free=1: +7.62% sys time (st.err 0.74%)
> > > hackbench, init_on_alloc=1: +7.75% sys time (st.err 2.14%)
> > >
> > > Linux build with -j12, init_on_free=1: +8.38% wall time (st.err 0.39%)
> > > Linux build with -j12, init_on_free=1: +24.42% sys time (st.err 0.52%)
> > > Linux build with -j12, init_on_alloc=1: -0.13% wall time (st.err 0.42%)
> > > Linux build with -j12, init_on_alloc=1: +0.57% sys time (st.err 0.40%)
> > >
> > > The slowdown for init_on_free=0, init_on_alloc=0 compared to the
> > > baseline is within the standard error.
> > >
> > > The new features are also going to pave the way for hardware memory
> > > tagging (e.g. arm64's MTE), which will require both on_alloc and on_free
> > > hooks to set the tags for heap objects. With MTE, tagging will have the
> > > same cost as memory initialization.
> > >
> > > Although init_on_free is rather costly, there are paranoid use-cases where
> > > in-memory data lifetime is desired to be minimized. There are various
> > > arguments for/against the realism of the associated threat models, but
> > > given that we'll need the infrastructre for MTE anyway, and there are
> > > people who want wipe-on-free behavior no matter what the performance cost,
> > > it seems reasonable to include it in this series.
> >
> > Thanks for reworking the original implemenation. This looks much better!
> >
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > Acked-by: Kees Cook <keescook@chromium.org>
> > > To: Andrew Morton <akpm@linux-foundation.org>
> > > To: Christoph Lameter <cl@linux.com>
> > > To: Kees Cook <keescook@chromium.org>
> > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > Cc: Michal Hocko <mhocko@kernel.org>
> > > Cc: James Morris <jmorris@namei.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Cc: Kostya Serebryany <kcc@google.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Sandeep Patil <sspatil@android.com>
> > > Cc: Laura Abbott <labbott@redhat.com>
> > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > Cc: Jann Horn <jannh@google.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Marco Elver <elver@google.com>
> > > Cc: linux-mm@kvack.org
> > > Cc: linux-security-module@vger.kernel.org
> > > Cc: kernel-hardening@lists.openwall.com
> >
> > Acked-by: Michal Hocko <mhocko@suse.cz> # page allocator parts.
> >
> > kmalloc based parts look good to me as well but I am not sure I fill
> > qualified to give my ack there without much more digging and I do not
> > have much time for that now.
> >
> > [...]
> > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > > index fd5c95ff9251..2f75dd0d0d81 100644
> > > --- a/kernel/kexec_core.c
> > > +++ b/kernel/kexec_core.c
> > > @@ -315,7 +315,7 @@ static struct page *kimage_alloc_pages(gfp_t gfp_mask, unsigned int order)
> > > arch_kexec_post_alloc_pages(page_address(pages), count,
> > > gfp_mask);
> > >
> > > - if (gfp_mask & __GFP_ZERO)
> > > + if (want_init_on_alloc(gfp_mask))
> > > for (i = 0; i < count; i++)
> > > clear_highpage(pages + i);
> > > }
> >
> > I am not really sure I follow here. Why do we want to handle
> > want_init_on_alloc here? The allocated memory comes from the page
> > allocator and so it will get zeroed there. arch_kexec_post_alloc_pages
> > might touch the content there but is there any actual risk of any kind
> > of leak?
> You're right, we don't want to initialize this memory if init_on_alloc is on.
> We need something along the lines of:
> if (!static_branch_unlikely(&init_on_alloc))
> if (gfp_mask & __GFP_ZERO)
> // clear the pages
>
> Another option would be to disable initialization in alloc_pages() using a flag.
> >
> > > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > > index 8c94c89a6f7e..e164012d3491 100644
> > > --- a/mm/dmapool.c
> > > +++ b/mm/dmapool.c
> > > @@ -378,7 +378,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
> > > #endif
> > > spin_unlock_irqrestore(&pool->lock, flags);
> > >
> > > - if (mem_flags & __GFP_ZERO)
> > > + if (want_init_on_alloc(mem_flags))
> > > memset(retval, 0, pool->size);
> > >
> > > return retval;
> >
> > Don't you miss dma_pool_free and want_init_on_free?
> Agreed.
> I'll fix this and add tests for DMA pools as well.
This doesn't seem to be easy though. One needs a real DMA-capable
device to allocate using DMA pools.
On the other hand, what happens to a DMA pool when it's destroyed,
isn't it wiped by pagealloc?
I'm inclined towards not touching mm/dmapool.c in this patch series,
as it is probably orthogonal to the idea of hardening the
heap/pagealloc.
> > --
> > 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
--
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 v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-06-21 15:12 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=W90HNeZ0UcUctnbUBzJ=_b+gxMGdUoDyO3JPoyy4dGSg@mail.gmail.com>
On Fri 21-06-19 16:10:19, Alexander Potapenko wrote:
> On Fri, Jun 21, 2019 at 10:57 AM Alexander Potapenko <glider@google.com> wrote:
[...]
> > > > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > > > index 8c94c89a6f7e..e164012d3491 100644
> > > > --- a/mm/dmapool.c
> > > > +++ b/mm/dmapool.c
> > > > @@ -378,7 +378,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
> > > > #endif
> > > > spin_unlock_irqrestore(&pool->lock, flags);
> > > >
> > > > - if (mem_flags & __GFP_ZERO)
> > > > + if (want_init_on_alloc(mem_flags))
> > > > memset(retval, 0, pool->size);
> > > >
> > > > return retval;
> > >
> > > Don't you miss dma_pool_free and want_init_on_free?
> > Agreed.
> > I'll fix this and add tests for DMA pools as well.
> This doesn't seem to be easy though. One needs a real DMA-capable
> device to allocate using DMA pools.
> On the other hand, what happens to a DMA pool when it's destroyed,
> isn't it wiped by pagealloc?
Yes it should be returned to the page allocator AFAIR. But it is when we
are returning an object to the pool when you want to wipe the data, no?
Why cannot you do it along the already existing poisoning?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Alexander Potapenko @ 2019-06-21 15:24 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <20190621151210.GF3429@dhcp22.suse.cz>
On Fri, Jun 21, 2019 at 5:12 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Fri 21-06-19 16:10:19, Alexander Potapenko wrote:
> > On Fri, Jun 21, 2019 at 10:57 AM Alexander Potapenko <glider@google.com> wrote:
> [...]
> > > > > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > > > > index 8c94c89a6f7e..e164012d3491 100644
> > > > > --- a/mm/dmapool.c
> > > > > +++ b/mm/dmapool.c
> > > > > @@ -378,7 +378,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
> > > > > #endif
> > > > > spin_unlock_irqrestore(&pool->lock, flags);
> > > > >
> > > > > - if (mem_flags & __GFP_ZERO)
> > > > > + if (want_init_on_alloc(mem_flags))
> > > > > memset(retval, 0, pool->size);
> > > > >
> > > > > return retval;
> > > >
> > > > Don't you miss dma_pool_free and want_init_on_free?
> > > Agreed.
> > > I'll fix this and add tests for DMA pools as well.
> > This doesn't seem to be easy though. One needs a real DMA-capable
> > device to allocate using DMA pools.
> > On the other hand, what happens to a DMA pool when it's destroyed,
> > isn't it wiped by pagealloc?
>
> Yes it should be returned to the page allocator AFAIR. But it is when we
> are returning an object to the pool when you want to wipe the data, no?
My concern was that dma allocation is something orthogonal to heap and
page allocator.
I also don't know how many other allocators are left overboard, e.g.
we don't do anything to lib/genalloc.c yet.
> Why cannot you do it along the already existing poisoning?
I can sure keep these bits.
Any idea how the correct behavior of dma_pool_alloc/free can be tested?
> --
> 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 v7 1/2] mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options
From: Michal Hocko @ 2019-06-21 15:54 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Andrew Morton, Christoph Lameter, Kees Cook, Masahiro Yamada,
James Morris, Serge E. Hallyn, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Laura Abbott,
Randy Dunlap, Jann Horn, Mark Rutland, Marco Elver,
Linux Memory Management List, linux-security-module,
Kernel Hardening
In-Reply-To: <CAG_fn=W2fm5zkAUW8PcTYpfH57H89ukFGAoBHUOmyM-S1agdZg@mail.gmail.com>
On Fri 21-06-19 17:24:21, Alexander Potapenko wrote:
> On Fri, Jun 21, 2019 at 5:12 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Fri 21-06-19 16:10:19, Alexander Potapenko wrote:
> > > On Fri, Jun 21, 2019 at 10:57 AM Alexander Potapenko <glider@google.com> wrote:
> > [...]
> > > > > > diff --git a/mm/dmapool.c b/mm/dmapool.c
> > > > > > index 8c94c89a6f7e..e164012d3491 100644
> > > > > > --- a/mm/dmapool.c
> > > > > > +++ b/mm/dmapool.c
> > > > > > @@ -378,7 +378,7 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
> > > > > > #endif
> > > > > > spin_unlock_irqrestore(&pool->lock, flags);
> > > > > >
> > > > > > - if (mem_flags & __GFP_ZERO)
> > > > > > + if (want_init_on_alloc(mem_flags))
> > > > > > memset(retval, 0, pool->size);
> > > > > >
> > > > > > return retval;
> > > > >
> > > > > Don't you miss dma_pool_free and want_init_on_free?
> > > > Agreed.
> > > > I'll fix this and add tests for DMA pools as well.
> > > This doesn't seem to be easy though. One needs a real DMA-capable
> > > device to allocate using DMA pools.
> > > On the other hand, what happens to a DMA pool when it's destroyed,
> > > isn't it wiped by pagealloc?
> >
> > Yes it should be returned to the page allocator AFAIR. But it is when we
> > are returning an object to the pool when you want to wipe the data, no?
> My concern was that dma allocation is something orthogonal to heap and
> page allocator.
> I also don't know how many other allocators are left overboard, e.g.
> we don't do anything to lib/genalloc.c yet.
Well, that really depends what would you like to achieve by this
functionality. There are likely to be all sorts of allocators on top of
the core ones (e.g. mempool allocator). The question is whether you
really want to cover them all. Are they security relevant?
> > Why cannot you do it along the already existing poisoning?
> I can sure keep these bits.
> Any idea how the correct behavior of dma_pool_alloc/free can be tested?
Well, I would say that you have to rely on the review process here more
than any specific testing. In any case other allocators can be handled
incrementally. This is not all or nothing kinda stuff. I have pointed
out dma_pool because it only addresses one half of the work and it was
not clear why. If you want to drop dma_pool then this will be fine by
me. As this is a hardening feature you want to get coverage as large as
possible rather than 100%.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* RE: [RFC PATCH v4 04/12] x86/sgx: Require userspace to define enclave pages' protection bits
From: Xing, Cedric @ 2019-06-21 16:42 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190619222401.14942-5-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Wednesday, June 19, 2019 3:24 PM
>
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h index
> 6dba9f282232..67a3babbb24d 100644
> --- a/arch/x86/include/uapi/asm/sgx.h
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -35,15 +35,17 @@ struct sgx_enclave_create {
> * @src: address for the page data
> * @secinfo: address for the SECINFO data
> * @mrmask: bitmask for the measured 256 byte chunks
> + * @prot: maximal PROT_{READ,WRITE,EXEC} protections for the page
> */
> struct sgx_enclave_add_page {
> __u64 addr;
> __u64 src;
> __u64 secinfo;
> - __u64 mrmask;
> + __u16 mrmask;
> + __u8 prot;
> + __u8 pad;
> };
Given EPCM permissions cannot change in SGX1, these maximal PROT_* flags can be the same as EPCM permissions, so don't have to be specified by user code until SGX2. Given we don't have a clear picture on how SGX2 will work yet, I think we shall take "prot" off until it is proven necessary.
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 29384cdd0842..dabfe2a7245a 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -93,15 +93,64 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd, }
> #endif
>
> +/*
> + * Returns the AND of VM_{READ,WRITE,EXEC} permissions across all pages
> + * covered by the specific VMA. A non-existent (or yet to be added)
> +enclave
> + * page is considered to have no RWX permissions, i.e. is inaccessible.
> + */
> +static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
> + struct vm_area_struct *vma)
> +{
> + unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
> + unsigned long idx, idx_start, idx_end;
> + struct sgx_encl_page *page;
> +
> + idx_start = PFN_DOWN(vma->vm_start);
> + idx_end = PFN_DOWN(vma->vm_end - 1);
> +
> + for (idx = idx_start; idx <= idx_end; ++idx) {
> + /*
> + * No need to take encl->lock, vm_prot_bits is set prior to
> + * insertion and never changes, and racing with adding pages is
> + * a userspace bug.
> + */
> + rcu_read_lock();
> + page = radix_tree_lookup(&encl->page_tree, idx);
> + rcu_read_unlock();
This loop iterates through every page in the range, which could be very slow if the range is large.
> +
> + /* Do not allow R|W|X to a non-existent page. */
> + if (!page)
> + allowed_rwx = 0;
> + else
> + allowed_rwx &= page->vm_prot_bits;
> + if (!allowed_rwx)
> + break;
> + }
> +
> + return allowed_rwx;
> +}
> +
> static int sgx_mmap(struct file *file, struct vm_area_struct *vma) {
> struct sgx_encl *encl = file->private_data;
> + unsigned long allowed_rwx;
> int ret;
>
> + allowed_rwx = sgx_allowed_rwx(encl, vma);
> + if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
> + return -EACCES;
> +
> ret = sgx_encl_mm_add(encl, vma->vm_mm);
> if (ret)
> return ret;
>
> + if (!(allowed_rwx & VM_READ))
> + vma->vm_flags &= ~VM_MAYREAD;
> + if (!(allowed_rwx & VM_WRITE))
> + vma->vm_flags &= ~VM_MAYWRITE;
> + if (!(allowed_rwx & VM_EXEC))
> + vma->vm_flags &= ~VM_MAYEXEC;
> +
Say a range comprised of a RW sub-range and a RX sub-range is being mmap()'ed as R here. It'd succeed but mprotect(<RW sub-range>, RW) afterwards will fail because VM_MAYWRITE is cleared here. However, if those two sub-ranges are mapped by separate mmap() calls then the same mprotect() would succeed. The inconsistence here is unexpected and unprecedented.
> vma->vm_ops = &sgx_vm_ops;
> vma->vm_flags |= VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO;
> vma->vm_private_data = encl;
^ permalink raw reply
* RE: [RFC PATCH v4 07/12] LSM: x86/sgx: Introduce ->enclave_map() hook for Intel SGX
From: Xing, Cedric @ 2019-06-21 16:54 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190619222401.14942-8-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Wednesday, June 19, 2019 3:24 PM
>
> diff --git a/security/security.c b/security/security.c
> index 613a5c00e602..03951e08bdfc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2359,3 +2359,10 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
> call_void_hook(bpf_prog_free_security, aux);
> }
> #endif /* CONFIG_BPF_SYSCALL */
> +
> +#ifdef CONFIG_INTEL_SGX
> +int security_enclave_map(unsigned long prot)
> +{
> + return call_int_hook(enclave_map, 0, prot);
> +}
> +#endif /* CONFIG_INTEL_SGX */
Why is this new security_enclave_map() necessary while security_mmap_file() will also be invoked?
^ permalink raw reply
* RE: [RFC PATCH v4 09/12] LSM: x86/sgx: Introduce ->enclave_load() hook for Intel SGX
From: Xing, Cedric @ 2019-06-21 17:05 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190619222401.14942-10-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Wednesday, June 19, 2019 3:24 PM
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 6a1f54ba6794..572ddfc53039 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1832,11 +1832,18 @@ static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
> #ifdef CONFIG_INTEL_SGX
> #ifdef CONFIG_SECURITY
> int security_enclave_map(unsigned long prot);
> +int security_enclave_load(struct vm_area_struct *vma, unsigned long prot,
> + bool measured);
> #else
> static inline int security_enclave_map(unsigned long prot)
> {
> return 0;
> }
> +static inline int security_enclave_load(struct vm_area_struct *vma,
> + unsigned long prot, bool measured)
> +{
> + return 0;
> +}
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_INTEL_SGX */
Parameters to security_enclave_load() are specific on what's being loading only, but unspecific on which enclave to be loaded into. That kills the possibility of an LSM module making enclave dependent decisions.
Btw, if enclave (in the form of struct file) is also passed in as a parameter, it'd let LSM know that file is an enclave, hence would be able to make the same decision in security_mmap_file() as in security_enclave_map(). In other words, you wouldn't need security_enclave_map().
^ permalink raw reply
* RE: [RFC PATCH v4 08/12] security/selinux: Require SGX_MAPWX to map enclave page WX
From: Xing, Cedric @ 2019-06-21 17:09 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190619222401.14942-9-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Wednesday, June 19, 2019 3:24 PM
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702cf46ca..fc239e541b62 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6726,6 +6726,23 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> }
> #endif
>
> +#ifdef CONFIG_INTEL_SGX
> +static int selinux_enclave_map(unsigned long prot)
> +{
> + const struct cred *cred = current_cred();
> + u32 sid = cred_sid(cred);
> +
> + /* SGX is supported only in 64-bit kernels. */
> + WARN_ON_ONCE(!default_noexec);
> +
> + if ((prot & PROT_EXEC) && (prot & PROT_WRITE))
> + return avc_has_perm(&selinux_state, sid, sid,
> + SECCLASS_PROCESS2, PROCESS2__SGX_MAPWX,
> + NULL);
Why isn't SGX_MAPWX enclave specific but process wide?
^ permalink raw reply
* RE: [RFC PATCH v4 12/12] LSM: x86/sgx: Show line of sight to LSM support SGX2's EAUG
From: Xing, Cedric @ 2019-06-21 17:18 UTC (permalink / raw)
To: Christopherson, Sean J, Jarkko Sakkinen
Cc: linux-sgx@vger.kernel.org, linux-security-module@vger.kernel.org,
selinux@vger.kernel.org, Roberts, William C, Schaufler, Casey,
James Morris, Hansen, Dave, Andy Lutomirski, Jethro Beekman,
Dr . Greg Wettstein, Stephen Smalley
In-Reply-To: <20190619222401.14942-13-sean.j.christopherson@intel.com>
> From: Christopherson, Sean J
> Sent: Wednesday, June 19, 2019 3:24 PM
>
> diff --git a/arch/x86/kernel/cpu/sgx/driver/main.c b/arch/x86/kernel/cpu/sgx/driver/main.c
> index 4379a2fb1f82..b478c0f45279 100644
> --- a/arch/x86/kernel/cpu/sgx/driver/main.c
> +++ b/arch/x86/kernel/cpu/sgx/driver/main.c
> @@ -99,7 +99,8 @@ static long sgx_compat_ioctl(struct file *filep, unsigned int cmd,
> * page is considered to have no RWX permissions, i.e. is inaccessible.
> */
> static unsigned long sgx_allowed_rwx(struct sgx_encl *encl,
> - struct vm_area_struct *vma)
> + struct vm_area_struct *vma,
> + bool *eaug)
> {
> unsigned long allowed_rwx = VM_READ | VM_WRITE | VM_EXEC;
> unsigned long idx, idx_start, idx_end; @@ -123,6 +124,8 @@ static unsigned long
> sgx_allowed_rwx(struct sgx_encl *encl,
> allowed_rwx = 0;
> else
> allowed_rwx &= page->vm_prot_bits;
> + if (page->vm_prot_bits & SGX_VM_EAUG)
> + *eaug = true;
> if (!allowed_rwx)
> break;
> }
> @@ -134,16 +137,17 @@ static int sgx_mmap(struct file *file, struct vm_area_struct *vma)
> {
> struct sgx_encl *encl = file->private_data;
> unsigned long allowed_rwx, prot;
> + bool eaug = false;
> int ret;
>
> - allowed_rwx = sgx_allowed_rwx(encl, vma);
> + allowed_rwx = sgx_allowed_rwx(encl, vma, &eaug);
> if (vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC) & ~allowed_rwx)
> return -EACCES;
IIUC, "eaug range" has to be mapped PROT_NONE, then vm_ops->fault() won't be invoked. Am I correct? Then how to EAUG on #PF?
>
> prot = _calc_vm_trans(vma->vm_flags, VM_READ, PROT_READ) |
> _calc_vm_trans(vma->vm_flags, VM_WRITE, PROT_WRITE) |
> _calc_vm_trans(vma->vm_flags, VM_EXEC, PROT_EXEC);
> - ret = security_enclave_map(prot);
> + ret = security_enclave_map(prot, eaug);
> if (ret)
> return ret;
>
^ permalink raw reply
* [PATCH v3 00/24] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
This patchset provides the changes required for
the AppArmor security module to stack safely with any other.
v3: Incorporate feedback from v2
- Make lsmblob parameter and variable names more
meaningful, changing "le" and "l" to "blob".
- Improve consistency of constant naming.
- Do more sanity checking during LSM initialization.
- Be a bit clearer about what is temporary scaffolding.
- Rather than clutter security_getpeersec_dgram with
otherwise unnecessary checks remove the apparmor
stub, which does nothing useful.
Patches 0001-0003 complete the process of moving managment
of security blobs that might be shared from the individual
modules to the infrastructure.
Patches 0004-0014 replace system use of a "secid" with
a structure "lsmblob" containing information from the
security modules to be held and reused later. At this
point lsmblob contains an array of u32 secids, one "slot"
for each of the security modules compiled into the
kernel that used secids. A "slot" is allocated when
a security module registers a hook for one of the interfaces
that uses a secid or a security context. The infrastructure
is changed to use the slot number to pass the correct
secid to or from the security module hooks.
It is important that the lsmblob be a fixed size entity
that does not have to be allocated. Several of the places
where it is used would have performance and/or locking
issues with dynamic allocation.
Patch 0015 provides a mechanism for a process to
identify which security module's hooks should be used
when displaying or converting a security context string.
A new interface /proc/.../attr/display contains the name
of the security module to show. Reading from this file
will present the name of the module, while writing to
it will set the value. Only names of active security
modules are accepted. Internally, the name is translated
to the appropriate "slot" number for the module which
is then stored in the task security blob.
Patch 0016 Starts the process of changing how a security
context is represented. Since it is possible for a
security context to have been generated by more than one
security module it is now necessary to note which module
created a security context so that the correct "release"
hook can be called. There are several places where the
module that created a security context cannot be inferred.
This is achieved by introducing a "lsmcontext" structure
which contains the context string, its length and the
"slot" number of the security module that created it.
The security_release_secctx() interface is changed,
replacing the (string,len) pointer pair with a lsmcontext
pointer.
Patches 0012-0021 convert the security interfaces from
(string,len) pointer pairs to a lsmcontext pointer.
The slot number identifying the creating module is
added by the infrastructure. Where the security context
is stored for extended periods the data type is changed.
Patch 0022 provides a simple way for a security module
to know its "slot" number. The security_add_hooks()
initialization function returns the slot number, and the
security module need but stash the value for later use,
as is required by the Netlabel subsystem. The Netlabel
code is converted to save lsmblob structures instead
of secids in Patch 0023.
Finally, with all interference on the AppArmor hooks
removed, Patch 0024 removes the exclusive bit from
AppArmor. An unnecessary stub hook was also removed.
The Ubuntu project is using an earlier version of
this patchset in their distribution to enable stacking
for containers.
Performance measurements to date have the change
within the "noise". Better benchmarks are in the
works.
https://github.com/cschaufler/lsm-stacking.git#stack-5.2-v3-apparmor
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
drivers/android/binder.c | 24 +-
fs/kernfs/dir.c | 5 +-
fs/kernfs/inode.c | 35 ++-
fs/kernfs/kernfs-internal.h | 3 +-
fs/nfs/nfs4proc.c | 22 +-
fs/nfsd/nfs4xdr.c | 20 +-
fs/proc/base.c | 1 +
include/linux/cred.h | 3 +-
include/linux/lsm_hooks.h | 8 +-
include/linux/security.h | 165 ++++++++++---
include/net/af_unix.h | 2 +-
include/net/netlabel.h | 8 +-
include/net/scm.h | 14 +-
kernel/audit.c | 34 +--
kernel/audit.h | 9 +-
kernel/auditfilter.c | 6 +-
kernel/auditsc.c | 79 +++----
kernel/cred.c | 12 +-
net/ipv4/cipso_ipv4.c | 6 +-
net/ipv4/ip_sockglue.c | 12 +-
net/netfilter/nf_conntrack_netlink.c | 20 +-
net/netfilter/nf_conntrack_standalone.c | 11 +-
net/netfilter/nfnetlink_queue.c | 26 ++-
net/netfilter/nft_meta.c | 13 +-
net/netfilter/xt_SECMARK.c | 5 +-
net/netlabel/netlabel_kapi.c | 6 +-
net/netlabel/netlabel_unlabeled.c | 95 ++++----
net/netlabel/netlabel_unlabeled.h | 2 +-
net/netlabel/netlabel_user.c | 13 +-
net/netlabel/netlabel_user.h | 6 +-
net/unix/af_unix.c | 6 +-
security/apparmor/include/net.h | 6 +-
security/apparmor/lsm.c | 66 ++----
security/integrity/ima/ima.h | 14 +-
security/integrity/ima/ima_api.c | 9 +-
security/integrity/ima/ima_appraise.c | 6 +-
security/integrity/ima/ima_main.c | 36 +--
security/integrity/ima/ima_policy.c | 19 +-
security/security.c | 396 ++++++++++++++++++++++++++++----
security/selinux/hooks.c | 164 ++++++-------
security/selinux/include/objsec.h | 18 ++
security/selinux/include/security.h | 1 +
security/selinux/netlabel.c | 25 +-
security/selinux/ss/services.c | 7 +-
security/smack/smack.h | 19 ++
security/smack/smack_lsm.c | 154 ++++++-------
security/smack/smack_netfilter.c | 8 +-
security/smack/smackfs.c | 10 +-
48 files changed, 1017 insertions(+), 612 deletions(-)
^ permalink raw reply
* [PATCH v3 01/24] LSM: Infrastructure management of the superblock
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
Move management of the superblock->sb_security blob out
of the individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 1 +
security/security.c | 46 ++++++++++++++++++++----
security/selinux/hooks.c | 58 ++++++++++++-------------------
security/selinux/include/objsec.h | 6 ++++
security/selinux/ss/services.c | 3 +-
security/smack/smack.h | 6 ++++
security/smack/smack_lsm.c | 35 +++++--------------
7 files changed, 85 insertions(+), 70 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a240a3fc5fc4..f9222a04968d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2047,6 +2047,7 @@ struct lsm_blob_sizes {
int lbs_cred;
int lbs_file;
int lbs_inode;
+ int lbs_superblock;
int lbs_ipc;
int lbs_msg_msg;
int lbs_task;
diff --git a/security/security.c b/security/security.c
index 23cbb1a295a3..550988a0f024 100644
--- a/security/security.c
+++ b/security/security.c
@@ -172,6 +172,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
+ lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
}
@@ -300,12 +301,13 @@ static void __init ordered_lsm_init(void)
for (lsm = ordered_lsms; *lsm; lsm++)
prepare_lsm(*lsm);
- init_debug("cred blob size = %d\n", blob_sizes.lbs_cred);
- init_debug("file blob size = %d\n", blob_sizes.lbs_file);
- init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
- init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
- init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
- init_debug("task blob size = %d\n", blob_sizes.lbs_task);
+ init_debug("cred blob size = %d\n", blob_sizes.lbs_cred);
+ init_debug("file blob size = %d\n", blob_sizes.lbs_file);
+ init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
+ init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
+ init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
+ init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
+ init_debug("task blob size = %d\n", blob_sizes.lbs_task);
/*
* Create any kmem_caches needed for blobs
@@ -603,6 +605,27 @@ static void __init lsm_early_task(struct task_struct *task)
panic("%s: Early task alloc failed.\n", __func__);
}
+/**
+ * lsm_superblock_alloc - allocate a composite superblock blob
+ * @sb: the superblock that needs a blob
+ *
+ * Allocate the superblock blob for all the modules
+ *
+ * Returns 0, or -ENOMEM if memory can't be allocated.
+ */
+int lsm_superblock_alloc(struct super_block *sb)
+{
+ if (blob_sizes.lbs_superblock == 0) {
+ sb->s_security = NULL;
+ return 0;
+ }
+
+ sb->s_security = kzalloc(blob_sizes.lbs_superblock, GFP_KERNEL);
+ if (sb->s_security == NULL)
+ return -ENOMEM;
+ return 0;
+}
+
/*
* Hook list operation macros.
*
@@ -776,12 +799,21 @@ int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *
int security_sb_alloc(struct super_block *sb)
{
- return call_int_hook(sb_alloc_security, 0, sb);
+ int rc = lsm_superblock_alloc(sb);
+
+ if (unlikely(rc))
+ return rc;
+ rc = call_int_hook(sb_alloc_security, 0, sb);
+ if (unlikely(rc))
+ security_sb_free(sb);
+ return rc;
}
void security_sb_free(struct super_block *sb)
{
call_void_hook(sb_free_security, sb);
+ kfree(sb->s_security);
+ sb->s_security = NULL;
}
void security_free_mnt_opts(void **mnt_opts)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1d0b37af2444..7478d8eda00a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -335,7 +335,7 @@ static void inode_free_security(struct inode *inode)
if (!isec)
return;
- sbsec = inode->i_sb->s_security;
+ sbsec = selinux_superblock(inode->i_sb);
/*
* As not all inode security structures are in a list, we check for
* empty list outside of the lock to make sure that we won't waste
@@ -366,11 +366,7 @@ static int file_alloc_security(struct file *file)
static int superblock_alloc_security(struct super_block *sb)
{
- struct superblock_security_struct *sbsec;
-
- sbsec = kzalloc(sizeof(struct superblock_security_struct), GFP_KERNEL);
- if (!sbsec)
- return -ENOMEM;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
mutex_init(&sbsec->lock);
INIT_LIST_HEAD(&sbsec->isec_head);
@@ -379,18 +375,10 @@ static int superblock_alloc_security(struct super_block *sb)
sbsec->sid = SECINITSID_UNLABELED;
sbsec->def_sid = SECINITSID_FILE;
sbsec->mntpoint_sid = SECINITSID_UNLABELED;
- sb->s_security = sbsec;
return 0;
}
-static void superblock_free_security(struct super_block *sb)
-{
- struct superblock_security_struct *sbsec = sb->s_security;
- sb->s_security = NULL;
- kfree(sbsec);
-}
-
struct selinux_mnt_opts {
const char *fscontext, *context, *rootcontext, *defcontext;
};
@@ -507,7 +495,7 @@ static int selinux_is_genfs_special_handling(struct super_block *sb)
static int selinux_is_sblabel_mnt(struct super_block *sb)
{
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
/*
* IMPORTANT: Double-check logic in this function when adding a new
@@ -535,7 +523,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
static int sb_finish_set_opts(struct super_block *sb)
{
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
struct dentry *root = sb->s_root;
struct inode *root_inode = d_backing_inode(root);
int rc = 0;
@@ -648,7 +636,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
unsigned long *set_kern_flags)
{
const struct cred *cred = current_cred();
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
struct dentry *root = sbsec->sb->s_root;
struct selinux_mnt_opts *opts = mnt_opts;
struct inode_security_struct *root_isec;
@@ -881,8 +869,8 @@ static int selinux_set_mnt_opts(struct super_block *sb,
static int selinux_cmp_sb_context(const struct super_block *oldsb,
const struct super_block *newsb)
{
- struct superblock_security_struct *old = oldsb->s_security;
- struct superblock_security_struct *new = newsb->s_security;
+ struct superblock_security_struct *old = selinux_superblock(oldsb);
+ struct superblock_security_struct *new = selinux_superblock(newsb);
char oldflags = old->flags & SE_MNTMASK;
char newflags = new->flags & SE_MNTMASK;
@@ -914,8 +902,9 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
unsigned long *set_kern_flags)
{
int rc = 0;
- const struct superblock_security_struct *oldsbsec = oldsb->s_security;
- struct superblock_security_struct *newsbsec = newsb->s_security;
+ const struct superblock_security_struct *oldsbsec =
+ selinux_superblock(oldsb);
+ struct superblock_security_struct *newsbsec = selinux_superblock(newsb);
int set_fscontext = (oldsbsec->flags & FSCONTEXT_MNT);
int set_context = (oldsbsec->flags & CONTEXT_MNT);
@@ -1085,7 +1074,7 @@ static int show_sid(struct seq_file *m, u32 sid)
static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
{
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
int rc;
if (!(sbsec->flags & SE_SBINITIALIZED))
@@ -1377,7 +1366,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
if (isec->sclass == SECCLASS_FILE)
isec->sclass = inode_mode_to_security_class(inode->i_mode);
- sbsec = inode->i_sb->s_security;
+ sbsec = selinux_superblock(inode->i_sb);
if (!(sbsec->flags & SE_SBINITIALIZED)) {
/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
@@ -1767,7 +1756,8 @@ selinux_determine_inode_label(const struct task_security_struct *tsec,
const struct qstr *name, u16 tclass,
u32 *_new_isid)
{
- const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
+ const struct superblock_security_struct *sbsec =
+ selinux_superblock(dir->i_sb);
if ((sbsec->flags & SE_SBINITIALIZED) &&
(sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
@@ -1798,7 +1788,7 @@ static int may_create(struct inode *dir,
int rc;
dsec = inode_security(dir);
- sbsec = dir->i_sb->s_security;
+ sbsec = selinux_superblock(dir->i_sb);
sid = tsec->sid;
@@ -1947,7 +1937,7 @@ static int superblock_has_perm(const struct cred *cred,
struct superblock_security_struct *sbsec;
u32 sid = cred_sid(cred);
- sbsec = sb->s_security;
+ sbsec = selinux_superblock(sb);
return avc_has_perm(&selinux_state,
sid, sbsec->sid, SECCLASS_FILESYSTEM, perms, ad);
}
@@ -2578,11 +2568,6 @@ static int selinux_sb_alloc_security(struct super_block *sb)
return superblock_alloc_security(sb);
}
-static void selinux_sb_free_security(struct super_block *sb)
-{
- superblock_free_security(sb);
-}
-
static inline int opt_len(const char *s)
{
bool open_quote = false;
@@ -2653,7 +2638,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
{
struct selinux_mnt_opts *opts = mnt_opts;
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
u32 sid;
int rc;
@@ -2877,7 +2862,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
int rc;
char *context;
- sbsec = dir->i_sb->s_security;
+ sbsec = selinux_superblock(dir->i_sb);
newsid = tsec->create_sid;
@@ -3115,7 +3100,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
}
- sbsec = inode->i_sb->s_security;
+ sbsec = selinux_superblock(inode->i_sb);
if (!(sbsec->flags & SBLABEL_MNT))
return -EOPNOTSUPP;
@@ -3296,13 +3281,14 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
const void *value, size_t size, int flags)
{
struct inode_security_struct *isec = inode_security_novalidate(inode);
- struct superblock_security_struct *sbsec = inode->i_sb->s_security;
+ struct superblock_security_struct *sbsec;
u32 newsid;
int rc;
if (strcmp(name, XATTR_SELINUX_SUFFIX))
return -EOPNOTSUPP;
+ sbsec = selinux_superblock(inode->i_sb);
if (!(sbsec->flags & SBLABEL_MNT))
return -EOPNOTSUPP;
@@ -6647,6 +6633,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_inode = sizeof(struct inode_security_struct),
.lbs_ipc = sizeof(struct ipc_security_struct),
.lbs_msg_msg = sizeof(struct msg_security_struct),
+ .lbs_superblock = sizeof(struct superblock_security_struct),
};
static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
@@ -6675,7 +6662,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
- LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 231262d8eac9..d08d7e5d2f93 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -188,4 +188,10 @@ static inline struct ipc_security_struct *selinux_ipc(
return ipc->security + selinux_blob_sizes.lbs_ipc;
}
+static inline struct superblock_security_struct *selinux_superblock(
+ const struct super_block *superblock)
+{
+ return superblock->s_security + selinux_blob_sizes.lbs_superblock;
+}
+
#endif /* _SELINUX_OBJSEC_H_ */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ec62918521b1..e3f5d6aece66 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -50,6 +50,7 @@
#include <linux/audit.h>
#include <linux/mutex.h>
#include <linux/vmalloc.h>
+#include <linux/lsm_hooks.h>
#include <net/netlabel.h>
#include "flask.h"
@@ -2751,7 +2752,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
struct sidtab *sidtab;
int rc = 0;
struct ocontext *c;
- struct superblock_security_struct *sbsec = sb->s_security;
+ struct superblock_security_struct *sbsec = selinux_superblock(sb);
const char *fstype = sb->s_type->name;
read_lock(&state->ss->policy_rwlock);
diff --git a/security/smack/smack.h b/security/smack/smack.h
index cf52af77d15e..caecbcba9942 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -375,6 +375,12 @@ static inline struct smack_known **smack_ipc(const struct kern_ipc_perm *ipc)
return ipc->security + smack_blob_sizes.lbs_ipc;
}
+static inline struct superblock_smack *smack_superblock(
+ const struct super_block *superblock)
+{
+ return superblock->s_security + smack_blob_sizes.lbs_superblock;
+}
+
/*
* Is the directory transmuting?
*/
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 5c1613519d5a..807eff2ccce9 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -540,12 +540,7 @@ static int smack_syslog(int typefrom_file)
*/
static int smack_sb_alloc_security(struct super_block *sb)
{
- struct superblock_smack *sbsp;
-
- sbsp = kzalloc(sizeof(struct superblock_smack), GFP_KERNEL);
-
- if (sbsp == NULL)
- return -ENOMEM;
+ struct superblock_smack *sbsp = smack_superblock(sb);
sbsp->smk_root = &smack_known_floor;
sbsp->smk_default = &smack_known_floor;
@@ -554,22 +549,10 @@ static int smack_sb_alloc_security(struct super_block *sb)
/*
* SMK_SB_INITIALIZED will be zero from kzalloc.
*/
- sb->s_security = sbsp;
return 0;
}
-/**
- * smack_sb_free_security - free a superblock blob
- * @sb: the superblock getting the blob
- *
- */
-static void smack_sb_free_security(struct super_block *sb)
-{
- kfree(sb->s_security);
- sb->s_security = NULL;
-}
-
struct smack_mnt_opts {
const char *fsdefault, *fsfloor, *fshat, *fsroot, *fstransmute;
};
@@ -781,7 +764,7 @@ static int smack_set_mnt_opts(struct super_block *sb,
{
struct dentry *root = sb->s_root;
struct inode *inode = d_backing_inode(root);
- struct superblock_smack *sp = sb->s_security;
+ struct superblock_smack *sp = smack_superblock(sb);
struct inode_smack *isp;
struct smack_known *skp;
struct smack_mnt_opts *opts = mnt_opts;
@@ -880,7 +863,7 @@ static int smack_set_mnt_opts(struct super_block *sb,
*/
static int smack_sb_statfs(struct dentry *dentry)
{
- struct superblock_smack *sbp = dentry->d_sb->s_security;
+ struct superblock_smack *sbp = smack_superblock(dentry->d_sb);
int rc;
struct smk_audit_info ad;
@@ -917,7 +900,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task)
return 0;
- sbsp = inode->i_sb->s_security;
+ sbsp = smack_superblock(inode->i_sb);
if ((sbsp->smk_flags & SMK_SB_UNTRUSTED) &&
isp->smk_task != sbsp->smk_root)
return 0;
@@ -1168,7 +1151,7 @@ static int smack_inode_rename(struct inode *old_inode,
*/
static int smack_inode_permission(struct inode *inode, int mask)
{
- struct superblock_smack *sbsp = inode->i_sb->s_security;
+ struct superblock_smack *sbsp = smack_superblock(inode->i_sb);
struct smk_audit_info ad;
int no_block = mask & MAY_NOT_BLOCK;
int rc;
@@ -1410,7 +1393,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
*/
if (strcmp(name, XATTR_NAME_SMACK) == 0) {
struct super_block *sbp = dentry->d_sb;
- struct superblock_smack *sbsp = sbp->s_security;
+ struct superblock_smack *sbsp = smack_superblock(sbp);
isp->smk_inode = sbsp->smk_default;
} else if (strcmp(name, XATTR_NAME_SMACKEXEC) == 0)
@@ -1680,7 +1663,7 @@ static int smack_mmap_file(struct file *file,
isp = smack_inode(file_inode(file));
if (isp->smk_mmap == NULL)
return 0;
- sbsp = file_inode(file)->i_sb->s_security;
+ sbsp = smack_superblock(file_inode(file)->i_sb);
if (sbsp->smk_flags & SMK_SB_UNTRUSTED &&
isp->smk_mmap != sbsp->smk_root)
return -EACCES;
@@ -3288,7 +3271,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
goto unlockandout;
sbp = inode->i_sb;
- sbsp = sbp->s_security;
+ sbsp = smack_superblock(sbp);
/*
* We're going to use the superblock default label
* if there's no label on the file.
@@ -4575,6 +4558,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_inode = sizeof(struct inode_smack),
.lbs_ipc = sizeof(struct smack_known *),
.lbs_msg_msg = sizeof(struct smack_known *),
+ .lbs_superblock = sizeof(struct superblock_smack),
};
static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
@@ -4586,7 +4570,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(fs_context_parse_param, smack_fs_context_parse_param),
LSM_HOOK_INIT(sb_alloc_security, smack_sb_alloc_security),
- LSM_HOOK_INIT(sb_free_security, smack_sb_free_security),
LSM_HOOK_INIT(sb_free_mnt_opts, smack_free_mnt_opts),
LSM_HOOK_INIT(sb_eat_lsm_opts, smack_sb_eat_lsm_opts),
LSM_HOOK_INIT(sb_statfs, smack_sb_statfs),
--
2.20.1
^ permalink raw reply related
* [PATCH v3 03/24] LSM: Infrastructure management of the key blob
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
From: Casey Schaufler <cschaufler@schaufler-ca.com>
Move management of the key->security blob out of the
individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 1 +
security/security.c | 40 ++++++++++++++++++++++++++++++-
security/selinux/hooks.c | 23 +++++-------------
security/selinux/include/objsec.h | 7 ++++++
security/smack/smack.h | 7 ++++++
security/smack/smack_lsm.c | 33 ++++++++++++-------------
6 files changed, 75 insertions(+), 36 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index b353482ea348..3fe39abccc8f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2050,6 +2050,7 @@ struct lsm_blob_sizes {
int lbs_sock;
int lbs_superblock;
int lbs_ipc;
+ int lbs_key;
int lbs_msg_msg;
int lbs_task;
};
diff --git a/security/security.c b/security/security.c
index e32b7180282e..d05f00a40e82 100644
--- a/security/security.c
+++ b/security/security.c
@@ -172,6 +172,9 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
blob_sizes.lbs_inode = sizeof(struct rcu_head);
lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
+#ifdef CONFIG_KEYS
+ lsm_set_blob_size(&needed->lbs_key, &blob_sizes.lbs_key);
+#endif
lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
lsm_set_blob_size(&needed->lbs_sock, &blob_sizes.lbs_sock);
lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
@@ -307,6 +310,9 @@ static void __init ordered_lsm_init(void)
init_debug("file blob size = %d\n", blob_sizes.lbs_file);
init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
+#ifdef CONFIG_KEYS
+ init_debug("key blob size = %d\n", blob_sizes.lbs_key);
+#endif /* CONFIG_KEYS */
init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
@@ -573,6 +579,29 @@ static int lsm_ipc_alloc(struct kern_ipc_perm *kip)
return 0;
}
+#ifdef CONFIG_KEYS
+/**
+ * lsm_key_alloc - allocate a composite key blob
+ * @key: the key that needs a blob
+ *
+ * Allocate the key blob for all the modules
+ *
+ * Returns 0, or -ENOMEM if memory can't be allocated.
+ */
+int lsm_key_alloc(struct key *key)
+{
+ if (blob_sizes.lbs_key == 0) {
+ key->security = NULL;
+ return 0;
+ }
+
+ key->security = kzalloc(blob_sizes.lbs_key, GFP_KERNEL);
+ if (key->security == NULL)
+ return -ENOMEM;
+ return 0;
+}
+#endif /* CONFIG_KEYS */
+
/**
* lsm_msg_msg_alloc - allocate a composite msg_msg blob
* @mp: the msg_msg that needs a blob
@@ -2339,12 +2368,21 @@ EXPORT_SYMBOL(security_skb_classify_flow);
int security_key_alloc(struct key *key, const struct cred *cred,
unsigned long flags)
{
- return call_int_hook(key_alloc, 0, key, cred, flags);
+ int rc = lsm_key_alloc(key);
+
+ if (unlikely(rc))
+ return rc;
+ rc = call_int_hook(key_alloc, 0, key, cred, flags);
+ if (unlikely(rc))
+ security_key_free(key);
+ return rc;
}
void security_key_free(struct key *key)
{
call_void_hook(key_free, key);
+ kfree(key->security);
+ key->security = NULL;
}
int security_key_permission(key_ref_t key_ref,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f38a6f484613..ee840fecfebb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6353,11 +6353,7 @@ static int selinux_key_alloc(struct key *k, const struct cred *cred,
unsigned long flags)
{
const struct task_security_struct *tsec;
- struct key_security_struct *ksec;
-
- ksec = kzalloc(sizeof(struct key_security_struct), GFP_KERNEL);
- if (!ksec)
- return -ENOMEM;
+ struct key_security_struct *ksec = selinux_key(k);
tsec = selinux_cred(cred);
if (tsec->keycreate_sid)
@@ -6365,18 +6361,9 @@ static int selinux_key_alloc(struct key *k, const struct cred *cred,
else
ksec->sid = tsec->sid;
- k->security = ksec;
return 0;
}
-static void selinux_key_free(struct key *k)
-{
- struct key_security_struct *ksec = k->security;
-
- k->security = NULL;
- kfree(ksec);
-}
-
static int selinux_key_permission(key_ref_t key_ref,
const struct cred *cred,
unsigned perm)
@@ -6394,7 +6381,7 @@ static int selinux_key_permission(key_ref_t key_ref,
sid = cred_sid(cred);
key = key_ref_to_ptr(key_ref);
- ksec = key->security;
+ ksec = selinux_key(key);
return avc_has_perm(&selinux_state,
sid, ksec->sid, SECCLASS_KEY, perm, NULL);
@@ -6402,7 +6389,7 @@ static int selinux_key_permission(key_ref_t key_ref,
static int selinux_key_getsecurity(struct key *key, char **_buffer)
{
- struct key_security_struct *ksec = key->security;
+ struct key_security_struct *ksec = selinux_key(key);
char *context = NULL;
unsigned len;
int rc;
@@ -6627,6 +6614,9 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
.lbs_file = sizeof(struct file_security_struct),
.lbs_inode = sizeof(struct inode_security_struct),
.lbs_ipc = sizeof(struct ipc_security_struct),
+#ifdef CONFIG_KEYS
+ .lbs_key = sizeof(struct key_security_struct),
+#endif /* CONFIG_KEYS */
.lbs_msg_msg = sizeof(struct msg_security_struct),
.lbs_sock = sizeof(struct sk_security_struct),
.lbs_superblock = sizeof(struct superblock_security_struct),
@@ -6842,7 +6832,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
#ifdef CONFIG_KEYS
LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
- LSM_HOOK_INIT(key_free, selinux_key_free),
LSM_HOOK_INIT(key_permission, selinux_key_permission),
LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
#endif
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 29f02b8f8f31..3b78aa4ee98f 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -194,6 +194,13 @@ static inline struct superblock_security_struct *selinux_superblock(
return superblock->s_security + selinux_blob_sizes.lbs_superblock;
}
+#ifdef CONFIG_KEYS
+static inline struct key_security_struct *selinux_key(const struct key *key)
+{
+ return key->security + selinux_blob_sizes.lbs_key;
+}
+#endif /* CONFIG_KEYS */
+
static inline struct sk_security_struct *selinux_sock(const struct sock *sock)
{
return sock->sk_security + selinux_blob_sizes.lbs_sock;
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 4ac4bf3310d7..7cc3a3382fee 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -386,6 +386,13 @@ static inline struct superblock_smack *smack_superblock(
return superblock->s_security + smack_blob_sizes.lbs_superblock;
}
+#ifdef CONFIG_KEYS
+static inline struct smack_known **smack_key(const struct key *key)
+{
+ return key->security + smack_blob_sizes.lbs_key;
+}
+#endif /* CONFIG_KEYS */
+
/*
* Is the directory transmuting?
*/
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index fd69e1bd841b..e9560b078efe 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4179,23 +4179,13 @@ static void smack_inet_csk_clone(struct sock *sk,
static int smack_key_alloc(struct key *key, const struct cred *cred,
unsigned long flags)
{
+ struct smack_known **blob = smack_key(key);
struct smack_known *skp = smk_of_task(smack_cred(cred));
- key->security = skp;
+ *blob = skp;
return 0;
}
-/**
- * smack_key_free - Clear the key security blob
- * @key: the object
- *
- * Clear the blob pointer
- */
-static void smack_key_free(struct key *key)
-{
- key->security = NULL;
-}
-
/**
* smack_key_permission - Smack access on a key
* @key_ref: gets to the object
@@ -4208,6 +4198,8 @@ static void smack_key_free(struct key *key)
static int smack_key_permission(key_ref_t key_ref,
const struct cred *cred, unsigned perm)
{
+ struct smack_known **blob;
+ struct smack_known *skp;
struct key *keyp;
struct smk_audit_info ad;
struct smack_known *tkp = smk_of_task(smack_cred(cred));
@@ -4227,7 +4219,9 @@ static int smack_key_permission(key_ref_t key_ref,
* If the key hasn't been initialized give it access so that
* it may do so.
*/
- if (keyp->security == NULL)
+ blob = smack_key(keyp);
+ skp = *blob;
+ if (skp == NULL)
return 0;
/*
* This should not occur
@@ -4247,8 +4241,8 @@ static int smack_key_permission(key_ref_t key_ref,
request |= MAY_READ;
if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
request |= MAY_WRITE;
- rc = smk_access(tkp, keyp->security, request, &ad);
- rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
+ rc = smk_access(tkp, skp, request, &ad);
+ rc = smk_bu_note("key access", tkp, skp, request, rc);
return rc;
}
@@ -4263,11 +4257,12 @@ static int smack_key_permission(key_ref_t key_ref,
*/
static int smack_key_getsecurity(struct key *key, char **_buffer)
{
- struct smack_known *skp = key->security;
+ struct smack_known **blob = smack_key(key);
+ struct smack_known *skp = *blob;
size_t length;
char *copy;
- if (key->security == NULL) {
+ if (skp == NULL) {
*_buffer = NULL;
return 0;
}
@@ -4550,6 +4545,9 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_file = sizeof(struct smack_known *),
.lbs_inode = sizeof(struct inode_smack),
.lbs_ipc = sizeof(struct smack_known *),
+#ifdef CONFIG_KEYS
+ .lbs_key = sizeof(struct smack_known *),
+#endif /* CONFIG_KEYS */
.lbs_msg_msg = sizeof(struct smack_known *),
.lbs_sock = sizeof(struct socket_smack),
.lbs_superblock = sizeof(struct superblock_smack),
@@ -4671,7 +4669,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
/* key management security hooks */
#ifdef CONFIG_KEYS
LSM_HOOK_INIT(key_alloc, smack_key_alloc),
- LSM_HOOK_INIT(key_free, smack_key_free),
LSM_HOOK_INIT(key_permission, smack_key_permission),
LSM_HOOK_INIT(key_getsecurity, smack_key_getsecurity),
#endif /* CONFIG_KEYS */
--
2.20.1
^ permalink raw reply related
* [PATCH v3 04/24] LSM: Create and manage the lsmblob data structure.
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
When more than one security module is exporting data to
audit and networking sub-systems a single 32 bit integer
is no longer sufficient to represent the data. Add a
structure to be used instead.
The lsmblob structure is currently an array of
u32 "secids". There is an entry for each of the
security modules built into the system that would
use secids if active. The system assigns the module
a "slot" when it registers hooks. If modules are
compiled in but not registered there will be unused
slots.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 1 +
include/linux/security.h | 62 +++++++++++++++++++++++++++++++++++++++
security/security.c | 36 +++++++++++++++++++++++
3 files changed, 99 insertions(+)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3fe39abccc8f..4d1ddf1a2aa6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2038,6 +2038,7 @@ struct security_hook_list {
struct hlist_head *head;
union security_list_options hook;
char *lsm;
+ int slot;
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index 49f2685324b0..0aa9417a5762 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -76,6 +76,68 @@ enum lsm_event {
LSM_POLICY_CHANGE,
};
+/*
+ * Data exported by the security modules
+ */
+#define LSMBLOB_ENTRIES ( \
+ (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
+ (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0))
+
+struct lsmblob {
+ u32 secid[LSMBLOB_ENTRIES];
+};
+
+#define LSMBLOB_INVALID -1
+
+/**
+ * lsmblob_init - initialize an lsmblob structure.
+ * @blob: Pointer to the data to initialize
+ * @secid: The initial secid value
+ *
+ * Set all secid for all modules to the specified value.
+ */
+static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
+{
+ int i;
+
+ for (i = 0; i < LSMBLOB_ENTRIES; i++)
+ blob->secid[i] = secid;
+}
+
+/**
+ * lsmblob_is_set - report if there is an value in the lsmblob
+ * @blob: Pointer to the exported LSM data
+ *
+ * Returns true if there is a secid set, false otherwise
+ */
+static inline bool lsmblob_is_set(struct lsmblob *blob)
+{
+ int i;
+
+ for (i = 0; i < LSMBLOB_ENTRIES; i++)
+ if (blob->secid[i] != 0)
+ return true;
+ return false;
+}
+
+/**
+ * lsmblob_equal - report if the two lsmblob's are equal
+ * @bloba: Pointer to one LSM data
+ * @blobb: Pointer to the other LSM data
+ *
+ * Returns true if all entries in the two are equal, false otherwise
+ */
+static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
+{
+ int i;
+
+ for (i = 0; i < LSMBLOB_ENTRIES; i++)
+ if (bloba->secid[i] != blobb->secid[i])
+ return false;
+ return true;
+}
+
/* These functions are in security/commoncap.c */
extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
int cap, unsigned int opts);
diff --git a/security/security.c b/security/security.c
index d05f00a40e82..7618c761060d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void)
init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
init_debug("task blob size = %d\n", blob_sizes.lbs_task);
+ init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob));
/*
* Create any kmem_caches needed for blobs
@@ -420,6 +421,11 @@ static int lsm_append(char *new, char **result)
return 0;
}
+/*
+ * Current index to use while initializing the lsmblob secid list.
+ */
+static int lsm_slot __initdata;
+
/**
* security_add_hooks - Add a modules hooks to the hook lists.
* @hooks: the hooks to add
@@ -427,15 +433,45 @@ static int lsm_append(char *new, char **result)
* @lsm: the name of the security module
*
* Each LSM has to register its hooks with the infrastructure.
+ * If the LSM is using hooks that export secids allocate a slot
+ * for it in the lsmblob.
*/
void __init security_add_hooks(struct security_hook_list *hooks, int count,
char *lsm)
{
+ int slot = LSMBLOB_INVALID;
int i;
for (i = 0; i < count; i++) {
hooks[i].lsm = lsm;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+ /*
+ * If this is one of the hooks that uses a secid
+ * note it so that a slot can in allocated for the
+ * secid in the lsmblob structure.
+ */
+ if (hooks[i].head == &security_hook_heads.audit_rule_match ||
+ hooks[i].head == &security_hook_heads.kernel_act_as ||
+ hooks[i].head ==
+ &security_hook_heads.socket_getpeersec_dgram ||
+ hooks[i].head == &security_hook_heads.getprocattr ||
+ hooks[i].head == &security_hook_heads.setprocattr ||
+ hooks[i].head == &security_hook_heads.secctx_to_secid ||
+ hooks[i].head == &security_hook_heads.secid_to_secctx ||
+ hooks[i].head == &security_hook_heads.ipc_getsecid ||
+ hooks[i].head == &security_hook_heads.task_getsecid ||
+ hooks[i].head == &security_hook_heads.inode_getsecid ||
+ hooks[i].head == &security_hook_heads.cred_getsecid) {
+ if (slot == LSMBLOB_INVALID) {
+ slot = lsm_slot++;
+ if (slot >= LSMBLOB_ENTRIES)
+ panic("%s Too many LSMs registered.\n",
+ __func__);
+ init_debug("%s assigned lsmblob slot %d\n",
+ hooks[i].lsm, slot);
+ }
+ }
+ hooks[i].slot = slot;
}
if (lsm_append(lsm, &lsm_names) < 0)
panic("%s - Cannot get early memory.\n", __func__);
--
2.20.1
^ permalink raw reply related
* [PATCH v3 07/24] net: Prepare UDS for secuirty module stacking
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
Change the data used in UDS SO_PEERSEC processing from a
secid to a more general struct lsmblob. Update the
security_socket_getpeersec_dgram() interface to use the
lsmblob. There is a small amount of scaffolding code
that will come out when the security_secid_to_secctx()
code is brought in line with the lsmblob.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/security.h | 7 +++++--
include/net/af_unix.h | 2 +-
include/net/scm.h | 8 +++++---
net/ipv4/ip_sockglue.c | 8 +++++---
net/unix/af_unix.c | 6 +++---
security/security.c | 16 +++++++++++++---
6 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 4a78516cc74a..905830a90745 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1276,7 +1276,8 @@ int security_socket_shutdown(struct socket *sock, int how);
int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
int __user *optlen, unsigned len);
-int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
+int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
+ struct lsmblob *blob);
int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
void security_sk_free(struct sock *sk);
void security_sk_clone(const struct sock *sk, struct sock *newsk);
@@ -1414,7 +1415,9 @@ static inline int security_socket_getpeersec_stream(struct socket *sock, char __
return -ENOPROTOOPT;
}
-static inline int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
+static inline int security_socket_getpeersec_dgram(struct socket *sock,
+ struct sk_buff *skb,
+ struct lsmblob *blob)
{
return -ENOPROTOOPT;
}
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 3426d6dacc45..933492c08b8c 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -36,7 +36,7 @@ struct unix_skb_parms {
kgid_t gid;
struct scm_fp_list *fp; /* Passed files */
#ifdef CONFIG_SECURITY_NETWORK
- u32 secid; /* Security ID */
+ struct lsmblob lsmblob; /* Security LSM data */
#endif
u32 consumed;
} __randomize_layout;
diff --git a/include/net/scm.h b/include/net/scm.h
index 1ce365f4c256..e2e71c4bf9d0 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -33,7 +33,7 @@ struct scm_cookie {
struct scm_fp_list *fp; /* Passed files */
struct scm_creds creds; /* Skb credentials */
#ifdef CONFIG_SECURITY_NETWORK
- u32 secid; /* Passed security ID */
+ struct lsmblob lsmblob; /* Passed LSM data */
#endif
};
@@ -46,7 +46,7 @@ struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
#ifdef CONFIG_SECURITY_NETWORK
static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
{
- security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
+ security_socket_getpeersec_dgram(sock, NULL, &scm->lsmblob);
}
#else
static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct scm_cookie *scm)
@@ -97,7 +97,9 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
int err;
if (test_bit(SOCK_PASSSEC, &sock->flags)) {
- err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
+ /* Scaffolding - it has to be element 0 for now */
+ err = security_secid_to_secctx(scm->lsmblob.secid[0],
+ &secdata, &seclen);
if (!err) {
put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 82f341e84fae..2a5c868ce135 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -130,15 +130,17 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
{
+ struct lsmblob lb;
char *secdata;
- u32 seclen, secid;
+ u32 seclen;
int err;
- err = security_socket_getpeersec_dgram(NULL, skb, &secid);
+ err = security_socket_getpeersec_dgram(NULL, skb, &lb);
if (err)
return;
- err = security_secid_to_secctx(secid, &secdata, &seclen);
+ /* Scaffolding - it has to be element 0 */
+ err = security_secid_to_secctx(lb.secid[0], &secdata, &seclen);
if (err)
return;
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ddb838a1b74c..c50a004a1389 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -143,17 +143,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
#ifdef CONFIG_SECURITY_NETWORK
static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
{
- UNIXCB(skb).secid = scm->secid;
+ UNIXCB(skb).lsmblob = scm->lsmblob;
}
static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
{
- scm->secid = UNIXCB(skb).secid;
+ scm->lsmblob = UNIXCB(skb).lsmblob;
}
static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
{
- return (scm->secid == UNIXCB(skb).secid);
+ return lsmblob_equal(&scm->lsmblob, &(UNIXCB(skb).lsmblob));
}
#else
static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
diff --git a/security/security.c b/security/security.c
index 43f8018b9e13..c7b3d1a294ad 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2137,10 +2137,20 @@ int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
optval, optlen, len);
}
-int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
+int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb,
+ struct lsmblob *blob)
{
- return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
- skb, secid);
+ struct security_hook_list *hp;
+ int rc = -ENOPROTOOPT;
+
+ hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_dgram,
+ list) {
+ rc = hp->hook.socket_getpeersec_dgram(sock, skb,
+ &blob->secid[hp->slot]);
+ if (rc != 0)
+ break;
+ }
+ return rc;
}
EXPORT_SYMBOL(security_socket_getpeersec_dgram);
--
2.20.1
^ permalink raw reply related
* [PATCH v3 05/24] Use lsmblob in security_audit_rule_match
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
Change the secid parameter of security_audit_rule_match
to a lsmblob structure pointer. Pass the entry from the
lsmblob structure for the approprite slot to the LSM hook.
Change the users of security_audit_rule_match to use the
lsmblob instead of a u32. In some cases this requires a
temporary conversion using lsmblob_init() that will go
away when other interfaces get converted.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/security.h | 7 ++++---
kernel/auditfilter.c | 4 +++-
kernel/auditsc.c | 14 ++++++++++----
security/integrity/ima/ima.h | 4 ++--
security/integrity/ima/ima_policy.c | 7 +++++--
security/security.c | 14 ++++++++++++--
6 files changed, 36 insertions(+), 14 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 0aa9417a5762..52d89c4a9594 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1757,7 +1757,8 @@ static inline int security_key_getsecurity(struct key *key, char **_buffer)
#ifdef CONFIG_SECURITY
int security_audit_rule_init(u32 field, u32 op, char *rulestr, void **lsmrule);
int security_audit_rule_known(struct audit_krule *krule);
-int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule);
+int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
+ void *lsmrule);
void security_audit_rule_free(void *lsmrule);
#else
@@ -1773,8 +1774,8 @@ static inline int security_audit_rule_known(struct audit_krule *krule)
return 0;
}
-static inline int security_audit_rule_match(u32 secid, u32 field, u32 op,
- void *lsmrule)
+static inline int security_audit_rule_match(struct lsmblob *blob, u32 field,
+ u32 op, void *lsmrule)
{
return 0;
}
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 63f8b3f26fab..da211065160f 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1324,6 +1324,7 @@ int audit_filter(int msgtype, unsigned int listtype)
struct audit_field *f = &e->rule.fields[i];
pid_t pid;
u32 sid;
+ struct lsmblob blob;
switch (f->type) {
case AUDIT_PID:
@@ -1354,7 +1355,8 @@ int audit_filter(int msgtype, unsigned int listtype)
case AUDIT_SUBJ_CLR:
if (f->lsm_rule) {
security_task_getsecid(current, &sid);
- result = security_audit_rule_match(sid,
+ lsmblob_init(&blob, sid);
+ result = security_audit_rule_match(&blob,
f->type, f->op, f->lsm_rule);
}
break;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d1eab1d4a930..18ee5556c086 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -445,6 +445,7 @@ static int audit_filter_rules(struct task_struct *tsk,
const struct cred *cred;
int i, need_sid = 1;
u32 sid;
+ struct lsmblob blob;
unsigned int sessionid;
cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
@@ -630,7 +631,9 @@ static int audit_filter_rules(struct task_struct *tsk,
security_task_getsecid(tsk, &sid);
need_sid = 0;
}
- result = security_audit_rule_match(sid, f->type,
+ lsmblob_init(&blob, sid);
+ result = security_audit_rule_match(&blob,
+ f->type,
f->op,
f->lsm_rule);
}
@@ -645,15 +648,17 @@ static int audit_filter_rules(struct task_struct *tsk,
if (f->lsm_rule) {
/* Find files that match */
if (name) {
+ lsmblob_init(&blob, name->osid);
result = security_audit_rule_match(
- name->osid,
+ &blob,
f->type,
f->op,
f->lsm_rule);
} else if (ctx) {
list_for_each_entry(n, &ctx->names_list, list) {
+ lsmblob_init(&blob, n->osid);
if (security_audit_rule_match(
- n->osid,
+ &blob,
f->type,
f->op,
f->lsm_rule)) {
@@ -665,7 +670,8 @@ static int audit_filter_rules(struct task_struct *tsk,
/* Find ipc objects that match */
if (!ctx || ctx->type != AUDIT_IPC)
break;
- if (security_audit_rule_match(ctx->ipc.osid,
+ lsmblob_init(&blob, ctx->ipc.osid);
+ if (security_audit_rule_match(&blob,
f->type, f->op,
f->lsm_rule))
++result;
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..5a337239d9e4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -307,8 +307,8 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
return -EINVAL;
}
-static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
- void *lsmrule)
+static inline int security_filter_rule_match(struct lsmblob *blob, u32 field,
+ u32 op, void *lsmrule)
{
return -EINVAL;
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..e7b8ce942950 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -327,6 +327,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
for (i = 0; i < MAX_LSM_RULES; i++) {
int rc = 0;
u32 osid;
+ struct lsmblob blob;
int retried = 0;
if (!rule->lsm[i].rule)
@@ -337,7 +338,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
case LSM_OBJ_ROLE:
case LSM_OBJ_TYPE:
security_inode_getsecid(inode, &osid);
- rc = security_filter_rule_match(osid,
+ lsmblob_init(&blob, osid);
+ rc = security_filter_rule_match(&blob,
rule->lsm[i].type,
Audit_equal,
rule->lsm[i].rule);
@@ -345,7 +347,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
case LSM_SUBJ_USER:
case LSM_SUBJ_ROLE:
case LSM_SUBJ_TYPE:
- rc = security_filter_rule_match(secid,
+ lsmblob_init(&blob, secid);
+ rc = security_filter_rule_match(&blob,
rule->lsm[i].type,
Audit_equal,
rule->lsm[i].rule);
diff --git a/security/security.c b/security/security.c
index 7618c761060d..4692f44718c6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2452,9 +2452,19 @@ void security_audit_rule_free(void *lsmrule)
call_void_hook(audit_rule_free, lsmrule);
}
-int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
+int security_audit_rule_match(struct lsmblob *blob, u32 field, u32 op,
+ void *lsmrule)
{
- return call_int_hook(audit_rule_match, 0, secid, field, op, lsmrule);
+ struct security_hook_list *hp;
+ int rc;
+
+ hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
+ rc = hp->hook.audit_rule_match(blob->secid[hp->slot], field,
+ op, lsmrule);
+ if (rc != 0)
+ return rc;
+ }
+ return 0;
}
#endif /* CONFIG_AUDIT */
--
2.20.1
^ permalink raw reply related
* [PATCH v3 08/24] LSM: Use lsmblob in security_secctx_to_secid
From: Casey Schaufler @ 2019-06-21 18:52 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-1-casey@schaufler-ca.com>
Change security_secctx_to_secid() to fill in a lsmblob instead
of a u32 secid. Multiple LSMs may be able to interpret the
string, and this allows for setting whichever secid is
appropriate. In some cases there is scaffolding where other
interfaces have yet to be converted.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/security.h | 5 +++--
kernel/cred.c | 4 +---
net/netfilter/nft_meta.c | 13 ++++++-------
net/netfilter/xt_SECMARK.c | 5 ++++-
net/netlabel/netlabel_unlabeled.c | 14 ++++++++------
security/security.c | 16 +++++++++++++---
6 files changed, 35 insertions(+), 22 deletions(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 905830a90745..b0395d224c43 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -443,7 +443,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
int security_netlink_send(struct sock *sk, struct sk_buff *skb);
int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
-int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
+int security_secctx_to_secid(const char *secdata, u32 seclen,
+ struct lsmblob *blob);
void security_release_secctx(char *secdata, u32 seclen);
void security_inode_invalidate_secctx(struct inode *inode);
@@ -1226,7 +1227,7 @@ static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *secle
static inline int security_secctx_to_secid(const char *secdata,
u32 seclen,
- u32 *secid)
+ struct lsmblob *blob)
{
return -EOPNOTSUPP;
}
diff --git a/kernel/cred.c b/kernel/cred.c
index 71c14dda107e..d70a2c02ced4 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -725,14 +725,12 @@ EXPORT_SYMBOL(set_security_override);
int set_security_override_from_ctx(struct cred *new, const char *secctx)
{
struct lsmblob blob;
- u32 secid;
int ret;
- ret = security_secctx_to_secid(secctx, strlen(secctx), &secid);
+ ret = security_secctx_to_secid(secctx, strlen(secctx), &blob);
if (ret < 0)
return ret;
- lsmblob_init(&blob, secid);
return set_security_override(new, &blob);
}
EXPORT_SYMBOL(set_security_override_from_ctx);
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 987d2d6ce624..91973d3a5f6a 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -576,21 +576,20 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
static int nft_secmark_compute_secid(struct nft_secmark *priv)
{
- u32 tmp_secid = 0;
+ struct lsmblob blob;
int err;
- err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
+ err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
if (err)
return err;
- if (!tmp_secid)
- return -ENOENT;
-
- err = security_secmark_relabel_packet(tmp_secid);
+ /* Using le[0] is scaffolding */
+ err = security_secmark_relabel_packet(blob.secid[0]);
if (err)
return err;
- priv->secid = tmp_secid;
+ /* Using le[1] is scaffolding */
+ priv->secid = blob.secid[0];
return 0;
}
diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
index f16202d26c20..8081fadc30e9 100644
--- a/net/netfilter/xt_SECMARK.c
+++ b/net/netfilter/xt_SECMARK.c
@@ -49,13 +49,14 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
static int checkentry_lsm(struct xt_secmark_target_info *info)
{
+ struct lsmblob blob;
int err;
info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
info->secid = 0;
err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
- &info->secid);
+ &blob);
if (err) {
if (err == -EINVAL)
pr_info_ratelimited("invalid security context \'%s\'\n",
@@ -63,6 +64,8 @@ static int checkentry_lsm(struct xt_secmark_target_info *info)
return err;
}
+ /* scaffolding during the transition */
+ info->secid = blob.secid[0];
if (!info->secid) {
pr_info_ratelimited("unable to map security context \'%s\'\n",
info->secctx);
diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index c92894c3e40a..2976370e41aa 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -895,7 +895,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
void *addr;
void *mask;
u32 addr_len;
- u32 secid;
+ struct lsmblob blob;
struct netlbl_audit audit_info;
/* Don't allow users to add both IPv4 and IPv6 addresses for a
@@ -919,12 +919,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
ret_val = security_secctx_to_secid(
nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
- &secid);
+ &blob);
if (ret_val != 0)
return ret_val;
+ /* scaffolding with the [0] */
return netlbl_unlhsh_add(&init_net,
- dev_name, addr, mask, addr_len, secid,
+ dev_name, addr, mask, addr_len, blob.secid[0],
&audit_info);
}
@@ -946,7 +947,7 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
void *addr;
void *mask;
u32 addr_len;
- u32 secid;
+ struct lsmblob blob;
struct netlbl_audit audit_info;
/* Don't allow users to add both IPv4 and IPv6 addresses for a
@@ -968,12 +969,13 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
ret_val = security_secctx_to_secid(
nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
- &secid);
+ &blob);
if (ret_val != 0)
return ret_val;
+ /* scaffolding with the [0] */
return netlbl_unlhsh_add(&init_net,
- NULL, addr, mask, addr_len, secid,
+ NULL, addr, mask, addr_len, blob.secid[0],
&audit_info);
}
diff --git a/security/security.c b/security/security.c
index c7b3d1a294ad..cb1545bfe8c5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1999,10 +1999,20 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
}
EXPORT_SYMBOL(security_secid_to_secctx);
-int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
+int security_secctx_to_secid(const char *secdata, u32 seclen,
+ struct lsmblob *blob)
{
- *secid = 0;
- return call_int_hook(secctx_to_secid, 0, secdata, seclen, secid);
+ struct security_hook_list *hp;
+ int rc;
+
+ lsmblob_init(blob, 0);
+ hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
+ rc = hp->hook.secctx_to_secid(secdata, seclen,
+ &blob->secid[hp->slot]);
+ if (rc != 0)
+ return rc;
+ }
+ return 0;
}
EXPORT_SYMBOL(security_secctx_to_secid);
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox