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.
The architecture of shadow stack constrains the ability of userspace to
move the shadow stack pointer (SSP) in order to prevent corrupting or
switching to other shadow stacks. The RSTORSSP instruction can move the
SSP to different shadow stacks, but it requires a specially placed token
in order to do this. However, the architecture does not prevent
incrementing the stack pointer to wander onto an adjacent shadow stack. To
prevent this in software, enforce guard pages at the beginning of shadow
stack VMAs, such that there will always be a gap between adjacent shadow
stacks.
Make the gap big enough so that no userspace SSP changing operations
(besides RSTORSSP), can move the SSP from one stack to the next. The
SSP can be incremented or decremented by CALL, RET and INCSSP. CALL and
RET can move the SSP by a maximum of 8 bytes, at which point the shadow
stack would be accessed.
The INCSSP instruction can also increment the shadow stack pointer. It
is the shadow stack analog of an instruction like:
addq $0x80, %rsp
However, there is one important difference between an ADD on %rsp and
INCSSP. In addition to modifying SSP, INCSSP also reads from the memory
of the first and last elements that were "popped". It can be thought of
as acting like this:
READ_ONCE(ssp); // read+discard top element on stack
ssp += nr_to_pop * 8; // move the shadow stack
READ_ONCE(ssp-8); // read+discard last popped stack element
The maximum distance INCSSP can move the SSP is 2040 bytes, before it
would read the memory. Therefore, a single page gap will be enough to
prevent any operation from shifting the SSP to an adjacent stack, since
it would have to land in the gap at least once, causing a fault.
This could be accomplished by using VM_GROWSDOWN, but this has a
downside. The behavior would allow shadow stacks to grow, which is
unneeded and adds a strange difference to how most regular stacks work.
In the maple tree code, there is some logic for retrying the unmapped
area search if a guard gap is violated. This retry should happen for
shadow stack guard gap violations as well. This logic currently only
checks for VM_GROWSDOWN for start gaps. Since shadow stacks also have
a start gap as well, create an new define VM_STARTGAP_FLAGS to hold
all the VM flag bits that have start gaps, and make mmap use it.
Co-developed-by: Yu-cheng Yu <[email protected]>
Signed-off-by: Yu-cheng Yu <[email protected]>
Signed-off-by: Rick Edgecombe <[email protected]>
Reviewed-by: Borislav Petkov (AMD) <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Acked-by: Mike Rapoport (IBM) <[email protected]>
Tested-by: Pengfei Xu <[email protected]>
Tested-by: John Allen <[email protected]>
Tested-by: Kees Cook <[email protected]>
---
v9:
- Add logic needed to still have guard gaps with maple tree.
---
include/linux/mm.h | 54 ++++++++++++++++++++++++++++++++++++++++------
mm/mmap.c | 4 ++--
2 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index fb17cbd531ac..535c58d3b2e4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -342,7 +342,36 @@ extern unsigned int kobjsize(const void *objp);
#endif /* CONFIG_ARCH_HAS_PKEYS */
#ifdef CONFIG_X86_USER_SHADOW_STACK
-# define VM_SHADOW_STACK VM_HIGH_ARCH_5 /* Should not be set with VM_SHARED */
+/*
+ * This flag should not be set with VM_SHARED because of lack of support
+ * core mm. It will also get a guard page. This helps userspace protect
+ * itself from attacks. The reasoning is as follows:
+ *
+ * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
+ * INCSSP instruction can increment the shadow stack pointer. It is the
+ * shadow stack analog of an instruction like:
+ *
+ * addq $0x80, %rsp
+ *
+ * However, there is one important difference between an ADD on %rsp
+ * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
+ * memory of the first and last elements that were "popped". It can be
+ * thought of as acting like this:
+ *
+ * READ_ONCE(ssp); // read+discard top element on stack
+ * ssp += nr_to_pop * 8; // move the shadow stack
+ * READ_ONCE(ssp-8); // read+discard last popped stack element
+ *
+ * The maximum distance INCSSP can move the SSP is 2040 bytes, before
+ * it would read the memory. Therefore a single page gap will be enough
+ * to prevent any operation from shifting the SSP to an adjacent stack,
+ * since it would have to land in the gap at least once, causing a
+ * fault.
+ *
+ * Prevent using INCSSP to move the SSP between shadow stacks by
+ * having a PAGE_SIZE guard gap.
+ */
+# define VM_SHADOW_STACK VM_HIGH_ARCH_5
#else
# define VM_SHADOW_STACK VM_NONE
#endif
@@ -405,6 +434,8 @@ extern unsigned int kobjsize(const void *objp);
#define VM_STACK_DEFAULT_FLAGS VM_DATA_DEFAULT_FLAGS
#endif
+#define VM_STARTGAP_FLAGS (VM_GROWSDOWN | VM_SHADOW_STACK)
+
#ifdef CONFIG_STACK_GROWSUP
#define VM_STACK VM_GROWSUP
#else
@@ -3235,15 +3266,26 @@ struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
return mtree_load(&mm->mm_mt, addr);
}
+static inline unsigned long stack_guard_start_gap(struct vm_area_struct *vma)
+{
+ if (vma->vm_flags & VM_GROWSDOWN)
+ return stack_guard_gap;
+
+ /* See reasoning around the VM_SHADOW_STACK definition */
+ if (vma->vm_flags & VM_SHADOW_STACK)
+ return PAGE_SIZE;
+
+ return 0;
+}
+
static inline unsigned long vm_start_gap(struct vm_area_struct *vma)
{
+ unsigned long gap = stack_guard_start_gap(vma);
unsigned long vm_start = vma->vm_start;
- if (vma->vm_flags & VM_GROWSDOWN) {
- vm_start -= stack_guard_gap;
- if (vm_start > vma->vm_start)
- vm_start = 0;
- }
+ vm_start -= gap;
+ if (vm_start > vma->vm_start)
+ vm_start = 0;
return vm_start;
}
diff --git a/mm/mmap.c b/mm/mmap.c
index afdf5f78432b..d4793600a8d4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1570,7 +1570,7 @@ static unsigned long unmapped_area(struct vm_unmapped_area_info *info)
gap = mas.index;
gap += (info->align_offset - gap) & info->align_mask;
tmp = mas_next(&mas, ULONG_MAX);
- if (tmp && (tmp->vm_flags & VM_GROWSDOWN)) { /* Avoid prev check if possible */
+ if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */
if (vm_start_gap(tmp) < gap + length - 1) {
low_limit = tmp->vm_end;
mas_reset(&mas);
@@ -1622,7 +1622,7 @@ static unsigned long unmapped_area_topdown(struct vm_unmapped_area_info *info)
gap -= (gap - info->align_offset) & info->align_mask;
gap_end = mas.last;
tmp = mas_next(&mas, ULONG_MAX);
- if (tmp && (tmp->vm_flags & VM_GROWSDOWN)) { /* Avoid prev check if possible */
+ if (tmp && (tmp->vm_flags & VM_STARTGAP_FLAGS)) { /* Avoid prev check if possible */
if (vm_start_gap(tmp) <= gap_end) {
high_limit = vm_start_gap(tmp);
mas_reset(&mas);
--
2.34.1
On Mon, Jun 12, 2023 at 05:10:42PM -0700, Rick Edgecombe wrote:
> 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.
Reviewed-by: Mark Brown <[email protected]>
On Mon, Jun 12, 2023 at 05:10:42PM -0700, Rick Edgecombe wrote:
> +++ b/include/linux/mm.h
> @@ -342,7 +342,36 @@ extern unsigned int kobjsize(const void *objp);
> #endif /* CONFIG_ARCH_HAS_PKEYS */
>
> #ifdef CONFIG_X86_USER_SHADOW_STACK
> -# define VM_SHADOW_STACK VM_HIGH_ARCH_5 /* Should not be set with VM_SHARED */
> +/*
> + * This flag should not be set with VM_SHARED because of lack of support
> + * core mm. It will also get a guard page. This helps userspace protect
> + * itself from attacks. The reasoning is as follows:
> + *
> + * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
> + * INCSSP instruction can increment the shadow stack pointer. It is the
> + * shadow stack analog of an instruction like:
> + *
> + * addq $0x80, %rsp
> + *
> + * However, there is one important difference between an ADD on %rsp
> + * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
> + * memory of the first and last elements that were "popped". It can be
> + * thought of as acting like this:
> + *
> + * READ_ONCE(ssp); // read+discard top element on stack
> + * ssp += nr_to_pop * 8; // move the shadow stack
> + * READ_ONCE(ssp-8); // read+discard last popped stack element
> + *
> + * The maximum distance INCSSP can move the SSP is 2040 bytes, before
> + * it would read the memory. Therefore a single page gap will be enough
> + * to prevent any operation from shifting the SSP to an adjacent stack,
> + * since it would have to land in the gap at least once, causing a
> + * fault.
> + *
> + * Prevent using INCSSP to move the SSP between shadow stacks by
> + * having a PAGE_SIZE guard gap.
> + */
> +# define VM_SHADOW_STACK VM_HIGH_ARCH_5
> #else
> # define VM_SHADOW_STACK VM_NONE
> #endif
This is a lot of very x86-specific language in a generic header file.
I'm sure there's a better place for all this text.
On Thu, 2023-06-22 at 19:21 +0100, Matthew Wilcox wrote:
> On Mon, Jun 12, 2023 at 05:10:42PM -0700, Rick Edgecombe wrote:
> > +++ b/include/linux/mm.h
> > @@ -342,7 +342,36 @@ extern unsigned int kobjsize(const void
> > *objp);
> > #endif /* CONFIG_ARCH_HAS_PKEYS */
> >
> > #ifdef CONFIG_X86_USER_SHADOW_STACK
> > -# define VM_SHADOW_STACK VM_HIGH_ARCH_5 /* Should not be set
> > with VM_SHARED */
> > +/*
> > + * This flag should not be set with VM_SHARED because of lack of
> > support
> > + * core mm. It will also get a guard page. This helps userspace
> > protect
> > + * itself from attacks. The reasoning is as follows:
> > + *
> > + * The shadow stack pointer(SSP) is moved by CALL, RET, and
> > INCSSPQ. The
> > + * INCSSP instruction can increment the shadow stack pointer. It
> > is the
> > + * shadow stack analog of an instruction like:
> > + *
> > + * addq $0x80, %rsp
> > + *
> > + * However, there is one important difference between an ADD on
> > %rsp
> > + * and INCSSP. In addition to modifying SSP, INCSSP also reads
> > from the
> > + * memory of the first and last elements that were "popped". It
> > can be
> > + * thought of as acting like this:
> > + *
> > + * READ_ONCE(ssp); // read+discard top element on stack
> > + * ssp += nr_to_pop * 8; // move the shadow stack
> > + * READ_ONCE(ssp-8); // read+discard last popped stack element
> > + *
> > + * The maximum distance INCSSP can move the SSP is 2040 bytes,
> > before
> > + * it would read the memory. Therefore a single page gap will be
> > enough
> > + * to prevent any operation from shifting the SSP to an adjacent
> > stack,
> > + * since it would have to land in the gap at least once, causing a
> > + * fault.
> > + *
> > + * Prevent using INCSSP to move the SSP between shadow stacks by
> > + * having a PAGE_SIZE guard gap.
> > + */
> > +# define VM_SHADOW_STACK VM_HIGH_ARCH_5
> > #else
> > # define VM_SHADOW_STACK VM_NONE
> > #endif
>
> This is a lot of very x86-specific language in a generic header file.
> I'm sure there's a better place for all this text.
Yes, I couldn't find another place for it. This was the reasoning:
https://lore.kernel.org/lkml/[email protected]/
Did you have any particular place in mind?
On Thu, Jun 22, 2023 at 06:27:40PM +0000, Edgecombe, Rick P wrote:
> On Thu, 2023-06-22 at 19:21 +0100, Matthew Wilcox wrote:
> > On Mon, Jun 12, 2023 at 05:10:42PM -0700, Rick Edgecombe wrote:
> > > +++ b/include/linux/mm.h
> > > @@ -342,7 +342,36 @@ extern unsigned int kobjsize(const void
> > > *objp);
> > > ? #endif /* CONFIG_ARCH_HAS_PKEYS */
> > > ?
> > > ? #ifdef CONFIG_X86_USER_SHADOW_STACK
> > > -# define VM_SHADOW_STACK???????VM_HIGH_ARCH_5 /* Should not be set
> > > with VM_SHARED */
> > > +/*
> > > + * This flag should not be set with VM_SHARED because of lack of
> > > support
> > > + * core mm. It will also get a guard page. This helps userspace
> > > protect
> > > + * itself from attacks. The reasoning is as follows:
> > > + *
> > > + * The shadow stack pointer(SSP) is moved by CALL, RET, and
> > > INCSSPQ. The
> > > + * INCSSP instruction can increment the shadow stack pointer. It
> > > is the
> > > + * shadow stack analog of an instruction like:
> > > + *
> > > + *?? addq $0x80, %rsp
> > > + *
> > > + * However, there is one important difference between an ADD on
> > > %rsp
> > > + * and INCSSP. In addition to modifying SSP, INCSSP also reads
> > > from the
> > > + * memory of the first and last elements that were "popped". It
> > > can be
> > > + * thought of as acting like this:
> > > + *
> > > + * READ_ONCE(ssp);?????? // read+discard top element on stack
> > > + * ssp += nr_to_pop * 8; // move the shadow stack
> > > + * READ_ONCE(ssp-8);???? // read+discard last popped stack element
> > > + *
> > > + * The maximum distance INCSSP can move the SSP is 2040 bytes,
> > > before
> > > + * it would read the memory. Therefore a single page gap will be
> > > enough
> > > + * to prevent any operation from shifting the SSP to an adjacent
> > > stack,
> > > + * since it would have to land in the gap at least once, causing a
> > > + * fault.
> > > + *
> > > + * Prevent using INCSSP to move the SSP between shadow stacks by
> > > + * having a PAGE_SIZE guard gap.
> > > + */
> > > +# define VM_SHADOW_STACK???????VM_HIGH_ARCH_5
> > > ? #else
> > > ? # define VM_SHADOW_STACK??????VM_NONE
> > > ? #endif
> >
> > This is a lot of very x86-specific language in a generic header file.
> > I'm sure there's a better place for all this text.
>
> Yes, I couldn't find another place for it. This was the reasoning:
> https://lore.kernel.org/lkml/[email protected]/
>
> Did you have any particular place in mind?
Since it's near CONFIG_X86_USER_SHADOW_STACK the comment in mm.h could be
/*
* VMA is used for shadow stack and implies guard pages.
* See arch/x86/kernel/shstk.c for details
*/
and the long reasoning comment can be moved near alloc_shstk in
arch/x86/kernel/shstk.h
--
Sincerely yours,
Mike.
On Fri, Jun 23, 2023 at 10:40:00AM +0300, Mike Rapoport wrote:
> On Thu, Jun 22, 2023 at 06:27:40PM +0000, Edgecombe, Rick P wrote:
> > Yes, I couldn't find another place for it. This was the reasoning:
> > https://lore.kernel.org/lkml/[email protected]/
> > Did you have any particular place in mind?
> Since it's near CONFIG_X86_USER_SHADOW_STACK the comment in mm.h could be
> /*
> * VMA is used for shadow stack and implies guard pages.
> * See arch/x86/kernel/shstk.c for details
> */
> and the long reasoning comment can be moved near alloc_shstk in
> arch/x86/kernel/shstk.h
This isn't an x86 specific concept, arm64 has a very similar extension
called Guarded Control Stack (which I should be publishing changes for
in the not too distant future) and riscv also has something. For arm64
I'm using the generic mm changes wholesale, we have a similar need for
guard pages around the GCS and while the mechanics of accessing are
different the requirement ends up being the same. Perhaps we could just
rewrite the comment to say that guard pages prevent over/underflow of
the stack by userspace and that a single page is sufficient for all
current architectures, with the details of the working for x86 put in
some x86 specific place?
On Fri, 2023-06-23 at 13:17 +0100, Mark Brown wrote:
> On Fri, Jun 23, 2023 at 10:40:00AM +0300, Mike Rapoport wrote:
> > On Thu, Jun 22, 2023 at 06:27:40PM +0000, Edgecombe, Rick P wrote:
>
> > > Yes, I couldn't find another place for it. This was the
> > > reasoning:
> > >
> https://lore.kernel.org/lkml/[email protected]/
>
> > > Did you have any particular place in mind?
>
> > Since it's near CONFIG_X86_USER_SHADOW_STACK the comment in mm.h
> could be
>
> > /*
> > * VMA is used for shadow stack and implies guard pages.
> > * See arch/x86/kernel/shstk.c for details
> > */
>
> > and the long reasoning comment can be moved near alloc_shstk in
> > arch/x86/kernel/shstk.h
Makes sense. Not sure why I didn't think of this earlier.
>
> This isn't an x86 specific concept, arm64 has a very similar
> extension
> called Guarded Control Stack (which I should be publishing changes
> for
> in the not too distant future) and riscv also has something. For
> arm64
> I'm using the generic mm changes wholesale, we have a similar need
> for
> guard pages around the GCS and while the mechanics of accessing are
> different the requirement ends up being the same. Perhaps we could
> just
> rewrite the comment to say that guard pages prevent over/underflow of
> the stack by userspace and that a single page is sufficient for all
> current architectures, with the details of the working for x86 put in
> some x86 specific place?
Something sort of similar came up in regards to the riscv series, about
adding something like an is_shadow_stack_vma() helper. The plan was to
not make too many assumptions about the final details of the other
shadow stack features and leave that for refactoring. I think some kind
of generic comment like you suggest makes sense, but I don't want to
try to assert any arch specifics for features that are not upstream. It
should be very easy to tweak the comment when the time comes.
The points about x86 details not belonging in non-arch headers and
having some arch generic explanation in the file are well taken though.
On Sun, Jun 25, 2023 at 04:44:32PM +0000, Edgecombe, Rick P wrote:
> On Fri, 2023-06-23 at 13:17 +0100, Mark Brown wrote:
> > This isn't an x86 specific concept, arm64 has a very similar
> > extension
> > called Guarded Control Stack (which I should be publishing changes
> > for
> > in the not too distant future) and riscv also has something.? For
> > arm64
> > I'm using the generic mm changes wholesale, we have a similar need
> > for
> > guard pages around the GCS and while the mechanics of accessing are
> > different the requirement ends up being the same.? Perhaps we could
> > just
> > rewrite the comment to say that guard pages prevent over/underflow of
> > the stack by userspace and that a single page is sufficient for all
> > current architectures, with the details of the working for x86 put in
> > some x86 specific place?
> Something sort of similar came up in regards to the riscv series, about
> adding something like an is_shadow_stack_vma() helper. The plan was to
> not make too many assumptions about the final details of the other
> shadow stack features and leave that for refactoring. I think some kind
> of generic comment like you suggest makes sense, but I don't want to
> try to assert any arch specifics for features that are not upstream. It
> should be very easy to tweak the comment when the time comes.
> The points about x86 details not belonging in non-arch headers and
> having some arch generic explanation in the file are well taken though.
I think a statement to the effect that "this works for currently
supported architectures" is fine, if something comes along with
additional requirements then the comment can be adjusted as part of
merging the new thing.
The comment around VM_SHADOW_STACK in mm.h refers to a lot of x86
specific details that don't belong in a cross arch file. Remove these
out of core mm, and just leave the non-arch details.
Since the comment includes some useful details that would be good to
retain in the source somewhere, put the arch specifics parts in
arch/x86/shstk.c near alloc_shstk(), where memory of this type is
allocated. Include a reference to the existence of the x86 details near
the VM_SHADOW_STACK definition mm.h.
Signed-off-by: Rick Edgecombe <[email protected]>
---
arch/x86/kernel/shstk.c | 25 +++++++++++++++++++++++++
include/linux/mm.h | 32 ++++++--------------------------
2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index b26810c7cd1c..47f5204b0fa9 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -72,6 +72,31 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
return 0;
}
+/*
+ * VM_SHADOW_STACK will have a guard page. This helps userspace protect
+ * itself from attacks. The reasoning is as follows:
+ *
+ * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
+ * INCSSP instruction can increment the shadow stack pointer. It is the
+ * shadow stack analog of an instruction like:
+ *
+ * addq $0x80, %rsp
+ *
+ * However, there is one important difference between an ADD on %rsp
+ * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
+ * memory of the first and last elements that were "popped". It can be
+ * thought of as acting like this:
+ *
+ * READ_ONCE(ssp); // read+discard top element on stack
+ * ssp += nr_to_pop * 8; // move the shadow stack
+ * READ_ONCE(ssp-8); // read+discard last popped stack element
+ *
+ * The maximum distance INCSSP can move the SSP is 2040 bytes, before
+ * it would read the memory. Therefore a single page gap will be enough
+ * to prevent any operation from shifting the SSP to an adjacent stack,
+ * since it would have to land in the gap at least once, causing a
+ * fault.
+ */
static unsigned long alloc_shstk(unsigned long addr, unsigned long size,
unsigned long token_offset, bool set_res_tok)
{
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 535c58d3b2e4..b647cf2e94ea 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -343,33 +343,13 @@ extern unsigned int kobjsize(const void *objp);
#ifdef CONFIG_X86_USER_SHADOW_STACK
/*
- * This flag should not be set with VM_SHARED because of lack of support
- * core mm. It will also get a guard page. This helps userspace protect
- * itself from attacks. The reasoning is as follows:
+ * VM_SHADOW_STACK should not be set with VM_SHARED because of lack of
+ * support core mm.
*
- * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
- * INCSSP instruction can increment the shadow stack pointer. It is the
- * shadow stack analog of an instruction like:
- *
- * addq $0x80, %rsp
- *
- * However, there is one important difference between an ADD on %rsp
- * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
- * memory of the first and last elements that were "popped". It can be
- * thought of as acting like this:
- *
- * READ_ONCE(ssp); // read+discard top element on stack
- * ssp += nr_to_pop * 8; // move the shadow stack
- * READ_ONCE(ssp-8); // read+discard last popped stack element
- *
- * The maximum distance INCSSP can move the SSP is 2040 bytes, before
- * it would read the memory. Therefore a single page gap will be enough
- * to prevent any operation from shifting the SSP to an adjacent stack,
- * since it would have to land in the gap at least once, causing a
- * fault.
- *
- * Prevent using INCSSP to move the SSP between shadow stacks by
- * having a PAGE_SIZE guard gap.
+ * These VMAs will get a single end guard page. This helps userspace protect
+ * itself from attacks. A single page is enough for current shadow stack archs
+ * (x86). See the comments near alloc_shstk() in arch/x86/kernel/shstk.c
+ * for more details on the guard size.
*/
# define VM_SHADOW_STACK VM_HIGH_ARCH_5
#else
--
2.34.1
On Thu, Jul 06, 2023 at 04:32:48PM -0700, Rick Edgecombe wrote:
> The comment around VM_SHADOW_STACK in mm.h refers to a lot of x86
> specific details that don't belong in a cross arch file. Remove these
> out of core mm, and just leave the non-arch details.
>
> Since the comment includes some useful details that would be good to
> retain in the source somewhere, put the arch specifics parts in
> arch/x86/shstk.c near alloc_shstk(), where memory of this type is
> allocated. Include a reference to the existence of the x86 details near
> the VM_SHADOW_STACK definition mm.h.
Reviewed-by: Mark Brown <[email protected]>
Hi Dave, Rick,
It seems it didn't get into the current tip.
On Thu, Jul 06, 2023 at 04:32:48PM -0700, Rick Edgecombe wrote:
> The comment around VM_SHADOW_STACK in mm.h refers to a lot of x86
> specific details that don't belong in a cross arch file. Remove these
> out of core mm, and just leave the non-arch details.
>
> Since the comment includes some useful details that would be good to
> retain in the source somewhere, put the arch specifics parts in
> arch/x86/shstk.c near alloc_shstk(), where memory of this type is
> allocated. Include a reference to the existence of the x86 details near
> the VM_SHADOW_STACK definition mm.h.
>
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> arch/x86/kernel/shstk.c | 25 +++++++++++++++++++++++++
> include/linux/mm.h | 32 ++++++--------------------------
> 2 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index b26810c7cd1c..47f5204b0fa9 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -72,6 +72,31 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr)
> return 0;
> }
>
> +/*
> + * VM_SHADOW_STACK will have a guard page. This helps userspace protect
> + * itself from attacks. The reasoning is as follows:
> + *
> + * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
> + * INCSSP instruction can increment the shadow stack pointer. It is the
> + * shadow stack analog of an instruction like:
> + *
> + * addq $0x80, %rsp
> + *
> + * However, there is one important difference between an ADD on %rsp
> + * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
> + * memory of the first and last elements that were "popped". It can be
> + * thought of as acting like this:
> + *
> + * READ_ONCE(ssp); // read+discard top element on stack
> + * ssp += nr_to_pop * 8; // move the shadow stack
> + * READ_ONCE(ssp-8); // read+discard last popped stack element
> + *
> + * The maximum distance INCSSP can move the SSP is 2040 bytes, before
> + * it would read the memory. Therefore a single page gap will be enough
> + * to prevent any operation from shifting the SSP to an adjacent stack,
> + * since it would have to land in the gap at least once, causing a
> + * fault.
> + */
> static unsigned long alloc_shstk(unsigned long addr, unsigned long size,
> unsigned long token_offset, bool set_res_tok)
> {
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 535c58d3b2e4..b647cf2e94ea 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -343,33 +343,13 @@ extern unsigned int kobjsize(const void *objp);
>
> #ifdef CONFIG_X86_USER_SHADOW_STACK
> /*
> - * This flag should not be set with VM_SHARED because of lack of support
> - * core mm. It will also get a guard page. This helps userspace protect
> - * itself from attacks. The reasoning is as follows:
> + * VM_SHADOW_STACK should not be set with VM_SHARED because of lack of
> + * support core mm.
> *
> - * The shadow stack pointer(SSP) is moved by CALL, RET, and INCSSPQ. The
> - * INCSSP instruction can increment the shadow stack pointer. It is the
> - * shadow stack analog of an instruction like:
> - *
> - * addq $0x80, %rsp
> - *
> - * However, there is one important difference between an ADD on %rsp
> - * and INCSSP. In addition to modifying SSP, INCSSP also reads from the
> - * memory of the first and last elements that were "popped". It can be
> - * thought of as acting like this:
> - *
> - * READ_ONCE(ssp); // read+discard top element on stack
> - * ssp += nr_to_pop * 8; // move the shadow stack
> - * READ_ONCE(ssp-8); // read+discard last popped stack element
> - *
> - * The maximum distance INCSSP can move the SSP is 2040 bytes, before
> - * it would read the memory. Therefore a single page gap will be enough
> - * to prevent any operation from shifting the SSP to an adjacent stack,
> - * since it would have to land in the gap at least once, causing a
> - * fault.
> - *
> - * Prevent using INCSSP to move the SSP between shadow stacks by
> - * having a PAGE_SIZE guard gap.
> + * These VMAs will get a single end guard page. This helps userspace protect
> + * itself from attacks. A single page is enough for current shadow stack archs
> + * (x86). See the comments near alloc_shstk() in arch/x86/kernel/shstk.c
> + * for more details on the guard size.
> */
> # define VM_SHADOW_STACK VM_HIGH_ARCH_5
> #else
> --
> 2.34.1
>
--
Sincerely yours,
Mike.