Re: [PATCH] riscv: Add STACKPROTECTOR supported

From: Guo Ren <guoren@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: "Palmer Dabbelt" <palmerdabbelt@google.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Anup Patel" <anup@brainfault.org>,
	"Greentime Hu" <greentime.hu@sifive.com>,
	"Zong Li" <zong.li@sifive.com>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-csky@vger.kernel.org, "Guo Ren" <guoren@linux.alibaba.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Masami Hiramatsu" <mhiramat@kernel.org>,
	"Björn Töpel" <bjorn.topel@gmail.com>,
	"Greentime Hu" <green.hu@gmail.com>,
	"Atish Patra" <atish.patra@wdc.com>
Subject: Re: [PATCH] riscv: Add STACKPROTECTOR supported
Date: Mon, 6 Jul 2020 08:55:35 +0800	[thread overview]
Message-ID: <CAJF2gTTCEEoP3a9i_REXPwkJZ4Si_hYfjk_hwqHxkyfgw_xBLw@mail.gmail.com> (raw)
In-Reply-To: <202007051328.FE9EF99@keescook>

On Mon, Jul 6, 2020 at 4:31 AM Kees Cook <keescook@chromium.org> wrote:
>
> 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@chromium.org> wrote:
> > > On Sun, Jul 05, 2020 at 06:24:15AM +0000, guoren@kernel.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?)
Yes, it's just a csr read. But it's necessary? get_random_bytes should enough.

>
> > >
> > > > +
> > > > +     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.
Maybe some race condition problems. When canary changed the in the
switch to, but other CPUs still get that value concurrently.

>
> > #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...)
CONFIG_SMP is mostly default for me and let's talk about arm64/x86
per-task solution. That is the right way.


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/