Re: [PATCH v6 16/33] riscv/shstk: If needed allocate a new shadow stack on clone

[Deepak Gupta]

On Tue, Oct 08, 2024 at 10:55:29PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2024-10-08 at 15:36 -0700, Deepak Gupta wrote:
> > +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
> > +					   const struct kernel_clone_args *args)
> > +{
> > +	unsigned long addr, size;
> > +
> > +	/* If shadow stack is not supported, return 0 */
> > +	if (!cpu_supports_shadow_stack())
> > +		return 0;
> > +
> > +	/*
> > +	 * If shadow stack is not enabled on the new thread, skip any
> > +	 * switch to a new shadow stack.
> > +	 */
> > +	if (!is_shstk_enabled(tsk))
> > +		return 0;
> > +
> > +	/*
> > +	 * For CLONE_VFORK the child will share the parents shadow stack.
> > +	 * Set base = 0 and size = 0, this is special means to track this state
> > +	 * so the freeing logic run for child knows to leave it alone.
> > +	 */
> > +	if (args->flags & CLONE_VFORK) {
> > +		set_shstk_base(tsk, 0, 0);
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * For !CLONE_VM the child will use a copy of the parents shadow
> > +	 * stack.
> > +	 */
> > +	if (!(args->flags & CLONE_VM))
> > +		return 0;
> > +
> > +	/*
> > +	 * reaching here means, CLONE_VM was specified and thus a separate shadow
> > +	 * stack is needed for new cloned thread. Note: below allocation is happening
> > +	 * using current mm.
> > +	 */
> > +	size = calc_shstk_size(args->stack_size);
> > +	addr = allocate_shadow_stack(0, size, 0, false);
> > +	if (IS_ERR_VALUE(addr))
> > +		return addr;
> > +
> > +	set_shstk_base(tsk, addr, size);
> > +
> > +	return addr + size;
> > +}
>
> A lot of this patch and the previous one is similar to x86's and arm's. It great
> that we can have consistency around this behavior.
>
> There might be enough consistency to refactor some of the arch code into a
> kernel/shstk.c.
>
> Should we try?

Yeah you're right. Honestly, I've been shameless in adapting most of the flows
from x86 `shstk.c` for risc-v. So thank you for that.

Now that we've `ARCH_HAS_USER_SHADOW_STACK` part of multiple patch series (riscv
shadowstack, clone3 and I think arm64 gcs series as well). It's probably the
appropriate time to find common grounds.

This is what I suggest

- move most of the common/arch agnostic shadow stack stuff in kernel/shstk.c
  This gets part of compile if `ARCH_HAS_USER_SHADOW_STACK` is enabled/selected.

- allow arch specific branch out guard checks for "if cpu supports", "is shadow stack
  enabled on the task_struct" (I expect each arch layout of task_struct will be
  different, no point finding common ground there), etc.

I think it's worth a try. 
If you already don't have patches, I'll spend some time to see what it takes to
converge in my next version. If I end up into some roadblock, will use this thread
for further discussion.