Re: [RFC PATCH v1 24/28] riscv: select config for shadow stack and landing pad instr support

[Deepak Gupta]

On Thu, Jan 25, 2024 at 06:44:48PM +0000, Conor Dooley wrote:
> On Thu, Jan 25, 2024 at 10:12:16AM -0800, Deepak Gupta wrote:
> > On Thu, Jan 25, 2024 at 06:04:26PM +0000, Conor Dooley wrote:
> > > On Wed, Jan 24, 2024 at 10:21:49PM -0800, debug@xxxxxxxxxxxx wrote:
> > > > From: Deepak Gupta <debug@xxxxxxxxxxxx>
> > > >
> > > > This patch selects config shadow stack support and landing pad instr
> > > > support. Shadow stack support and landing instr support is hidden behind
> > > > `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.
> > > >
> > > > Signed-off-by: Deepak Gupta <debug@xxxxxxxxxxxx>
> > > > ---
> > > >  arch/riscv/Kconfig | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index 9d386e9edc45..437b2f9abf3e 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -163,6 +163,7 @@ config RISCV
> > > >  	select SYSCTL_EXCEPTION_TRACE
> > > >  	select THREAD_INFO_IN_TASK
> > > >  	select TRACE_IRQFLAGS_SUPPORT
> > > > +	select RISCV_USER_CFI
> > >
> > > This select makes no sense to me, it will unconditionally enable
> > > RISCV_USER_CFI. I don't think that that is your intent, since you have a
> > > detailed option below that allows the user to turn it on or off.
> > >
> > > If you remove it, the commit message will need to change too FYI.
> > >
> >
> > Selecting this config puts support in Kernel so that it can run tasks who wants
> > to enable hardware assisted control flow integrity for themselves. But apps still
> > always need to optin using `prctls`. Those prctls are stubs and return EINVAL when
> > this config is not selected. Not selecting this config means, kernel will not support
> > enabling this feature for user mode.
>
> I don't think you understand me. "select RISCV_USER_CFI" will
> unconditionally build it into the kernel, making stubs etc useless.
> You're talking like (and the rest of your commit implements it!) that
> this feature can be enabled in menuconfig etc. Having this select
> will always enable the config option, rendering the choice below
> redundant. Try turning it off in menuconfig.

Aah got it now. Thanks.
I'll fix this messaging and select in next version.

> Oh and if it were valid to have here, you put it in out of order. That's
> an alphanumerically sorted list :)

Thanks for pointing that out. Will fix it.
> Cheers,
> Conor.