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;