| Message ID | 20231222235248.576482-1-debug@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] mm: abstract shadow stack vma behind arch_is_shadow_stack_vma | expand |
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"?
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. >
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.
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
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.
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 <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.
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 --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); } /*
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(-)