On Tue, 2023-11-28 at 18:22 +0000, Mark Brown wrote:
> -unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
> unsigned long clone_flags,
> - unsigned long stack_size)
> +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
> + const struct kernel_clone_args
> *args)
> {
> struct thread_shstk *shstk = &tsk->thread.shstk;
> + unsigned long clone_flags = args->flags;
> unsigned long addr, size;
>
> /*
> * If shadow stack is not enabled on the new thread, skip any
> - * switch to a new shadow stack.
> + * implicit switch to a new shadow stack and reject attempts
> to
> + * explciitly specify one.
> */
> - if (!features_enabled(ARCH_SHSTK_SHSTK))
> - return 0;
> + if (!features_enabled(ARCH_SHSTK_SHSTK)) {
> + if (args->shadow_stack_size)
> + return (unsigned long)ERR_PTR(-EINVAL);
>
> - /*
> - * For CLONE_VFORK the child will share the parents shadow
> stack.
> - * Make sure to clear the internal tracking of the thread
> shadow
> - * stack so the freeing logic run for child knows to leave it
> alone.
> - */
> - if (clone_flags & CLONE_VFORK) {
> - shstk->base = 0;
> - shstk->size = 0;
> return 0;
> }
>
> /*
> - * For !CLONE_VM the child will use a copy of the parents
> shadow
> - * stack.
> + * If the user specified a shadow stack then do some basic
> + * validation and use it, otherwise fall back to a default
> + * shadow stack size if the clone_flags don't indicate an
> + * allocation is unneeded.
> */
> - if (!(clone_flags & CLONE_VM))
> - return 0;
> + if (args->shadow_stack_size) {
> + size = args->shadow_stack_size;
> + } else {
> + /*
> + * For CLONE_VFORK the child will share the parents
> + * shadow stack. Make sure to clear the internal
> + * tracking of the thread shadow stack so the freeing
> + * logic run for child knows to leave it alone.
> + */
> + if (clone_flags & CLONE_VFORK) {
> + shstk->base = 0;
> + shstk->size = 0;
> + return 0;
> + }
> +
> + /*
> + * For !CLONE_VM the child will use a copy of the
> + * parents shadow stack.
> + */
> + if (!(clone_flags & CLONE_VM))
> + return 0;
> +
> + size = args->stack_size;
> +
> + }
>
> - size = adjust_shstk_size(stack_size);
> + size = adjust_shstk_size(size);
> addr = alloc_shstk(0, size, 0, false);
Hmm. I didn't test this, but in the copy_process(), copy_mm() happens
before this point. So the shadow stack would get mapped in current's MM
(i.e. the parent). So in the !CLONE_VM case with shadow_stack_size!=0
the SSP in the child will be updated to an area that is not mapped in
the child. I think we need to pass tsk->mm into alloc_shstk(). But such
an exotic clone usage does give me pause, regarding whether all of this
is premature.
Otherwise it looked ok from the x86/shstk perspective.
> if (IS_ERR_VALUE(addr))
> return addr;
