diff mbox series

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

Message ID 20230218211433.26859-23-rick.p.edgecombe@intel.com
State New
Headers
Series Shadow stacks for userspace |

Commit Message

Edgecombe, Rick P Feb. 18, 2023, 9:14 p.m. UTC 
  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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
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>

---
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/mmap.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

David Hildenbrand Feb. 20, 2023, 12:58 p.m. UTC | #1 
On 18.02.23 22:14, 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.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> 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>
> 
> ---
> 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/mmap.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 425a9349e610..9f85596cce31 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3290,6 +3290,8 @@ void vm_stat_account(struct mm_struct *mm, vm_flags_t flags, long npages)
>   		mm->exec_vm += npages;
>   	else if (is_stack_mapping(flags))
>   		mm->stack_vm += npages;
> +	else if (flags & VM_SHADOW_STACK)
> +		mm->stack_vm += npages;

Why not modify is_stack_mapping() ?
Edgecombe, Rick P Feb. 20, 2023, 10:44 p.m. UTC | #2 
On Mon, 2023-02-20 at 13:58 +0100, David Hildenbrand wrote:
> On 18.02.23 22:14, 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.
> > 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> > Tested-by: John Allen <john.allen@amd.com>
> > 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>
> > 
> > ---
> > 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/mmap.c | 2 ++
> >    1 file changed, 2 insertions(+)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 425a9349e610..9f85596cce31 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -3290,6 +3290,8 @@ void vm_stat_account(struct mm_struct *mm,
> > vm_flags_t flags, long npages)
> >                mm->exec_vm += npages;
> >        else if (is_stack_mapping(flags))
> >                mm->stack_vm += npages;
> > +     else if (flags & VM_SHADOW_STACK)
> > +             mm->stack_vm += npages;
> 
> Why not modify is_stack_mapping() ?

It kind of sticks out a little in this conditional, but
is_stack_mapping() has this comment:
/*
 * Stack area - automatically grows in one direction
 *
 * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
 * do_mmap() forbids all other combinations.
 */

Shadow stack don't grow, so it doesn't quite fit. There used to be an
is_shadow_stack_mapping(), but it was removed because all that was
needed (for the time being) was the simple bitwise AND:

https://lore.kernel.org/lkml/804adbac-61e6-0fd2-f726-5735fb290199@intel.com/
David Hildenbrand Feb. 21, 2023, 8:31 a.m. UTC | #3 
On 20.02.23 23:44, Edgecombe, Rick P wrote:
> On Mon, 2023-02-20 at 13:58 +0100, David Hildenbrand wrote:
>> On 18.02.23 22:14, 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.
>>>
>>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
>>> Tested-by: John Allen <john.allen@amd.com>
>>> 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>
>>>
>>> ---
>>> 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/mmap.c | 2 ++
>>>     1 file changed, 2 insertions(+)
>>>
>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>> index 425a9349e610..9f85596cce31 100644
>>> --- a/mm/mmap.c
>>> +++ b/mm/mmap.c
>>> @@ -3290,6 +3290,8 @@ void vm_stat_account(struct mm_struct *mm,
>>> vm_flags_t flags, long npages)
>>>                 mm->exec_vm += npages;
>>>         else if (is_stack_mapping(flags))
>>>                 mm->stack_vm += npages;
>>> +     else if (flags & VM_SHADOW_STACK)
>>> +             mm->stack_vm += npages;
>>
>> Why not modify is_stack_mapping() ?
> 
> It kind of sticks out a little in this conditional, but
> is_stack_mapping() has this comment:
> /*
>   * Stack area - automatically grows in one direction
>   *
>   * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
>   * do_mmap() forbids all other combinations.
>   */
> 
> Shadow stack don't grow, so it doesn't quite fit. There used to be an
> is_shadow_stack_mapping(), but it was removed because all that was
> needed (for the time being) was the simple bitwise AND:
> 
> https://lore.kernel.org/lkml/804adbac-61e6-0fd2-f726-5735fb290199@intel.com/

As there is only a single user of is_stack_mapping(), I'd simply have 
adjusted the doc of is_stack_mapping() to include shadow stacks.
Edgecombe, Rick P Feb. 22, 2023, 12:06 a.m. UTC | #4 
On Tue, 2023-02-21 at 09:31 +0100, David Hildenbrand wrote:
> > > Why not modify is_stack_mapping() ?
> > 
> > It kind of sticks out a little in this conditional, but
> > is_stack_mapping() has this comment:
> > /*
> >    * Stack area - automatically grows in one direction
> >    *
> >    * VM_GROWSUP / VM_GROWSDOWN VMAs are always private anonymous:
> >    * do_mmap() forbids all other combinations.
> >    */
> > 
> > Shadow stack don't grow, so it doesn't quite fit. There used to be
> > an
> > is_shadow_stack_mapping(), but it was removed because all that was
> > needed (for the time being) was the simple bitwise AND:
> > 
> > 
https://lore.kernel.org/lkml/804adbac-61e6-0fd2-f726-5735fb290199@intel.com/
> 
> As there is only a single user of is_stack_mapping(), I'd simply
> have 
> adjusted the doc of is_stack_mapping() to include shadow stacks.

Ok, I'll update the comment and add it there.
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 425a9349e610..9f85596cce31 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -3290,6 +3290,8 @@  void vm_stat_account(struct mm_struct *mm, vm_flags_t flags, long npages)
 		mm->exec_vm += npages;
 	else if (is_stack_mapping(flags))
 		mm->stack_vm += npages;
+	else if (flags & VM_SHADOW_STACK)
+		mm->stack_vm += npages;
 	else if (is_data_mapping(flags))
 		mm->data_vm += npages;
 }