Re: [RFC PATCH v1 09/28] mm: abstract shadow stack vma behind `arch_is_shadow_stack`

Deepak Gupta
2024-02-22T01:33:01Z

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
>