Re: [PATCH RFT v4 2/5] fork: Add shadow stack support to clone3()

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

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;