diff mbox series

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

Message ID 20240125062739.1339782-10-debug@rivosinc.com
State New
Headers
Series riscv control-flow integrity for usermode |

Commit Message

Deepak Gupta Jan. 25, 2024, 6:21 a.m. UTC 
  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(-)

Comments

David Hildenbrand Jan. 25, 2024, 8:18 a.m. UTC | #1 
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.
Deepak Gupta Jan. 25, 2024, 5:07 p.m. UTC | #2 
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
>
David Hildenbrand Feb. 13, 2024, 10:34 a.m. UTC | #3 
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.
Deepak Gupta Feb. 22, 2024, 1:32 a.m. UTC | #4 
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 mbox series

Patch

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);
 }
 
 /*