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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <61f80d032c6a630dd641c9b598b37c2eb40d51e8.camel@intel.com>
Date:   Tue, 5 Dec 2023 00:26:57 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "debug@...osinc.com" <debug@...osinc.com>,
        "mgorman@...e.de" <mgorman@...e.de>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "vschneid@...hat.com" <vschneid@...hat.com>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "bristot@...hat.com" <bristot@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "bp@...en8.de" <bp@...en8.de>,
        "bsegall@...gle.com" <bsegall@...gle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>
CC:     "keescook@...omium.org" <keescook@...omium.org>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH RFT v4 2/5] fork: Add shadow stack support to clone3()

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;