| Message ID | 20240403234054.2020347-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 04.04.24 01:34, Deepak Gupta wrote: > VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow stack VMA. > > This patch changes checks of VM_SHADOW_STACK flag in generic code to call > to a function `vma_is_shadow_stack` which will return true if its a > shadow stack vma and default stub (when support doesnt exist) returns false. > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > Suggested-by: Mike Rapoport <rppt@kernel.org> > --- > include/linux/mm.h | 13 ++++++++++++- > mm/gup.c | 5 +++-- > mm/internal.h | 2 +- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 64109f6c70f5..9952937be659 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -363,8 +363,19 @@ extern unsigned int kobjsize(const void *objp); > > #ifndef VM_SHADOW_STACK > # define VM_SHADOW_STACK VM_NONE > + > +static inline bool vma_is_shadow_stack(vm_flags_t vm_flags) > +{ > + return false; > +} > +#else > +static inline bool vma_is_shadow_stack(vm_flags_t vm_flags) > +{ > + return (vm_flags & VM_SHADOW_STACK); > +} > #endif You can simply do outside the ifdef static inline bool vma_is_shadow_stack(vm_flags_t vm_flags) { return !!(vm_flags & VM_SHADOW_STACK); } This will work even when VM_SHADOW_STACK is defined to be VM_NONE. > > + unrelated code change > #if defined(CONFIG_X86) > # define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */ > #elif defined(CONFIG_PPC) > @@ -3473,7 +3484,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 && vma_is_shadow_stack(vma->vm_flags)) Pretty sure: if (vma_is_shadow_stack(vma->vm_flags)) > return PAGE_SIZE; > > return 0; > diff --git a/mm/gup.c b/mm/gup.c > index df83182ec72d..a7a02eb0a6b3 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1053,7 +1053,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) || vma_is_shadow_stack(vm_flags)) { > if (!(gup_flags & FOLL_FORCE)) > return -EFAULT; > /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */ > @@ -1071,7 +1071,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) && !vma_is_shadow_stack(vm_flags)) { > + /* reads allowed if its shadow stack vma */ > if (!(gup_flags & FOLL_FORCE)) > return -EFAULT; > /* Unless I am missing something, this is not a simple cleanup. It should go into a separate patch with a clearly documented reason for that change.
On Thu, Apr 04, 2024 at 09:02:17PM +0200, David Hildenbrand wrote: >On 04.04.24 01:34, Deepak Gupta wrote: >> } >>- } else if (!(vm_flags & VM_READ)) { >>+ } else if (!(vm_flags & VM_READ) && !vma_is_shadow_stack(vm_flags)) { >>+ /* reads allowed if its shadow stack vma */ >> if (!(gup_flags & FOLL_FORCE)) >> return -EFAULT; >> /* > >Unless I am missing something, this is not a simple cleanup. It should >go into a separate patch with a clearly documented reason for that >change. I tried that here https://lore.kernel.org/linux-mm/CAKC1njTPBqtsAOn-CWhB+-8FaZ2KWkkz-vRZr7MZq=0yLUdjcQ@mail.gmail.com/T/ But at that time, VM_SHADOW_STACK for riscv meant only VM_WRITE. So I think there was obvious uneasiness with that part. Now we have VM_SHADOW_STACK pretty much same for all arches and only 64bit. I'll try it again as a separate patch. > >-- >Cheers, > >David / dhildenb >
diff --git a/include/linux/mm.h b/include/linux/mm.h index 64109f6c70f5..9952937be659 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -363,8 +363,19 @@ extern unsigned int kobjsize(const void *objp); #ifndef VM_SHADOW_STACK # define VM_SHADOW_STACK VM_NONE + +static inline bool vma_is_shadow_stack(vm_flags_t vm_flags) +{ + return false; +} +#else +static inline bool vma_is_shadow_stack(vm_flags_t vm_flags) +{ + return (vm_flags & VM_SHADOW_STACK); +} #endif + #if defined(CONFIG_X86) # define VM_PAT VM_ARCH_1 /* PAT reserves whole VMA at once (x86) */ #elif defined(CONFIG_PPC) @@ -3473,7 +3484,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 && vma_is_shadow_stack(vma->vm_flags)) return PAGE_SIZE; return 0; diff --git a/mm/gup.c b/mm/gup.c index df83182ec72d..a7a02eb0a6b3 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1053,7 +1053,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) || vma_is_shadow_stack(vm_flags)) { if (!(gup_flags & FOLL_FORCE)) return -EFAULT; /* hugetlb does not support FOLL_FORCE|FOLL_WRITE. */ @@ -1071,7 +1071,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) && !vma_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 f309a010d50f..5035b5a58df0 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) || vma_is_shadow_stack(flags); } /*
VM_SHADOW_STACK (alias to VM_HIGH_ARCH_5) to encode shadow stack VMA. This patch changes checks of VM_SHADOW_STACK flag in generic code to call to a function `vma_is_shadow_stack` which will return true if its a shadow stack vma and default stub (when support doesnt exist) returns false. Signed-off-by: Deepak Gupta <debug@rivosinc.com> Suggested-by: Mike Rapoport <rppt@kernel.org> --- include/linux/mm.h | 13 ++++++++++++- mm/gup.c | 5 +++-- mm/internal.h | 2 +- 3 files changed, 16 insertions(+), 4 deletions(-)