| Message ID | 20230218211433.26859-23-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
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() ?
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/
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.
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 --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; }