Message-ID: <CAKC1njQHu1UeSSApWHXedEURBezuQL1BDcrpsSfi=D0JmDFX8A@mail.gmail.com>
Date: Thu, 20 Mar 2025 15:29:55 -0700
From: Deepak Gupta <debug@...osinc.com>
To: Radim Krčmář <rkrcmar@...tanamicro.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	"Liam R. Howlett" <Liam.Howlett@...cle.com>, Vlastimil Babka <vbabka@...e.cz>, 
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Conor Dooley <conor@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Arnd Bergmann <arnd@...db.de>, 
	Christian Brauner <brauner@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Oleg Nesterov <oleg@...hat.com>, Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>, Jann Horn <jannh@...gle.com>, 
	Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, 
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-arch@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, alistair.francis@....com, 
	richard.henderson@...aro.org, jim.shu@...ive.com, andybnac@...il.com, 
	kito.cheng@...ive.com, charlie@...osinc.com, atishp@...osinc.com, 
	evan@...osinc.com, cleger@...osinc.com, alexghiti@...osinc.com, 
	samitolvanen@...gle.com, broonie@...nel.org, rick.p.edgecombe@...el.com, 
	Zong Li <zong.li@...ive.com>, linux-riscv <linux-riscv-bounces@...ts.infradead.org>
Subject: Re: [PATCH v12 25/28] riscv: create a config for shadow stack and
 landing pad instr support

On Thu, Mar 20, 2025 at 2:25 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
>
> 2025-03-14T14:39:44-07:00, Deepak Gupta <debug@...osinc.com>:
> > This patch creates a config for shadow stack support and landing pad instr
> > support. Shadow stack support and landing instr support can be enabled by
> > selecting `CONFIG_RISCV_USER_CFI`. Selecting `CONFIG_RISCV_USER_CFI` wires
> > up path to enumerate CPU support and if cpu support exists, kernel will
> > support cpu assisted user mode cfi.
> >
> > If CONFIG_RISCV_USER_CFI is selected, select `ARCH_USES_HIGH_VMA_FLAGS`,
> > `ARCH_HAS_USER_SHADOW_STACK` and DYNAMIC_SIGFRAME for riscv.
> >
> > Reviewed-by: Zong Li <zong.li@...ive.com>
> > Signed-off-by: Deepak Gupta <debug@...osinc.com>
> > ---
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > @@ -250,6 +250,26 @@ config ARCH_HAS_BROKEN_DWARF5
> > +config RISCV_USER_CFI
> > +     def_bool y
> > +     bool "riscv userspace control flow integrity"
> > +     depends on 64BIT && $(cc-option,-mabi=lp64 -march=rv64ima_zicfiss)
> > +     depends on RISCV_ALTERNATIVE
> > +     select ARCH_HAS_USER_SHADOW_STACK
> > +     select ARCH_USES_HIGH_VMA_FLAGS
> > +     select DYNAMIC_SIGFRAME
> > +     help
> > +       Provides CPU assisted control flow integrity to userspace tasks.
> > +       Control flow integrity is provided by implementing shadow stack for
> > +       backward edge and indirect branch tracking for forward edge in program.
> > +       Shadow stack protection is a hardware feature that detects function
> > +       return address corruption. This helps mitigate ROP attacks.
> > +       Indirect branch tracking enforces that all indirect branches must land
> > +       on a landing pad instruction else CPU will fault. This mitigates against
> > +       JOP / COP attacks. Applications must be enabled to use it, and old user-
> > +       space does not get protection "for free".
> > +       default y
>
> A high level question to kick off my review:
>
> Why are landing pads and shadow stacks merged together?
>
> Apart from adding build flexibility, we could also split the patches
> into two isolated series, because the features are independent.

Strictly from CPU extensions point of view they are independent features.
Although from a usability point of view they complement each other. A user
wanting to enable support for control flow integrity wouldn't be enabling
only landing pad and leaving return flow open for an attacker and vice-versa.
That's why I defined a single CONFIG for CFI.

>From organizing patches in the patch series, shadow stack and landing
pad patches do not cross into each other and are different from each
other except dt-bindings, hwprobe, csr definitions. I can separate them
out as well if that's desired.

Furthermore, I do not see an implementation only implementing zicfilp
while not implementing zicfiss. There is a case of a nommu case where
only zicfilp might be implemented and no zicfiss. However that's the case
which is anyways riscv linux kernel is not actively being tested. IIRC,
it was (nommu linux) considered to be phased out/not supported as well.

We could have two different configs but I don't see what would serve
apart from the ability to build support for landing pad and shadow stack
differently. As I said from a usability point of view both features
are complimenting
to each other rather than standing out alone and providing full protection.

A kernel is built with support for enabling both features or none. Sure user
can use either of the prctl to enable either of the features in whatever
combination they see fit.

Powered by blists - more mailing lists