diff mbox series

[v1] mm: abstract shadow stack vma behind arch_is_shadow_stack_vma

Message ID 20231222235248.576482-1-debug@rivosinc.com
State New
Headers
Series [v1] mm: abstract shadow stack vma behind arch_is_shadow_stack_vma |

Commit Message

Deepak Gupta Dec. 22, 2023, 11:51 p.m. UTC 
  x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
need a way to encode shadow stack on 32bit and 64bit both and they may
encode this information differently in VMAs.

This patch changes checks of VM_SHADOW_STACK flag in generic code to call
to a function `arch_is_shadow_stack_vma` which will return true if arch
supports shadow stack and vma is shadow stack else stub returns false.

Signed-off-by: Deepak Gupta <debug@rivosinc.com>
---
 include/linux/mm.h | 15 ++++++++++++++-
 mm/gup.c           |  2 +-
 mm/internal.h      |  2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

Comments

Andrew Morton Dec. 27, 2023, 9:45 p.m. UTC | #1 
On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:

> x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> need a way to encode shadow stack on 32bit and 64bit both and they may
> encode this information differently in VMAs.

Is such a patch in the pipeline?  Otherwise we're making a change that
serves no purpose.

> This patch changes checks of VM_SHADOW_STACK flag in generic code to call
> to a function `arch_is_shadow_stack_vma` which will return true if arch
> supports shadow stack and vma is shadow stack else stub returns false.
> 
> ...
>
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -352,8 +352,21 @@ extern unsigned int kobjsize(const void *objp);
>   * for more details on the guard size.
>   */
>  # define VM_SHADOW_STACK	VM_HIGH_ARCH_5
> +
> +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> +{
> +	return (vm_flags & VM_SHADOW_STACK) ? true : false;
> +}

The naming seems a little wrong.  I'd expect it to take a vma* arg. 
Maybe just drop the "_vma"?
Deepak Gupta Dec. 27, 2023, 10:20 p.m. UTC | #2 
On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
>
> > x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> > stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> > need a way to encode shadow stack on 32bit and 64bit both and they may
> > encode this information differently in VMAs.
>
> Is such a patch in the pipeline?  Otherwise we're making a change that
> serves no purpose.

Yes I do have patches in the pipeline for riscv.
On riscv, presence of only `VM_WRITE` (i.e. (flags & (VM_READ |
VM_WRITE | VM_EXEC))
== VM_WRITE) would mean a shadow stack.
And yes there would be  relevant patches to ensure that existing consumers using
`PROT_WRITE` gets translated to (VM_WRITE | VM_READ)

>
> > This patch changes checks of VM_SHADOW_STACK flag in generic code to call
> > to a function `arch_is_shadow_stack_vma` which will return true if arch
> > supports shadow stack and vma is shadow stack else stub returns false.
> >
> > ...
> >
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -352,8 +352,21 @@ extern unsigned int kobjsize(const void *objp);
> >   * for more details on the guard size.
> >   */
> >  # define VM_SHADOW_STACK     VM_HIGH_ARCH_5
> > +
> > +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> > +{
> > +     return (vm_flags & VM_SHADOW_STACK) ? true : false;
> > +}
>
> The naming seems a little wrong.  I'd expect it to take a vma* arg.
> Maybe just drop the "_vma"?

Well I did start with taking vma* argument but then realized that
`is_stack_mapping`
only takes vma flags. And in order to change that I would have to
change `vm_stat_account`
and every place it's called.

In the next version I'll either do that or drop `_vma` from the
proposed function name.

>
Andrew Morton Dec. 27, 2023, 10:24 p.m. UTC | #3 
On Wed, 27 Dec 2023 14:20:36 -0800 Deepak Gupta <debug@rivosinc.com> wrote:

> On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > > x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> > > stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> > > need a way to encode shadow stack on 32bit and 64bit both and they may
> > > encode this information differently in VMAs.
> >
> > Is such a patch in the pipeline?  Otherwise we're making a change that
> > serves no purpose.
> 
> Yes I do have patches in the pipeline for riscv.
> On riscv, presence of only `VM_WRITE` (i.e. (flags & (VM_READ |
> VM_WRITE | VM_EXEC))
> == VM_WRITE) would mean a shadow stack.
> And yes there would be  relevant patches to ensure that existing consumers using
> `PROT_WRITE` gets translated to (VM_WRITE | VM_READ)

OK, please plan to carry this patch in whatever tree contains the above.
Deepak Gupta Dec. 30, 2023, 2:30 a.m. UTC | #4 
On Wed, Dec 27, 2023 at 2:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 27 Dec 2023 14:20:36 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
>
> > On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
> > >
> > > > x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> > > > stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> > > > need a way to encode shadow stack on 32bit and 64bit both and they may
> > > > encode this information differently in VMAs.
> > >
> > > Is such a patch in the pipeline?  Otherwise we're making a change that
> > > serves no purpose.
> >
> > Yes I do have patches in the pipeline for riscv.
> > On riscv, presence of only `VM_WRITE` (i.e. (flags & (VM_READ |
> > VM_WRITE | VM_EXEC))
> > == VM_WRITE) would mean a shadow stack.
> > And yes there would be  relevant patches to ensure that existing consumers using
> > `PROT_WRITE` gets translated to (VM_WRITE | VM_READ)
>
> OK, please plan to carry this patch in whatever tree contains the above.
>
>
ACK. Thanks
Mike Rapoport Jan. 2, 2024, 1:56 p.m. UTC | #5 
On Wed, Dec 27, 2023 at 01:45:14PM -0800, Andrew Morton wrote:
> On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
> 
> > x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> > stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> > need a way to encode shadow stack on 32bit and 64bit both and they may
> > encode this information differently in VMAs.
> 
> Is such a patch in the pipeline?  Otherwise we're making a change that
> serves no purpose.
> 
> > This patch changes checks of VM_SHADOW_STACK flag in generic code to call
> > to a function `arch_is_shadow_stack_vma` which will return true if arch
> > supports shadow stack and vma is shadow stack else stub returns false.
> > 
> > ...
> >
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -352,8 +352,21 @@ extern unsigned int kobjsize(const void *objp);
> >   * for more details on the guard size.
> >   */
> >  # define VM_SHADOW_STACK	VM_HIGH_ARCH_5
> > +
> > +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> > +{
> > +	return (vm_flags & VM_SHADOW_STACK) ? true : false;
> > +}
> 
> The naming seems a little wrong.  I'd expect it to take a vma* arg. 
> Maybe just drop the "_vma"?

I'd suggest to use vma_is_shadow_stack() to make it inline with other
vma_is_*() tests.
Edgecombe, Rick P Jan. 2, 2024, 5:50 p.m. UTC | #6 
On Fri, 2023-12-22 at 15:51 -0800, Deepak Gupta wrote:
> +
> +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> +{
> +       return (vm_flags & VM_SHADOW_STACK) ? true : false;
> +}
> +

The bit after the "?" should be unneeded. I would think that patterns
like:

   bool res = val ? true : false;

...should never be needed for the kernel's current bool typedef. Is
there some special arch define consideration or something, I'm unaware
of?

https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool
Deepak Gupta Jan. 2, 2024, 6:45 p.m. UTC | #7 
On Tue, Jan 2, 2024 at 9:50 AM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Fri, 2023-12-22 at 15:51 -0800, Deepak Gupta wrote:
> > +
> > +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> > +{
> > +       return (vm_flags & VM_SHADOW_STACK) ? true : false;
> > +}
> > +
>
> The bit after the "?" should be unneeded. I would think that patterns
> like:
>
>    bool res = val ? true : false;
>
> ...should never be needed for the kernel's current bool typedef. Is
> there some special arch define consideration or something, I'm unaware
> of?
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html#using-bool

Thanks. Just checked out the link you sent.
Yes it's not needed. Will remove it.
Deepak Gupta Jan. 2, 2024, 6:45 p.m. UTC | #8 
On Tue, Jan 2, 2024 at 5:57 AM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Wed, Dec 27, 2023 at 01:45:14PM -0800, Andrew Morton wrote:
> > On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <debug@rivosinc.com> wrote:
> >
> > > x86 has used VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow
> > > stack VMA. VM_SHADOW_STACK is thus not possible on 32bit. Some arches may
> > > need a way to encode shadow stack on 32bit and 64bit both and they may
> > > encode this information differently in VMAs.
> >
> > Is such a patch in the pipeline?  Otherwise we're making a change that
> > serves no purpose.
> >
> > > This patch changes checks of VM_SHADOW_STACK flag in generic code to call
> > > to a function `arch_is_shadow_stack_vma` which will return true if arch
> > > supports shadow stack and vma is shadow stack else stub returns false.
> > >
> > > ...
> > >
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -352,8 +352,21 @@ extern unsigned int kobjsize(const void *objp);
> > >   * for more details on the guard size.
> > >   */
> > >  # define VM_SHADOW_STACK   VM_HIGH_ARCH_5
> > > +
> > > +static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
> > > +{
> > > +   return (vm_flags & VM_SHADOW_STACK) ? true : false;
> > > +}
> >
> > The naming seems a little wrong.  I'd expect it to take a vma* arg.
> > Maybe just drop the "_vma"?
>
> I'd suggest to use vma_is_shadow_stack() to make it inline with other
> vma_is_*() tests.

Noted. Thanks

>
> --
> Sincerely yours,
> Mike.
diff mbox series

Patch

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 418d26608ece..9586e7bbd2aa 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -352,8 +352,21 @@  extern unsigned int kobjsize(const void *objp);
  * for more details on the guard size.
  */
 # define VM_SHADOW_STACK	VM_HIGH_ARCH_5
+
+static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
+{
+	return (vm_flags & VM_SHADOW_STACK) ? true : false;
+}
+
 #else
+
 # define VM_SHADOW_STACK	VM_NONE
+
+static inline bool arch_is_shadow_stack_vma(vm_flags_t vm_flags)
+{
+	return false;
+}
+
 #endif
 
 #if defined(CONFIG_X86)
@@ -3452,7 +3465,7 @@  static inline unsigned long stack_guard_start_gap(struct vm_area_struct *vma)
 		return stack_guard_gap;
 
 	/* See reasoning around the VM_SHADOW_STACK definition */
-	if (vma->vm_flags & VM_SHADOW_STACK)
+	if (arch_is_shadow_stack_vma(vma->vm_flags))
 		return PAGE_SIZE;
 
 	return 0;
diff --git a/mm/gup.c b/mm/gup.c
index 231711efa390..dcc2aa079163 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1051,7 +1051,7 @@  static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
 		    !writable_file_mapping_allowed(vma, gup_flags))
 			return -EFAULT;
 
-		if (!(vm_flags & VM_WRITE) || (vm_flags & VM_SHADOW_STACK)) {
+		if (!(vm_flags & VM_WRITE) || arch_is_shadow_stack_vma(vm_flags)) {
 			if (!(gup_flags & FOLL_FORCE))
 				return -EFAULT;
 			/* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */
diff --git a/mm/internal.h b/mm/internal.h
index b61034bd50f5..05a6b47c3ca1 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -572,7 +572,7 @@  static inline bool is_exec_mapping(vm_flags_t flags)
  */
 static inline bool is_stack_mapping(vm_flags_t flags)
 {
-	return ((flags & VM_STACK) == VM_STACK) || (flags & VM_SHADOW_STACK);
+	return ((flags & VM_STACK) == VM_STACK) || arch_is_shadow_stack_vma(flags);
 }
 
 /*