[v7,22/41] mm/mmap: Add shadow stack pages to memory accounting

  From: Yu-cheng Yu <yu-cheng.yu@intel.com>

The x86 Control-flow Enforcement Technology (CET) feature includes a new
type of memory called shadow stack. This shadow stack memory has some
unusual properties, which requires some core mm changes to function
properly.

Account shadow stack pages to stack memory. Do this by adding a
VM_SHADOW_STACK check in is_stack_mapping().

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <keescook@chromium.org>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Kees Cook <keescook@chromium.org>

---
v7:
 - Change is_stack_mapping() to know about VM_SHADOW_STACK so the
   additions in vm_stat_account() can be dropped. (David Hildenbrand)

v3:
 - Remove unneeded VM_SHADOW_STACK check in accountable_mapping()
   (Kirill)

v2:
 - Remove is_shadow_stack_mapping() and just change it to directly bitwise
   and VM_SHADOW_STACK.

Yu-cheng v26:
 - Remove redundant #ifdef CONFIG_MMU.

Yu-cheng v25:
 - Remove #ifdef CONFIG_ARCH_HAS_SHADOW_STACK for is_shadow_stack_mapping().
---
 mm/internal.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

On Mon, Feb 27, 2023 at 02:29:38PM -0800, Rick Edgecombe wrote:
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> 
> The x86 Control-flow Enforcement Technology (CET) feature includes a new
> type of memory called shadow stack. This shadow stack memory has some
> unusual properties, which requires some core mm changes to function
> properly.
> 
> Account shadow stack pages to stack memory. Do this by adding a
> VM_SHADOW_STACK check in is_stack_mapping().

That last sentence is superfluous.
  

On Mon, 2023-03-06 at 14:01 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:38PM -0800, Rick Edgecombe wrote:
> > From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > 
> > The x86 Control-flow Enforcement Technology (CET) feature includes
> > a new
> > type of memory called shadow stack. This shadow stack memory has
> > some
> > unusual properties, which requires some core mm changes to function
> > properly.
> > 
> > Account shadow stack pages to stack memory. Do this by adding a
> > VM_SHADOW_STACK check in is_stack_mapping().
> 
> That last sentence is superfluous.

Before this version it was open coded, but David Hildenbrand suggested
this is_stack_mapping() solution. Should it be explained more, or just
dropped?
  

On Mon, Mar 06, 2023 at 06:11:32PM +0000, Edgecombe, Rick P wrote:
> Before this version it was open coded, but David Hildenbrand suggested
> this is_stack_mapping() solution. Should it be explained more, or just
> dropped?

Well, "adding a VM_SHADOW_STACK check in is_stack_mapping()" is what's
in the diff already and it is kinda obvious. So why write what the patch
does when one can simply look at the diff?
  

On 27.02.23 23:29, Rick Edgecombe wrote:
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> 
> The x86 Control-flow Enforcement Technology (CET) feature includes a new
> type of memory called shadow stack. This shadow stack memory has some
> unusual properties, which requires some core mm changes to function
> properly.
> 
> Account shadow stack pages to stack memory. Do this by adding a
> VM_SHADOW_STACK check in is_stack_mapping().
> 
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> 
> ---
> v7:
>   - Change is_stack_mapping() to know about VM_SHADOW_STACK so the
>     additions in vm_stat_account() can be dropped. (David Hildenbrand)
> 
> v3:
>   - Remove unneeded VM_SHADOW_STACK check in accountable_mapping()
>     (Kirill)
> 
> v2:
>   - Remove is_shadow_stack_mapping() and just change it to directly bitwise
>     and VM_SHADOW_STACK.
> 
> Yu-cheng v26:
>   - Remove redundant #ifdef CONFIG_MMU.
> 
> Yu-cheng v25:
>   - Remove #ifdef CONFIG_ARCH_HAS_SHADOW_STACK for is_shadow_stack_mapping().
> ---
>   mm/internal.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 7920a8b7982e..1d13d5580f64 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -491,14 +491,14 @@ static inline bool is_exec_mapping(vm_flags_t flags)
>   }
>   
>   /*
> - * Stack area - automatically grows in one direction
> + * Stack area


Maybe "Stack area (including shadow stacks)"


Acked-by: David Hildenbrand <david@redhat.com>
  

On Mon, Feb 27, 2023 at 2:31 PM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> From: Yu-cheng Yu <yu-cheng.yu@intel.com>
>
> The x86 Control-flow Enforcement Technology (CET) feature includes a new
> type of memory called shadow stack. This shadow stack memory has some
> unusual properties, which requires some core mm changes to function
> properly.
>
> Account shadow stack pages to stack memory. Do this by adding a
> VM_SHADOW_STACK check in is_stack_mapping().
>
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Kees Cook <keescook@chromium.org>
>
> ---
> v7:
>  - Change is_stack_mapping() to know about VM_SHADOW_STACK so the
>    additions in vm_stat_account() can be dropped. (David Hildenbrand)
>
> v3:
>  - Remove unneeded VM_SHADOW_STACK check in accountable_mapping()
>    (Kirill)
>
> v2:
>  - Remove is_shadow_stack_mapping() and just change it to directly bitwise
>    and VM_SHADOW_STACK.
>
> Yu-cheng v26:
>  - Remove redundant #ifdef CONFIG_MMU.
>
> Yu-cheng v25:
>  - Remove #ifdef CONFIG_ARCH_HAS_SHADOW_STACK for is_shadow_stack_mapping().
> ---
>  mm/internal.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/internal.h b/mm/internal.h
> index 7920a8b7982e..1d13d5580f64 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -491,14 +491,14 @@ static inline bool is_exec_mapping(vm_flags_t flags)
>  }
>
>  /*
> - * Stack area - automatically grows in one direction
> + * Stack area
>   *
> - * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
> - * do_mmap() forbids all other combinations.
> + * VM_GROWSUP, VM_GROWSDOWN VMAs are always private
> + * anonymous. do_mmap() forbids all other combinations.
>   */
>  static inline bool is_stack_mapping(vm_flags_t flags)
>  {
> -       return (flags & VM_STACK) == VM_STACK;
> +       return ((flags & VM_STACK) == VM_STACK) || (flags & VM_SHADOW_STACK);

Same comment here. `VM_SHADOW_STACK` is an x86 specific way of
encoding a shadow stack.
Instead let's have a proxy here which allows architectures to have
their own encodings to represent a shadow stack.

>  }
>
>  /*
> --
> 2.17.1
>
  

On 3/17/23 10:12, Deepak Gupta wrote:
>>  /*
>> - * Stack area - automatically grows in one direction
>> + * Stack area
>>   *
>> - * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
>> - * do_mmap() forbids all other combinations.
>> + * VM_GROWSUP, VM_GROWSDOWN VMAs are always private
>> + * anonymous. do_mmap() forbids all other combinations.
>>   */
>>  static inline bool is_stack_mapping(vm_flags_t flags)
>>  {
>> -       return (flags & VM_STACK) == VM_STACK;
>> +       return ((flags & VM_STACK) == VM_STACK) || (flags & VM_SHADOW_STACK);
> Same comment here. `VM_SHADOW_STACK` is an x86 specific way of
> encoding a shadow stack.
> Instead let's have a proxy here which allows architectures to have
> their own encodings to represent a shadow stack.

This doesn't _preclude_ another architecture from coming along and doing
that, right?  I'd just prefer that shadow stack architecture #2 comes
along and refactors this in precisely the way _they_ need it.
  

On Fri, Mar 17, 2023 at 10:16 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 3/17/23 10:12, Deepak Gupta wrote:
> >>  /*
> >> - * Stack area - automatically grows in one direction
> >> + * Stack area
> >>   *
> >> - * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
> >> - * do_mmap() forbids all other combinations.
> >> + * VM_GROWSUP, VM_GROWSDOWN VMAs are always private
> >> + * anonymous. do_mmap() forbids all other combinations.
> >>   */
> >>  static inline bool is_stack_mapping(vm_flags_t flags)
> >>  {
> >> -       return (flags & VM_STACK) == VM_STACK;
> >> +       return ((flags & VM_STACK) == VM_STACK) || (flags & VM_SHADOW_STACK);
> > Same comment here. `VM_SHADOW_STACK` is an x86 specific way of
> > encoding a shadow stack.
> > Instead let's have a proxy here which allows architectures to have
> > their own encodings to represent a shadow stack.
>
> This doesn't _preclude_ another architecture from coming along and doing
> that, right?  I'd just prefer that shadow stack architecture #2 comes
> along and refactors this in precisely the way _they_ need it.

There are two issues here
 - Encoding of shadow stack: Another arch can choose different encoding.
   And yes, another architecture can come in and re-factor it. But so
much thought and work has been given to x86 implementation to keep
   shadow stack to not impact arch agnostic parts of the kernel. So
why creep it in here.

- VM_SHADOW_STACK is coming out of the VM_HIGH_ARCH_XX bit position
which makes it arch specific.

If re-factor takes care then I would say the 2nd issue still exists,
it's better to keep it away from arch agnostic code.
  

On Fri, 2023-03-17 at 10:28 -0700, Deepak Gupta wrote:
> On Fri, Mar 17, 2023 at 10:16 AM Dave Hansen <dave.hansen@intel.com>
> wrote:
> > 
> > On 3/17/23 10:12, Deepak Gupta wrote:
> > > >   /*
> > > > - * Stack area - automatically grows in one direction
> > > > + * Stack area
> > > >    *
> > > > - * VM_GROWSUP / VM_GROWSDOWN VMAs are always private
> > > > anonymous:
> > > > - * do_mmap() forbids all other combinations.
> > > > + * VM_GROWSUP, VM_GROWSDOWN VMAs are always private
> > > > + * anonymous. do_mmap() forbids all other combinations.
> > > >    */
> > > >   static inline bool is_stack_mapping(vm_flags_t flags)
> > > >   {
> > > > -       return (flags & VM_STACK) == VM_STACK;
> > > > +       return ((flags & VM_STACK) == VM_STACK) || (flags &
> > > > VM_SHADOW_STACK);
> > > 
> > > Same comment here. `VM_SHADOW_STACK` is an x86 specific way of
> > > encoding a shadow stack.
> > > Instead let's have a proxy here which allows architectures to
> > > have
> > > their own encodings to represent a shadow stack.
> > 
> > This doesn't _preclude_ another architecture from coming along and
> > doing
> > that, right?  I'd just prefer that shadow stack architecture #2
> > comes
> > along and refactors this in precisely the way _they_ need it.
> 
> There are two issues here
>  - Encoding of shadow stack: Another arch can choose different
> encoding.
>    And yes, another architecture can come in and re-factor it. But so
> much thought and work has been given to x86 implementation to keep
>    shadow stack to not impact arch agnostic parts of the kernel. So
> why creep it in here.
> 
> - VM_SHADOW_STACK is coming out of the VM_HIGH_ARCH_XX bit position
> which makes it arch specific.
> 
> 

VM_SHADOW_STACK is defined like this (trimmed for clarity):
#ifdef CONFIG_X86_USER_SHADOW_STACK
# define VM_SHADOW_STACK	VM_HIGH_ARCH_5
#else
# define VM_SHADOW_STACK	VM_NONE
#endif

Also, we actually had an is_shadow_stack_mapping(vma) in the past, but
it was dropped from other feedback. I think it might be too soon to say
whether other implementations won't end up with a similar vma flag, so
this would be premature refactoring. If not though, a helper like that
seems like a reasonable solution.
  

On Fri, Mar 17, 2023 at 10:42 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Fri, 2023-03-17 at 10:28 -0700, Deepak Gupta wrote:
> > On Fri, Mar 17, 2023 at 10:16 AM Dave Hansen <dave.hansen@intel.com>
> > wrote:
> > >
> > > On 3/17/23 10:12, Deepak Gupta wrote:
> > > > >   /*
> > > > > - * Stack area - automatically grows in one direction
> > > > > + * Stack area
> > > > >    *
> > > > > - * VM_GROWSUP / VM_GROWSDOWN VMAs are always private
> > > > > anonymous:
> > > > > - * do_mmap() forbids all other combinations.
> > > > > + * VM_GROWSUP, VM_GROWSDOWN VMAs are always private
> > > > > + * anonymous. do_mmap() forbids all other combinations.
> > > > >    */
> > > > >   static inline bool is_stack_mapping(vm_flags_t flags)
> > > > >   {
> > > > > -       return (flags & VM_STACK) == VM_STACK;
> > > > > +       return ((flags & VM_STACK) == VM_STACK) || (flags &
> > > > > VM_SHADOW_STACK);
> > > >
> > > > Same comment here. `VM_SHADOW_STACK` is an x86 specific way of
> > > > encoding a shadow stack.
> > > > Instead let's have a proxy here which allows architectures to
> > > > have
> > > > their own encodings to represent a shadow stack.
> > >
> > > This doesn't _preclude_ another architecture from coming along and
> > > doing
> > > that, right?  I'd just prefer that shadow stack architecture #2
> > > comes
> > > along and refactors this in precisely the way _they_ need it.
> >
> > There are two issues here
> >  - Encoding of shadow stack: Another arch can choose different
> > encoding.
> >    And yes, another architecture can come in and re-factor it. But so
> > much thought and work has been given to x86 implementation to keep
> >    shadow stack to not impact arch agnostic parts of the kernel. So
> > why creep it in here.
> >
> > - VM_SHADOW_STACK is coming out of the VM_HIGH_ARCH_XX bit position
> > which makes it arch specific.
> >
> >
>
> VM_SHADOW_STACK is defined like this (trimmed for clarity):
> #ifdef CONFIG_X86_USER_SHADOW_STACK
> # define VM_SHADOW_STACK        VM_HIGH_ARCH_5
> #else
> # define VM_SHADOW_STACK        VM_NONE
> #endif

Ok.

>
> Also, we actually had an is_shadow_stack_mapping(vma) in the past, but
> it was dropped from other feedback.

looks like I've been late to the party.
IMHO, that was the right approach.

>
>