linux-kernel - Re: [PATCH v2 2/2] arm64: scs: use vmapped IRQ and SDEI shadow stacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201201101819.GA26471@willie-the-truck>
Date:   Tue, 1 Dec 2020 10:18:19 +0000
From:   Will Deacon <will@...nel.org>
To:     Sami Tolvanen <samitolvanen@...gle.com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@....com>,
        James Morse <james.morse@....com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Kees Cook <keescook@...omium.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] arm64: scs: use vmapped IRQ and SDEI shadow stacks

On Mon, Nov 30, 2020 at 01:13:07PM -0800, Sami Tolvanen wrote:
> On Mon, Nov 30, 2020 at 3:49 AM Will Deacon <will@...nel.org> wrote:
> >
> > On Tue, Nov 24, 2020 at 11:59:40AM -0800, Sami Tolvanen wrote:
> > > Use scs_alloc() to allocate also IRQ and SDEI shadow stacks instead of
> > > using statically allocated stacks.
> > >
> > > Signed-off-by: Sami Tolvanen <samitolvanen@...gle.com>
> > > ---
> > >  arch/arm64/kernel/Makefile |  1 -
> > >  arch/arm64/kernel/entry.S  |  6 ++--
> > >  arch/arm64/kernel/irq.c    | 19 ++++++++++
> > >  arch/arm64/kernel/scs.c    | 16 ---------
> > >  arch/arm64/kernel/sdei.c   | 71 +++++++++++++++++++++++++++++++-------
> > >  include/linux/scs.h        |  4 ---
> > >  6 files changed, 81 insertions(+), 36 deletions(-)
> > >  delete mode 100644 arch/arm64/kernel/scs.c
> >
> > [...]
> >
> > > @@ -70,18 +97,40 @@ static int _init_sdei_stack(unsigned long * __percpu *ptr, int cpu)
> > >       return 0;
> > >  }
> > >
> > > +static int _init_sdei_scs(unsigned long * __percpu *ptr, int cpu)
> > > +{
> > > +     void *s;
> > > +
> > > +     s = scs_alloc(cpu_to_node(cpu));
> > > +     if (!s)
> > > +             return -ENOMEM;
> > > +     per_cpu(*ptr, cpu) = s;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int init_sdei_stacks(void)
> > >  {
> > >       int cpu;
> > >       int err = 0;
> > >
> > >       for_each_possible_cpu(cpu) {
> > > -             err = _init_sdei_stack(&sdei_stack_normal_ptr, cpu);
> > > -             if (err)
> > > -                     break;
> > > -             err = _init_sdei_stack(&sdei_stack_critical_ptr, cpu);
> > > -             if (err)
> > > -                     break;
> > > +             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > +                     err = _init_sdei_stack(&sdei_stack_normal_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +                     err = _init_sdei_stack(&sdei_stack_critical_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +             }
> > > +             if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> > > +                     err = _init_sdei_scs(&sdei_shadow_call_stack_normal_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +                     err = _init_sdei_scs(&sdei_shadow_call_stack_critical_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> >
> > This looks ok to me, but I think it would be better to follow the same
> > approach as you have for the IRQ stacks and instead have a separate
> > init_sdei_scs() function (similarly for the free() path), which means
> > you can simply the IS_ENABLED() checks too.
> 
> OK, I can change this in v3. It makes error handling in
> sdei_arch_get_entry_point() a bit more awkward though. We'll need
> something like this:
> 
>         if (IS_ENABLED(CONFIG_VMAP_STACK)) {
>                 if (init_sdei_stacks())
>                         return 0;
>         }
> 
>         if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>                 if (init_sdei_scs()) {
>                         if (IS_ENABLED(CONFIG_VMAP_STACK))
>                                 free_sdei_stacks();
>                         return 0;
>                 }

Can you push the IS_ENABLED() checks into their respective functions?
Then you can do something like:

	if (init_sdei_stacks())
		return 0;

	if (init_sdei_scs())
		goto out_free_stacks;

	...

out_free_stacks:
	free_sdei_stacks();
	return 0;


Will