linux-kernel - Re: [PATCH] riscv: Add STACKPROTECTOR supported

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]

Date:   Sun, 5 Jul 2020 13:31:52 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Guo Ren <guoren@...nel.org>
Cc:     Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Anup Patel <anup@...infault.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        Zong Li <zong.li@...ive.com>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-csky@...r.kernel.org, Guo Ren <guoren@...ux.alibaba.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Björn Töpel <bjorn.topel@...il.com>,
        Greentime Hu <green.hu@...il.com>,
        Atish Patra <atish.patra@....com>
Subject: Re: [PATCH] riscv: Add STACKPROTECTOR supported

On Sun, Jul 05, 2020 at 10:16:14PM +0800, Guo Ren wrote:
> On Sun, Jul 5, 2020 at 2:53 PM Kees Cook <keescook@...omium.org> wrote:
> > On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@...nel.org wrote:
> > > +static __always_inline void boot_init_stack_canary(void)
> > > +{
> > > +     unsigned long canary;
> > > +
> > > +     /* Try to get a semi random initial value. */
> > > +     get_random_bytes(&canary, sizeof(canary));
> > > +     canary ^= LINUX_VERSION_CODE;
> > > +     canary &= CANARY_MASK;
> >
> > Does riscv have any kind of instruction counters or other trivial timers
> > that could be mixed in here? (e.g. x86's TSC)
> Do you mean:
>   get_random_bytes(&canary, sizeof(canary));
> + canary += get_cycles64() + (get_cycles64() << 32UL);
>   canary ^= LINUX_VERSION_CODE;
>   canary &= CANARY_MASK;
> 
> Ok ?

Sure -- I assume get_cycles64() is architecturally "simple"? (i.e. it
doesn't require that the entire time-keeping subsystem has started?)

> >
> > > +
> > > +     current->stack_canary = canary;
> > > +     __stack_chk_guard = current->stack_canary;
> >
> > What's needed for riscv to support a per-task canary? (e.g. x86's TLS or
> > arm64's register-specific methods)
> Some archs change __stack_chk_guard in _switch_to of entry.S, but it
> depends on !CONFIG_SMP.

Oh, funny. I hadn't actually noticed that logic for the !CONFIG_SMP
cases. I see to problem with that, but the more important case, I think
is the per-task canaries.

> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
>         get  value  from next_task->stack_canary
>         store  value to __stack_chk_guard
> #endif
> 
> It's a so limitation solution for per-task canary, so I didn't copy it
> into riscv?

Right -- it's a limited solution. On the other had, is !CONFIG_SMP
expected to be a common config for riscv? If so, it's worth adding. If
not, I'd say skip it. (Though it looks very simple to do...)

-- 
Kees Cook