| Message ID | 20240125062739.1339782-10-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | riscv control-flow integrity for usermode | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On 25.01.24 07:21, debug@rivosinc.com wrote: > From: Deepak Gupta <debug@rivosinc.com> > > 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` which will return true if arch > supports shadow stack and vma is shadow stack else stub returns false. > > There was a suggestion to name it as `vma_is_shadow_stack`. I preferred to > keep `arch` prefix in there because it's each arch specific. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > --- > include/linux/mm.h | 18 +++++++++++++++++- > mm/gup.c | 5 +++-- > mm/internal.h | 2 +- > 3 files changed, 21 insertions(+), 4 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index dfe0e8118669..15c70fc677a3 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -352,6 +352,10 @@ 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(vm_flags_t vm_flags) > +{ > + return (vm_flags & VM_SHADOW_STACK); > +} > #endif > > #ifdef CONFIG_RISCV_USER_CFI > @@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp); > * with VM_SHARED. > */ > #define VM_SHADOW_STACK VM_WRITE > + > +static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) > +{ > + return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE); > +} > + Please no such hacks just to work around the 32bit vmflags limitation.
On Thu, Jan 25, 2024 at 09:18:07AM +0100, David Hildenbrand wrote: >On 25.01.24 07:21, debug@rivosinc.com wrote: >>From: Deepak Gupta <debug@rivosinc.com> >> >>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` which will return true if arch >>supports shadow stack and vma is shadow stack else stub returns false. >> >>There was a suggestion to name it as `vma_is_shadow_stack`. I preferred to >>keep `arch` prefix in there because it's each arch specific. >> >>Signed-off-by: Deepak Gupta <debug@rivosinc.com> >>--- >> include/linux/mm.h | 18 +++++++++++++++++- >> mm/gup.c | 5 +++-- >> mm/internal.h | 2 +- >> 3 files changed, 21 insertions(+), 4 deletions(-) >> >>diff --git a/include/linux/mm.h b/include/linux/mm.h >>index dfe0e8118669..15c70fc677a3 100644 >>--- a/include/linux/mm.h >>+++ b/include/linux/mm.h >>@@ -352,6 +352,10 @@ 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(vm_flags_t vm_flags) >>+{ >>+ return (vm_flags & VM_SHADOW_STACK); >>+} >> #endif >> #ifdef CONFIG_RISCV_USER_CFI >>@@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp); >> * with VM_SHARED. >> */ >> #define VM_SHADOW_STACK VM_WRITE >>+ >>+static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) >>+{ >>+ return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE); >>+} >>+ > >Please no such hacks just to work around the 32bit vmflags limitation. As I said in another response. Noted. And if there're no takers for 32bit on riscv (which highly likely is the case) This will go away in next version of patchsets. > >-- >Cheers, > >David / dhildenb >
On 25.01.24 18:07, Deepak Gupta wrote: > On Thu, Jan 25, 2024 at 09:18:07AM +0100, David Hildenbrand wrote: >> On 25.01.24 07:21, debug@rivosinc.com wrote: >>> From: Deepak Gupta <debug@rivosinc.com> >>> >>> 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` which will return true if arch >>> supports shadow stack and vma is shadow stack else stub returns false. >>> >>> There was a suggestion to name it as `vma_is_shadow_stack`. I preferred to >>> keep `arch` prefix in there because it's each arch specific. >>> >>> Signed-off-by: Deepak Gupta <debug@rivosinc.com> >>> --- >>> include/linux/mm.h | 18 +++++++++++++++++- >>> mm/gup.c | 5 +++-- >>> mm/internal.h | 2 +- >>> 3 files changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>> index dfe0e8118669..15c70fc677a3 100644 >>> --- a/include/linux/mm.h >>> +++ b/include/linux/mm.h >>> @@ -352,6 +352,10 @@ 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(vm_flags_t vm_flags) >>> +{ >>> + return (vm_flags & VM_SHADOW_STACK); >>> +} >>> #endif >>> #ifdef CONFIG_RISCV_USER_CFI >>> @@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp); >>> * with VM_SHARED. >>> */ >>> #define VM_SHADOW_STACK VM_WRITE >>> + >>> +static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) >>> +{ >>> + return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE); >>> +} >>> + >> >> Please no such hacks just to work around the 32bit vmflags limitation. > > As I said in another response. Noted. > And if there're no takers for 32bit on riscv (which highly likely is the case) > This will go away in next version of patchsets. Sorry for the (unusually for me) late reply. Simplifying to riscv64 sounds great. Alternatively, maybe VM_SHADOW_STACK is not even required at all on riscv if we can teach all code to only stare at arch_is_shadow_stack() instead. ... but, just using the same VM_SHADOW_STACK will it all much cleaner. Eventually, we can just stop playing arch-specific games with arch_is_shadow_stack and VM_SHADOW_STACK.
On Tue, Feb 13, 2024 at 11:34:59AM +0100, David Hildenbrand wrote: >On 25.01.24 18:07, Deepak Gupta wrote: >>On Thu, Jan 25, 2024 at 09:18:07AM +0100, David Hildenbrand wrote: >>>On 25.01.24 07:21, debug@rivosinc.com wrote: >>>>From: Deepak Gupta <debug@rivosinc.com> >>>> >>>>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` which will return true if arch >>>>supports shadow stack and vma is shadow stack else stub returns false. >>>> >>>>There was a suggestion to name it as `vma_is_shadow_stack`. I preferred to >>>>keep `arch` prefix in there because it's each arch specific. >>>> >>>>Signed-off-by: Deepak Gupta <debug@rivosinc.com> >>>>--- >>>> include/linux/mm.h | 18 +++++++++++++++++- >>>> mm/gup.c | 5 +++-- >>>> mm/internal.h | 2 +- >>>> 3 files changed, 21 insertions(+), 4 deletions(-) >>>> >>>>diff --git a/include/linux/mm.h b/include/linux/mm.h >>>>index dfe0e8118669..15c70fc677a3 100644 >>>>--- a/include/linux/mm.h >>>>+++ b/include/linux/mm.h >>>>@@ -352,6 +352,10 @@ 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(vm_flags_t vm_flags) >>>>+{ >>>>+ return (vm_flags & VM_SHADOW_STACK); >>>>+} >>>> #endif >>>> #ifdef CONFIG_RISCV_USER_CFI >>>>@@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp); >>>> * with VM_SHARED. >>>> */ >>>> #define VM_SHADOW_STACK VM_WRITE >>>>+ >>>>+static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) >>>>+{ >>>>+ return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE); >>>>+} >>>>+ >>> >>>Please no such hacks just to work around the 32bit vmflags limitation. >> >>As I said in another response. Noted. >>And if there're no takers for 32bit on riscv (which highly likely is the case) >>This will go away in next version of patchsets. > >Sorry for the (unusually for me) late reply. Simplifying to riscv64 >sounds great. > >Alternatively, maybe VM_SHADOW_STACK is not even required at all on >riscv if we can teach all code to only stare at arch_is_shadow_stack() >instead. Sorry for late reply. I think for risc-v this can be done, if done in following way static inline bool arch_is_shadow_stack(struct vm_flags_t vm_flags) { return (vm_get_page_prot(vm_flags) == PAGE_SHADOWSTACK); } But doing above will require following - Inventing a new PROT_XXX type (let's call it PROT_SHADOWSTACK) that is only exposed to kernel. PROT_SHADOWSTACK protection flag would allow `do_mmap` to do right thing and setup appropriate protection perms. We wouldn't want to expose this protection type to user mode (because `map_shadow_stack` already exists for that). Current patch series uses PROT_SHADOWSTACK because VM_SHADOW_STACK was aliased to VM_WRITE. - As you said teach all the generic code as well to use arch_is_shadow_stack which might become complicated (can't say for sure) > >... but, just using the same VM_SHADOW_STACK will it all much cleaner. >Eventually, we can just stop playing arch-specific games with >arch_is_shadow_stack and VM_SHADOW_STACK. Yeah for now, looks like easier thing to do. > >-- >Cheers, > >David / dhildenb >
diff --git a/include/linux/mm.h b/include/linux/mm.h index dfe0e8118669..15c70fc677a3 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -352,6 +352,10 @@ 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(vm_flags_t vm_flags) +{ + return (vm_flags & VM_SHADOW_STACK); +} #endif #ifdef CONFIG_RISCV_USER_CFI @@ -362,10 +366,22 @@ extern unsigned int kobjsize(const void *objp); * with VM_SHARED. */ #define VM_SHADOW_STACK VM_WRITE + +static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) +{ + return ((vm_flags & (VM_WRITE | VM_READ | VM_EXEC)) == VM_WRITE); +} + #endif #ifndef VM_SHADOW_STACK # define VM_SHADOW_STACK VM_NONE + +static inline bool arch_is_shadow_stack(vm_flags_t vm_flags) +{ + return false; +} + #endif #if defined(CONFIG_X86) @@ -3464,7 +3480,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 (vma->vm_flags && arch_is_shadow_stack(vma->vm_flags)) return PAGE_SIZE; return 0; diff --git a/mm/gup.c b/mm/gup.c index 231711efa390..45798782ed2c 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(vm_flags)) { if (!(gup_flags & FOLL_FORCE)) return -EFAULT; /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */ @@ -1069,7 +1069,8 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags) if (!is_cow_mapping(vm_flags)) return -EFAULT; } - } else if (!(vm_flags & VM_READ)) { + } else if (!(vm_flags & VM_READ) && !arch_is_shadow_stack(vm_flags)) { + /* reads allowed if its shadow stack vma */ if (!(gup_flags & FOLL_FORCE)) return -EFAULT; /* diff --git a/mm/internal.h b/mm/internal.h index b61034bd50f5..0abf00c93fe1 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(flags); } /*