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

From: David Hildenbrand
Date: Tue Feb 21 2023 - 03:32:09 EST


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@xxxxxxxxx>

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@xxxxxxxxxxxx>
Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
Tested-by: John Allen <john.allen@xxxxxxx>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
Cc: Kees Cook <keescook@xxxxxxxxxxxx>

---
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@xxxxxxxxx/

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.

--
Thanks,

David / dhildenb