Re: [PATCH v20 25/25] mm: Introduce PROT_SHSTK for shadow stack

Kees Cook
2021-02-10T19:47:22Z

On Wed, Feb 10, 2021 at 09:57:03AM -0800, Yu-cheng Yu wrote:
> There are three possible options to create a shadow stack allocation API:
> an arch_prctl, a new syscall, or adding PROT_SHSTK to mmap()/mprotect().
> Each has its advantages and compromises.
>
> An arch_prctl() is the least intrusive. However, the existing x86
> arch_prctl() takes only two parameters. Multiple parameters must be
> passed in a memory buffer. There is a proposal to pass more parameters in
> registers [1], but no active discussion on that.
>
> A new syscall minimizes compatibility issues and offers an extensible frame
> work to other architectures, but this will likely result in some overlap of
> mmap()/mprotect().
>
> The introduction of PROT_SHSTK to mmap()/mprotect() takes advantage of
> existing APIs. The x86-specific PROT_SHSTK is translated to VM_SHSTK and
> a shadow stack mapping is created without reinventing the wheel. There are
> potential pitfalls though. The most obvious one would be using this as a
> bypass to shadow stack protection. However, the attacker would have to get
> to the syscall first.
>
> Since arch_calc_vm_prot_bits() is modified, I have moved arch_vm_get_page
> _prot() and arch_calc_vm_prot_bits() to x86/include/asm/mman.h.
> This will be more consistent with other architectures.

This portion of the patch seems logically separate from the PROT_SHSTK
implementation. Can you please separate it into its own patch?

>
> [1] https://lore.kernel.org/lkml/20200828121624.108243-1-hjl.tools@gmail.com/
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

With that done:

Reviewed-by: Kees Cook <keescook@chromium.org>

--
Kees Cook