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 <[email protected]>
---
include/linux/mm.h | 15 ++++++++++++++-
mm/gup.c | 2 +-
mm/internal.h | 2 +-
3 files changed, 16 insertions(+), 3 deletions(-)
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);
}
/*
--
2.43.0
On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <[email protected]> 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"?
On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <[email protected]> wrote:
>
> On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <[email protected]> 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.
>
On Wed, 27 Dec 2023 14:20:36 -0800 Deepak Gupta <[email protected]> wrote:
> On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <[email protected]> wrote:
> >
> > On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <[email protected]> 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.
On Wed, Dec 27, 2023 at 2:24 PM Andrew Morton <[email protected]> wrote:
>
> On Wed, 27 Dec 2023 14:20:36 -0800 Deepak Gupta <[email protected]> wrote:
>
> > On Wed, Dec 27, 2023 at 1:45 PM Andrew Morton <[email protected]> wrote:
> > >
> > > On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <[email protected]> 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
On Wed, Dec 27, 2023 at 01:45:14PM -0800, Andrew Morton wrote:
> On Fri, 22 Dec 2023 15:51:04 -0800 Deepak Gupta <[email protected]> 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.
--
Sincerely yours,
Mike.
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
On Tue, Jan 2, 2024 at 9:50 AM Edgecombe, Rick P
<[email protected]> 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.
On Tue, Jan 2, 2024 at 5:57 AM Mike Rapoport <[email protected]> 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 <[email protected]> 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.