[PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

Hi all,

Here's a series of cleanups I hacked together on top of a modified v13
of the Shadow Call Stack patches from Sami:

  https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck

The main changes are:

  * Move code out of arch/arm64 and into the core implementation
  * Store the full SCS stack pointer instead of the offset
  * Code simplification and general style things

I'd like to queue this on top early next week so that it can spend some
quality time in linux-next.

Cheers,

Will

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@am.com>
Cc: Jann Horn <jannh@google.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: <kernel-team@android.com>

--->8

Will Deacon (6):
  arm64: scs: Store absolute SCS stack pointer value in thread_info
  scs: Move accounting into alloc/free functions
  arm64: scs: Use 'scs_sp' register alias for x18
  scs: Move scs_overflow_check() out of architecture code
  scs: Remove references to asm/scs.h from core code
  scs: Move DEFINE_SCS macro into core code

 arch/Kconfig                         |  4 +--
 arch/arm64/include/asm/scs.h         | 29 ++++------------
 arch/arm64/include/asm/thread_info.h |  4 +--
 arch/arm64/kernel/asm-offsets.c      |  2 +-
 arch/arm64/kernel/entry.S            | 10 +++---
 arch/arm64/kernel/head.S             |  2 +-
 arch/arm64/kernel/process.c          |  2 --
 arch/arm64/kernel/scs.c              |  6 +---
 include/linux/scs.h                  | 16 +++++----
 kernel/sched/core.c                  |  3 ++
 kernel/scs.c                         | 52 +++++++++++++---------------
 11 files changed, 55 insertions(+), 75 deletions(-)

-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

Hi all,

Here's a series of cleanups I hacked together on top of a modified v13
of the Shadow Call Stack patches from Sami:

  https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck

The main changes are:

  * Move code out of arch/arm64 and into the core implementation
  * Store the full SCS stack pointer instead of the offset
  * Code simplification and general style things

I'd like to queue this on top early next week so that it can spend some
quality time in linux-next.

Cheers,

Will

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@am.com>
Cc: Jann Horn <jannh@google.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: <kernel-team@android.com>

--->8

Will Deacon (6):
  arm64: scs: Store absolute SCS stack pointer value in thread_info
  scs: Move accounting into alloc/free functions
  arm64: scs: Use 'scs_sp' register alias for x18
  scs: Move scs_overflow_check() out of architecture code
  scs: Remove references to asm/scs.h from core code
  scs: Move DEFINE_SCS macro into core code

 arch/Kconfig                         |  4 +--
 arch/arm64/include/asm/scs.h         | 29 ++++------------
 arch/arm64/include/asm/thread_info.h |  4 +--
 arch/arm64/kernel/asm-offsets.c      |  2 +-
 arch/arm64/kernel/entry.S            | 10 +++---
 arch/arm64/kernel/head.S             |  2 +-
 arch/arm64/kernel/process.c          |  2 --
 arch/arm64/kernel/scs.c              |  6 +---
 include/linux/scs.h                  | 16 +++++----
 kernel/sched/core.c                  |  3 ++
 kernel/scs.c                         | 52 +++++++++++++---------------
 11 files changed, 55 insertions(+), 75 deletions(-)

-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Will Deacon]

Storing the SCS information in thread_info as a {base,offset} pair
introduces an additional load instruction on the ret-to-user path,
since the SCS stack pointer in x18 has to be converted back to an offset
by subtracting the base.

Replace the offset with the absolute SCS stack pointer value instead
and avoid the redundant load.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h         | 9 ++++-----
 arch/arm64/include/asm/thread_info.h | 4 ++--
 arch/arm64/kernel/asm-offsets.c      | 2 +-
 include/linux/scs.h                  | 8 ++++----
 kernel/scs.c                         | 3 +--
 5 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 96549353b0cb..6b8cf4352fe3 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -4,16 +4,15 @@
 
 #ifdef __ASSEMBLY__
 
+#include <asm/asm-offsets.h>
+
 #ifdef CONFIG_SHADOW_CALL_STACK
 	.macro scs_load tsk, tmp
-	ldp	x18, \tmp, [\tsk, #TSK_TI_SCS_BASE]
-	add	x18, x18, \tmp
+	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 
 	.macro scs_save tsk, tmp
-	ldr	\tmp, [\tsk, #TSK_TI_SCS_BASE]
-	sub	\tmp, x18, \tmp
-	str	\tmp, [\tsk, #TSK_TI_SCS_OFFSET]
+	str	x18, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 #else
 	.macro scs_load tsk, tmp
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 9df79c0a4c43..6ea8b6a26ae9 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -43,7 +43,7 @@ struct thread_info {
 	};
 #ifdef CONFIG_SHADOW_CALL_STACK
 	void			*scs_base;
-	unsigned long		scs_offset;
+	void			*scs_sp;
 #endif
 };
 
@@ -107,7 +107,7 @@ void arch_release_task_struct(struct task_struct *tsk);
 #ifdef CONFIG_SHADOW_CALL_STACK
 #define INIT_SCS							\
 	.scs_base	= init_shadow_call_stack,			\
-	.scs_offset	= 0,
+	.scs_sp		= init_shadow_call_stack,
 #else
 #define INIT_SCS
 #endif
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index d7934250b68c..a098a45f63d8 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -36,7 +36,7 @@ int main(void)
 #endif
 #ifdef CONFIG_SHADOW_CALL_STACK
   DEFINE(TSK_TI_SCS_BASE,	offsetof(struct task_struct, thread_info.scs_base));
-  DEFINE(TSK_TI_SCS_OFFSET,	offsetof(struct task_struct, thread_info.scs_offset));
+  DEFINE(TSK_TI_SCS_SP,		offsetof(struct task_struct, thread_info.scs_sp));
 #endif
   DEFINE(TSK_STACK,		offsetof(struct task_struct, stack));
 #ifdef CONFIG_STACKPROTECTOR
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 3f3662621a27..0eb2485ef832 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -27,7 +27,7 @@
 #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
 
 #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
-#define task_scs_offset(tsk)	(task_thread_info(tsk)->scs_offset)
+#define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
 
 void scs_init(void);
 int scs_prepare(struct task_struct *tsk, int node);
@@ -39,7 +39,7 @@ static inline void scs_task_reset(struct task_struct *tsk)
 	 * Reset the shadow stack to the base address in case the task
 	 * is reused.
 	 */
-	task_scs_offset(tsk) = 0;
+	task_scs_sp(tsk) = task_scs(tsk);
 }
 
 static inline unsigned long *__scs_magic(void *s)
@@ -50,9 +50,9 @@ static inline unsigned long *__scs_magic(void *s)
 static inline bool scs_corrupted(struct task_struct *tsk)
 {
 	unsigned long *magic = __scs_magic(task_scs(tsk));
+	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
 
-	return (task_scs_offset(tsk) >= SCS_SIZE - 1 ||
-		READ_ONCE_NOCHECK(*magic) != SCS_END_MAGIC);
+	return sz >= SCS_SIZE - 1 || READ_ONCE_NOCHECK(*magic) != SCS_END_MAGIC;
 }
 
 #else /* CONFIG_SHADOW_CALL_STACK */
diff --git a/kernel/scs.c b/kernel/scs.c
index 9389c28f0853..5ff8663e4a67 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
 	if (!s)
 		return -ENOMEM;
 
-	task_scs(tsk) = s;
-	task_scs_offset(tsk) = 0;
+	task_scs(tsk) = task_scs_sp(tsk) = s;
 	scs_account(tsk, 1);
 	return 0;
 }
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Will Deacon]

Storing the SCS information in thread_info as a {base,offset} pair
introduces an additional load instruction on the ret-to-user path,
since the SCS stack pointer in x18 has to be converted back to an offset
by subtracting the base.

Replace the offset with the absolute SCS stack pointer value instead
and avoid the redundant load.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h         | 9 ++++-----
 arch/arm64/include/asm/thread_info.h | 4 ++--
 arch/arm64/kernel/asm-offsets.c      | 2 +-
 include/linux/scs.h                  | 8 ++++----
 kernel/scs.c                         | 3 +--
 5 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 96549353b0cb..6b8cf4352fe3 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -4,16 +4,15 @@
 
 #ifdef __ASSEMBLY__
 
+#include <asm/asm-offsets.h>
+
 #ifdef CONFIG_SHADOW_CALL_STACK
 	.macro scs_load tsk, tmp
-	ldp	x18, \tmp, [\tsk, #TSK_TI_SCS_BASE]
-	add	x18, x18, \tmp
+	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 
 	.macro scs_save tsk, tmp
-	ldr	\tmp, [\tsk, #TSK_TI_SCS_BASE]
-	sub	\tmp, x18, \tmp
-	str	\tmp, [\tsk, #TSK_TI_SCS_OFFSET]
+	str	x18, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 #else
 	.macro scs_load tsk, tmp
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 9df79c0a4c43..6ea8b6a26ae9 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -43,7 +43,7 @@ struct thread_info {
 	};
 #ifdef CONFIG_SHADOW_CALL_STACK
 	void			*scs_base;
-	unsigned long		scs_offset;
+	void			*scs_sp;
 #endif
 };
 
@@ -107,7 +107,7 @@ void arch_release_task_struct(struct task_struct *tsk);
 #ifdef CONFIG_SHADOW_CALL_STACK
 #define INIT_SCS							\
 	.scs_base	= init_shadow_call_stack,			\
-	.scs_offset	= 0,
+	.scs_sp		= init_shadow_call_stack,
 #else
 #define INIT_SCS
 #endif
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index d7934250b68c..a098a45f63d8 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -36,7 +36,7 @@ int main(void)
 #endif
 #ifdef CONFIG_SHADOW_CALL_STACK
   DEFINE(TSK_TI_SCS_BASE,	offsetof(struct task_struct, thread_info.scs_base));
-  DEFINE(TSK_TI_SCS_OFFSET,	offsetof(struct task_struct, thread_info.scs_offset));
+  DEFINE(TSK_TI_SCS_SP,		offsetof(struct task_struct, thread_info.scs_sp));
 #endif
   DEFINE(TSK_STACK,		offsetof(struct task_struct, stack));
 #ifdef CONFIG_STACKPROTECTOR
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 3f3662621a27..0eb2485ef832 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -27,7 +27,7 @@
 #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
 
 #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
-#define task_scs_offset(tsk)	(task_thread_info(tsk)->scs_offset)
+#define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
 
 void scs_init(void);
 int scs_prepare(struct task_struct *tsk, int node);
@@ -39,7 +39,7 @@ static inline void scs_task_reset(struct task_struct *tsk)
 	 * Reset the shadow stack to the base address in case the task
 	 * is reused.
 	 */
-	task_scs_offset(tsk) = 0;
+	task_scs_sp(tsk) = task_scs(tsk);
 }
 
 static inline unsigned long *__scs_magic(void *s)
@@ -50,9 +50,9 @@ static inline unsigned long *__scs_magic(void *s)
 static inline bool scs_corrupted(struct task_struct *tsk)
 {
 	unsigned long *magic = __scs_magic(task_scs(tsk));
+	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
 
-	return (task_scs_offset(tsk) >= SCS_SIZE - 1 ||
-		READ_ONCE_NOCHECK(*magic) != SCS_END_MAGIC);
+	return sz >= SCS_SIZE - 1 || READ_ONCE_NOCHECK(*magic) != SCS_END_MAGIC;
 }
 
 #else /* CONFIG_SHADOW_CALL_STACK */
diff --git a/kernel/scs.c b/kernel/scs.c
index 9389c28f0853..5ff8663e4a67 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
 	if (!s)
 		return -ENOMEM;
 
-	task_scs(tsk) = s;
-	task_scs_offset(tsk) = 0;
+	task_scs(tsk) = task_scs_sp(tsk) = s;
 	scs_account(tsk, 1);
 	return 0;
 }
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> Storing the SCS information in thread_info as a {base,offset} pair
> introduces an additional load instruction on the ret-to-user path,
> since the SCS stack pointer in x18 has to be converted back to an offset
> by subtracting the base.
> 
> Replace the offset with the absolute SCS stack pointer value instead
> and avoid the redundant load.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

One trivial nit below, but regardless this looks sound to me, and I
certainly prefer having the absolute address rather than an offset, so:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> diff --git a/kernel/scs.c b/kernel/scs.c
> index 9389c28f0853..5ff8663e4a67 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
>  	if (!s)
>  		return -ENOMEM;
>  
> -	task_scs(tsk) = s;
> -	task_scs_offset(tsk) = 0;
> +	task_scs(tsk) = task_scs_sp(tsk) = s;

I think this would be more legible as two statements:

|	task_sys(tsk) = s;
|	task_scs_sp(tsk) = s;

... as we usually save `foo = bar = baz` stuff for the start of a
function or within loop conditions.

Thanks,
Mark.

>  	scs_account(tsk, 1);
>  	return 0;
>  }
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> Storing the SCS information in thread_info as a {base,offset} pair
> introduces an additional load instruction on the ret-to-user path,
> since the SCS stack pointer in x18 has to be converted back to an offset
> by subtracting the base.
> 
> Replace the offset with the absolute SCS stack pointer value instead
> and avoid the redundant load.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

One trivial nit below, but regardless this looks sound to me, and I
certainly prefer having the absolute address rather than an offset, so:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> diff --git a/kernel/scs.c b/kernel/scs.c
> index 9389c28f0853..5ff8663e4a67 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
>  	if (!s)
>  		return -ENOMEM;
>  
> -	task_scs(tsk) = s;
> -	task_scs_offset(tsk) = 0;
> +	task_scs(tsk) = task_scs_sp(tsk) = s;

I think this would be more legible as two statements:

|	task_sys(tsk) = s;
|	task_scs_sp(tsk) = s;

... as we usually save `foo = bar = baz` stuff for the start of a
function or within loop conditions.

Thanks,
Mark.

>  	scs_account(tsk, 1);
>  	return 0;
>  }
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Will Deacon]

On Mon, May 18, 2020 at 12:37:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> > Storing the SCS information in thread_info as a {base,offset} pair
> > introduces an additional load instruction on the ret-to-user path,
> > since the SCS stack pointer in x18 has to be converted back to an offset
> > by subtracting the base.
> > 
> > Replace the offset with the absolute SCS stack pointer value instead
> > and avoid the redundant load.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> One trivial nit below, but regardless this looks sound to me, and I
> certainly prefer having the absolute address rather than an offset, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 9389c28f0853..5ff8663e4a67 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
> >  	if (!s)
> >  		return -ENOMEM;
> >  
> > -	task_scs(tsk) = s;
> > -	task_scs_offset(tsk) = 0;
> > +	task_scs(tsk) = task_scs_sp(tsk) = s;
> 
> I think this would be more legible as two statements:
> 
> |	task_sys(tsk) = s;
> |	task_scs_sp(tsk) = s;

I think it's nice to be able to say:

	task_scs(tsk) = task_scs_sp(tsk);

because it makes it very clear that they are initialised to the same thing.
Having it as two statements means somebody will update one and forget to
update the other one.

> ... as we usually save `foo = bar = baz` stuff for the start of a
> function or within loop conditions.

Hmm, I can't really find anything consistent in that regard, to be honest
with you. Did I miss something in the coding style doc?

I'll leave it as-is for now.

Will

Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

[Will Deacon]

On Mon, May 18, 2020 at 12:37:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> > Storing the SCS information in thread_info as a {base,offset} pair
> > introduces an additional load instruction on the ret-to-user path,
> > since the SCS stack pointer in x18 has to be converted back to an offset
> > by subtracting the base.
> > 
> > Replace the offset with the absolute SCS stack pointer value instead
> > and avoid the redundant load.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> One trivial nit below, but regardless this looks sound to me, and I
> certainly prefer having the absolute address rather than an offset, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 9389c28f0853..5ff8663e4a67 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
> >  	if (!s)
> >  		return -ENOMEM;
> >  
> > -	task_scs(tsk) = s;
> > -	task_scs_offset(tsk) = 0;
> > +	task_scs(tsk) = task_scs_sp(tsk) = s;
> 
> I think this would be more legible as two statements:
> 
> |	task_sys(tsk) = s;
> |	task_scs_sp(tsk) = s;

I think it's nice to be able to say:

	task_scs(tsk) = task_scs_sp(tsk);

because it makes it very clear that they are initialised to the same thing.
Having it as two statements means somebody will update one and forget to
update the other one.

> ... as we usually save `foo = bar = baz` stuff for the start of a
> function or within loop conditions.

Hmm, I can't really find anything consistent in that regard, to be honest
with you. Did I miss something in the coding style doc?

I'll leave it as-is for now.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] scs: Move accounting into alloc/free functions

[Will Deacon]

There's no need to perform the shadow stack page accounting independently
of the lifetime of the underlying allocation, so call the accounting code
from the {alloc,free}() functions and simplify the code in the process.

Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/scs.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/kernel/scs.c b/kernel/scs.c
index 5ff8663e4a67..aea841cd7586 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -14,25 +14,35 @@
 
 static struct kmem_cache *scs_cache;
 
+static void __scs_account(void *s, int account)
+{
+	struct page *scs_page = virt_to_page(s);
+
+	mod_zone_page_state(page_zone(scs_page), NR_KERNEL_SCS_KB,
+			    account * (SCS_SIZE / SZ_1K));
+}
+
 static void *scs_alloc(int node)
 {
-	void *s;
-
-	s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
-	if (s) {
-		*__scs_magic(s) = SCS_END_MAGIC;
-		/*
-		 * Poison the allocation to catch unintentional accesses to
-		 * the shadow stack when KASAN is enabled.
-		 */
-		kasan_poison_object_data(scs_cache, s);
-	}
+	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
+
+	if (!s)
+		return NULL;
 
+	*__scs_magic(s) = SCS_END_MAGIC;
+
+	/*
+	 * Poison the allocation to catch unintentional accesses to
+	 * the shadow stack when KASAN is enabled.
+	 */
+	kasan_poison_object_data(scs_cache, s);
+	__scs_account(s, 1);
 	return s;
 }
 
 static void scs_free(void *s)
 {
+	__scs_account(s, -1);
 	kasan_unpoison_object_data(scs_cache, s);
 	kmem_cache_free(scs_cache, s);
 }
@@ -42,17 +52,6 @@ void __init scs_init(void)
 	scs_cache = kmem_cache_create("scs_cache", SCS_SIZE, 0, 0, NULL);
 }
 
-static struct page *__scs_page(struct task_struct *tsk)
-{
-	return virt_to_page(task_scs(tsk));
-}
-
-static void scs_account(struct task_struct *tsk, int account)
-{
-	mod_zone_page_state(page_zone(__scs_page(tsk)), NR_KERNEL_SCS_KB,
-		account * (SCS_SIZE / 1024));
-}
-
 int scs_prepare(struct task_struct *tsk, int node)
 {
 	void *s = scs_alloc(node);
@@ -61,7 +60,6 @@ int scs_prepare(struct task_struct *tsk, int node)
 		return -ENOMEM;
 
 	task_scs(tsk) = task_scs_sp(tsk) = s;
-	scs_account(tsk, 1);
 	return 0;
 }
 
@@ -102,6 +100,5 @@ void scs_release(struct task_struct *tsk)
 
 	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
 	scs_check_usage(tsk);
-	scs_account(tsk, -1);
 	scs_free(s);
 }
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 2/6] scs: Move accounting into alloc/free functions

[Will Deacon]

There's no need to perform the shadow stack page accounting independently
of the lifetime of the underlying allocation, so call the accounting code
from the {alloc,free}() functions and simplify the code in the process.

Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/scs.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/kernel/scs.c b/kernel/scs.c
index 5ff8663e4a67..aea841cd7586 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -14,25 +14,35 @@
 
 static struct kmem_cache *scs_cache;
 
+static void __scs_account(void *s, int account)
+{
+	struct page *scs_page = virt_to_page(s);
+
+	mod_zone_page_state(page_zone(scs_page), NR_KERNEL_SCS_KB,
+			    account * (SCS_SIZE / SZ_1K));
+}
+
 static void *scs_alloc(int node)
 {
-	void *s;
-
-	s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
-	if (s) {
-		*__scs_magic(s) = SCS_END_MAGIC;
-		/*
-		 * Poison the allocation to catch unintentional accesses to
-		 * the shadow stack when KASAN is enabled.
-		 */
-		kasan_poison_object_data(scs_cache, s);
-	}
+	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
+
+	if (!s)
+		return NULL;
 
+	*__scs_magic(s) = SCS_END_MAGIC;
+
+	/*
+	 * Poison the allocation to catch unintentional accesses to
+	 * the shadow stack when KASAN is enabled.
+	 */
+	kasan_poison_object_data(scs_cache, s);
+	__scs_account(s, 1);
 	return s;
 }
 
 static void scs_free(void *s)
 {
+	__scs_account(s, -1);
 	kasan_unpoison_object_data(scs_cache, s);
 	kmem_cache_free(scs_cache, s);
 }
@@ -42,17 +52,6 @@ void __init scs_init(void)
 	scs_cache = kmem_cache_create("scs_cache", SCS_SIZE, 0, 0, NULL);
 }
 
-static struct page *__scs_page(struct task_struct *tsk)
-{
-	return virt_to_page(task_scs(tsk));
-}
-
-static void scs_account(struct task_struct *tsk, int account)
-{
-	mod_zone_page_state(page_zone(__scs_page(tsk)), NR_KERNEL_SCS_KB,
-		account * (SCS_SIZE / 1024));
-}
-
 int scs_prepare(struct task_struct *tsk, int node)
 {
 	void *s = scs_alloc(node);
@@ -61,7 +60,6 @@ int scs_prepare(struct task_struct *tsk, int node)
 		return -ENOMEM;
 
 	task_scs(tsk) = task_scs_sp(tsk) = s;
-	scs_account(tsk, 1);
 	return 0;
 }
 
@@ -102,6 +100,5 @@ void scs_release(struct task_struct *tsk)
 
 	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
 	scs_check_usage(tsk);
-	scs_account(tsk, -1);
 	scs_free(s);
 }
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] scs: Move accounting into alloc/free functions

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:52PM +0100, Will Deacon wrote:
> There's no need to perform the shadow stack page accounting independently
> of the lifetime of the underlying allocation, so call the accounting code
> from the {alloc,free}() functions and simplify the code in the process.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  kernel/scs.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)

One (super trivial) nit below, but regardless this looks like a sound
and sensible cleanup, so:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> diff --git a/kernel/scs.c b/kernel/scs.c
> index 5ff8663e4a67..aea841cd7586 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -14,25 +14,35 @@

>  static void *scs_alloc(int node)
>  {

> +	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
> +
> +	if (!s)
> +		return NULL;

Super trivial nit, but could we omit the line space between these two,
to fit with usual style?

Mark.

Re: [PATCH 2/6] scs: Move accounting into alloc/free functions

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:52PM +0100, Will Deacon wrote:
> There's no need to perform the shadow stack page accounting independently
> of the lifetime of the underlying allocation, so call the accounting code
> from the {alloc,free}() functions and simplify the code in the process.
> 
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  kernel/scs.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)

One (super trivial) nit below, but regardless this looks like a sound
and sensible cleanup, so:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> diff --git a/kernel/scs.c b/kernel/scs.c
> index 5ff8663e4a67..aea841cd7586 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -14,25 +14,35 @@

>  static void *scs_alloc(int node)
>  {

> +	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
> +
> +	if (!s)
> +		return NULL;

Super trivial nit, but could we omit the line space between these two,
to fit with usual style?

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] scs: Move accounting into alloc/free functions

[Will Deacon]

On Mon, May 18, 2020 at 12:38:58PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:52PM +0100, Will Deacon wrote:
> > There's no need to perform the shadow stack page accounting independently
> > of the lifetime of the underlying allocation, so call the accounting code
> > from the {alloc,free}() functions and simplify the code in the process.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  kernel/scs.c | 45 +++++++++++++++++++++------------------------
> >  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> One (super trivial) nit below, but regardless this looks like a sound
> and sensible cleanup, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 5ff8663e4a67..aea841cd7586 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -14,25 +14,35 @@
> 
> >  static void *scs_alloc(int node)
> >  {
> 
> > +	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
> > +
> > +	if (!s)
> > +		return NULL;
> 
> Super trivial nit, but could we omit the line space between these two,
> to fit with usual style?

I really like having the empty line after the last variable declaration.

Sorry,

Will

Re: [PATCH 2/6] scs: Move accounting into alloc/free functions

[Will Deacon]

On Mon, May 18, 2020 at 12:38:58PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:52PM +0100, Will Deacon wrote:
> > There's no need to perform the shadow stack page accounting independently
> > of the lifetime of the underlying allocation, so call the accounting code
> > from the {alloc,free}() functions and simplify the code in the process.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >  kernel/scs.c | 45 +++++++++++++++++++++------------------------
> >  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> One (super trivial) nit below, but regardless this looks like a sound
> and sensible cleanup, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 5ff8663e4a67..aea841cd7586 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -14,25 +14,35 @@
> 
> >  static void *scs_alloc(int node)
> >  {
> 
> > +	void *s = kmem_cache_alloc_node(scs_cache, GFP_SCS, node);
> > +
> > +	if (!s)
> > +		return NULL;
> 
> Super trivial nit, but could we omit the line space between these two,
> to fit with usual style?

I really like having the empty line after the last variable declaration.

Sorry,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Will Deacon]

x18 holds the SCS stack pointer value, so introduce a register alias to
make this easier to read in assembly code.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h |  6 ++++--
 arch/arm64/kernel/entry.S    | 10 +++++-----
 arch/arm64/kernel/head.S     |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 6b8cf4352fe3..d46efdd2060a 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -7,12 +7,14 @@
 #include <asm/asm-offsets.h>
 
 #ifdef CONFIG_SHADOW_CALL_STACK
+	scs_sp	.req	x18
+
 	.macro scs_load tsk, tmp
-	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
+	ldr	scs_sp, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 
 	.macro scs_save tsk, tmp
-	str	x18, [\tsk, #TSK_TI_SCS_SP]
+	str	scs_sp, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 #else
 	.macro scs_load tsk, tmp
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index cb0516e6f963..741faf0706f1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 #ifdef CONFIG_SHADOW_CALL_STACK
-	mov	x24, x18		// preserve the original shadow stack
+	mov	x24, scs_sp		// preserve the original shadow stack
 #endif
 
 	/*
@@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 
 #ifdef CONFIG_SHADOW_CALL_STACK
 	/* also switch to the irq shadow stack */
-	adr_this_cpu x18, irq_shadow_call_stack, x26
+	adr_this_cpu scs_sp, irq_shadow_call_stack, x26
 #endif
 
 9998:
@@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 	.macro	irq_stack_exit
 	mov	sp, x19
 #ifdef CONFIG_SHADOW_CALL_STACK
-	mov	x18, x24
+	mov	scs_sp, x24
 #endif
 	.endm
 
@@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler)
 #ifdef CONFIG_SHADOW_CALL_STACK
 	/* Use a separate shadow call stack for normal and critical events */
 	cbnz	w4, 3f
-	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6
+	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6
 	b	4f
-3:	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6
+3:	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6
 4:
 #endif
 
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 2b01c19c5483..1293baddfd20 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
 	mov	x29, sp
 
 #ifdef CONFIG_SHADOW_CALL_STACK
-	adr_l	x18, init_shadow_call_stack	// Set shadow call stack
+	adr_l	scs_sp, init_shadow_call_stack	// Set shadow call stack
 #endif
 
 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Will Deacon]

x18 holds the SCS stack pointer value, so introduce a register alias to
make this easier to read in assembly code.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h |  6 ++++--
 arch/arm64/kernel/entry.S    | 10 +++++-----
 arch/arm64/kernel/head.S     |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index 6b8cf4352fe3..d46efdd2060a 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -7,12 +7,14 @@
 #include <asm/asm-offsets.h>
 
 #ifdef CONFIG_SHADOW_CALL_STACK
+	scs_sp	.req	x18
+
 	.macro scs_load tsk, tmp
-	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
+	ldr	scs_sp, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 
 	.macro scs_save tsk, tmp
-	str	x18, [\tsk, #TSK_TI_SCS_SP]
+	str	scs_sp, [\tsk, #TSK_TI_SCS_SP]
 	.endm
 #else
 	.macro scs_load tsk, tmp
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index cb0516e6f963..741faf0706f1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 #ifdef CONFIG_SHADOW_CALL_STACK
-	mov	x24, x18		// preserve the original shadow stack
+	mov	x24, scs_sp		// preserve the original shadow stack
 #endif
 
 	/*
@@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 
 #ifdef CONFIG_SHADOW_CALL_STACK
 	/* also switch to the irq shadow stack */
-	adr_this_cpu x18, irq_shadow_call_stack, x26
+	adr_this_cpu scs_sp, irq_shadow_call_stack, x26
 #endif
 
 9998:
@@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
 	.macro	irq_stack_exit
 	mov	sp, x19
 #ifdef CONFIG_SHADOW_CALL_STACK
-	mov	x18, x24
+	mov	scs_sp, x24
 #endif
 	.endm
 
@@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler)
 #ifdef CONFIG_SHADOW_CALL_STACK
 	/* Use a separate shadow call stack for normal and critical events */
 	cbnz	w4, 3f
-	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6
+	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6
 	b	4f
-3:	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6
+3:	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6
 4:
 #endif
 
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 2b01c19c5483..1293baddfd20 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
 	mov	x29, sp
 
 #ifdef CONFIG_SHADOW_CALL_STACK
-	adr_l	x18, init_shadow_call_stack	// Set shadow call stack
+	adr_l	scs_sp, init_shadow_call_stack	// Set shadow call stack
 #endif
 
 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> x18 holds the SCS stack pointer value, so introduce a register alias to
> make this easier to read in assembly code.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

I scanned through arm64 for all instances of x18, and it looks like
you've covered all the relevant uses here. In kvm we save/restore x18 a
bunch becasue it might be a platform register, but we do that
unconditionally and without knowledge of what it contains, so I think
that's fine to leave as-is. Therefore:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

As an aside, the comment in entry-ftrace.S is now stale where it says
that x18 is safe to clobber. I can send a patch to clean that up, unless
you want to do that yourself.

Mark.

> ---
>  arch/arm64/include/asm/scs.h |  6 ++++--
>  arch/arm64/kernel/entry.S    | 10 +++++-----
>  arch/arm64/kernel/head.S     |  2 +-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
> index 6b8cf4352fe3..d46efdd2060a 100644
> --- a/arch/arm64/include/asm/scs.h
> +++ b/arch/arm64/include/asm/scs.h
> @@ -7,12 +7,14 @@
>  #include <asm/asm-offsets.h>
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
> +	scs_sp	.req	x18
> +
>  	.macro scs_load tsk, tmp
> -	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
> +	ldr	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
>  
>  	.macro scs_save tsk, tmp
> -	str	x18, [\tsk, #TSK_TI_SCS_SP]
> +	str	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
>  #else
>  	.macro scs_load tsk, tmp
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index cb0516e6f963..741faf0706f1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  	.macro	irq_stack_entry
>  	mov	x19, sp			// preserve the original sp
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	mov	x24, x18		// preserve the original shadow stack
> +	mov	x24, scs_sp		// preserve the original shadow stack
>  #endif
>  
>  	/*
> @@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	/* also switch to the irq shadow stack */
> -	adr_this_cpu x18, irq_shadow_call_stack, x26
> +	adr_this_cpu scs_sp, irq_shadow_call_stack, x26
>  #endif
>  
>  9998:
> @@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  	.macro	irq_stack_exit
>  	mov	sp, x19
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	mov	x18, x24
> +	mov	scs_sp, x24
>  #endif
>  	.endm
>  
> @@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler)
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	/* Use a separate shadow call stack for normal and critical events */
>  	cbnz	w4, 3f
> -	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6
> +	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6
>  	b	4f
> -3:	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6
> +3:	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6
>  4:
>  #endif
>  
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 2b01c19c5483..1293baddfd20 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
>  	mov	x29, sp
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	adr_l	x18, init_shadow_call_stack	// Set shadow call stack
> +	adr_l	scs_sp, init_shadow_call_stack	// Set shadow call stack
>  #endif
>  
>  	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> x18 holds the SCS stack pointer value, so introduce a register alias to
> make this easier to read in assembly code.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

I scanned through arm64 for all instances of x18, and it looks like
you've covered all the relevant uses here. In kvm we save/restore x18 a
bunch becasue it might be a platform register, but we do that
unconditionally and without knowledge of what it contains, so I think
that's fine to leave as-is. Therefore:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

As an aside, the comment in entry-ftrace.S is now stale where it says
that x18 is safe to clobber. I can send a patch to clean that up, unless
you want to do that yourself.

Mark.

> ---
>  arch/arm64/include/asm/scs.h |  6 ++++--
>  arch/arm64/kernel/entry.S    | 10 +++++-----
>  arch/arm64/kernel/head.S     |  2 +-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
> index 6b8cf4352fe3..d46efdd2060a 100644
> --- a/arch/arm64/include/asm/scs.h
> +++ b/arch/arm64/include/asm/scs.h
> @@ -7,12 +7,14 @@
>  #include <asm/asm-offsets.h>
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
> +	scs_sp	.req	x18
> +
>  	.macro scs_load tsk, tmp
> -	ldr	x18, [\tsk, #TSK_TI_SCS_SP]
> +	ldr	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
>  
>  	.macro scs_save tsk, tmp
> -	str	x18, [\tsk, #TSK_TI_SCS_SP]
> +	str	scs_sp, [\tsk, #TSK_TI_SCS_SP]
>  	.endm
>  #else
>  	.macro scs_load tsk, tmp
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index cb0516e6f963..741faf0706f1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -394,7 +394,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  	.macro	irq_stack_entry
>  	mov	x19, sp			// preserve the original sp
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	mov	x24, x18		// preserve the original shadow stack
> +	mov	x24, scs_sp		// preserve the original shadow stack
>  #endif
>  
>  	/*
> @@ -416,7 +416,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	/* also switch to the irq shadow stack */
> -	adr_this_cpu x18, irq_shadow_call_stack, x26
> +	adr_this_cpu scs_sp, irq_shadow_call_stack, x26
>  #endif
>  
>  9998:
> @@ -430,7 +430,7 @@ alternative_insn eret, nop, ARM64_UNMAP_KERNEL_AT_EL0
>  	.macro	irq_stack_exit
>  	mov	sp, x19
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	mov	x18, x24
> +	mov	scs_sp, x24
>  #endif
>  	.endm
>  
> @@ -1071,9 +1071,9 @@ SYM_CODE_START(__sdei_asm_handler)
>  #ifdef CONFIG_SHADOW_CALL_STACK
>  	/* Use a separate shadow call stack for normal and critical events */
>  	cbnz	w4, 3f
> -	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_normal, tmp=x6
> +	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_normal, tmp=x6
>  	b	4f
> -3:	adr_this_cpu dst=x18, sym=sdei_shadow_call_stack_critical, tmp=x6
> +3:	adr_this_cpu dst=scs_sp, sym=sdei_shadow_call_stack_critical, tmp=x6
>  4:
>  #endif
>  
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 2b01c19c5483..1293baddfd20 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -426,7 +426,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
>  	mov	x29, sp
>  
>  #ifdef CONFIG_SHADOW_CALL_STACK
> -	adr_l	x18, init_shadow_call_stack	// Set shadow call stack
> +	adr_l	scs_sp, init_shadow_call_stack	// Set shadow call stack
>  #endif
>  
>  	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Will Deacon]

On Mon, May 18, 2020 at 12:55:47PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> > x18 holds the SCS stack pointer value, so introduce a register alias to
> > make this easier to read in assembly code.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> I scanned through arm64 for all instances of x18, and it looks like
> you've covered all the relevant uses here. In kvm we save/restore x18 a
> bunch becasue it might be a platform register, but we do that
> unconditionally and without knowledge of what it contains, so I think
> that's fine to leave as-is. Therefore:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> As an aside, the comment in entry-ftrace.S is now stale where it says
> that x18 is safe to clobber. I can send a patch to clean that up, unless
> you want to do that yourself.

Thanks, I'll fix that up (see below). Also, apologies for typo'ing your
email address when I sent this out on Friday.

Will

--->8

From 7e86208cd6541c1229bc1fcd206596308d1727f8 Mon Sep 17 00:00:00 2001
From: Will Deacon <will@kernel.org>
Date: Mon, 18 May 2020 14:01:01 +0100
Subject: [PATCH] arm64: entry-ftrace.S: Update comment to indicate that x18 is
 live

The Shadow Call Stack pointer is held in x18, so update the ftrace
entry comment to indicate that it cannot be safely clobbered.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry-ftrace.S | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 833d48c9acb5..a338f40e64d3 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -23,8 +23,9 @@
  *
  * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
  *
- * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
- * live, and x9-x18 are safe to clobber.
+ * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are
+ * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to
+ * clobber.
  *
  * We save the callsite's context into a pt_regs before invoking any ftrace
  * callbacks. So that we can get a sensible backtrace, we create a stack record
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Will Deacon]

On Mon, May 18, 2020 at 12:55:47PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> > x18 holds the SCS stack pointer value, so introduce a register alias to
> > make this easier to read in assembly code.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> I scanned through arm64 for all instances of x18, and it looks like
> you've covered all the relevant uses here. In kvm we save/restore x18 a
> bunch becasue it might be a platform register, but we do that
> unconditionally and without knowledge of what it contains, so I think
> that's fine to leave as-is. Therefore:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> As an aside, the comment in entry-ftrace.S is now stale where it says
> that x18 is safe to clobber. I can send a patch to clean that up, unless
> you want to do that yourself.

Thanks, I'll fix that up (see below). Also, apologies for typo'ing your
email address when I sent this out on Friday.

Will

--->8

From 7e86208cd6541c1229bc1fcd206596308d1727f8 Mon Sep 17 00:00:00 2001
From: Will Deacon <will@kernel.org>
Date: Mon, 18 May 2020 14:01:01 +0100
Subject: [PATCH] arm64: entry-ftrace.S: Update comment to indicate that x18 is
 live

The Shadow Call Stack pointer is held in x18, so update the ftrace
entry comment to indicate that it cannot be safely clobbered.

Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry-ftrace.S | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index 833d48c9acb5..a338f40e64d3 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -23,8 +23,9 @@
  *
  * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
  *
- * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
- * live, and x9-x18 are safe to clobber.
+ * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are
+ * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to
+ * clobber.
  *
  * We save the callsite's context into a pt_regs before invoking any ftrace
  * callbacks. So that we can get a sensible backtrace, we create a stack record
-- 
2.26.2.761.g0e0b3e54be-goog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Mark Rutland]

On Mon, May 18, 2020 at 02:03:36PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 12:55:47PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> > > x18 holds the SCS stack pointer value, so introduce a register alias to
> > > make this easier to read in assembly code.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > I scanned through arm64 for all instances of x18, and it looks like
> > you've covered all the relevant uses here. In kvm we save/restore x18 a
> > bunch becasue it might be a platform register, but we do that
> > unconditionally and without knowledge of what it contains, so I think
> > that's fine to leave as-is. Therefore:
> > 
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > As an aside, the comment in entry-ftrace.S is now stale where it says
> > that x18 is safe to clobber. I can send a patch to clean that up, unless
> > you want to do that yourself.
> 
> Thanks, I'll fix that up (see below). Also, apologies for typo'ing your
> email address when I sent this out on Friday.

No worries; I'd already forgotten!

> 
> Will
> 
> --->8
> 
> From 7e86208cd6541c1229bc1fcd206596308d1727f8 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Mon, 18 May 2020 14:01:01 +0100
> Subject: [PATCH] arm64: entry-ftrace.S: Update comment to indicate that x18 is
>  live
> 
> The Shadow Call Stack pointer is held in x18, so update the ftrace
> entry comment to indicate that it cannot be safely clobbered.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/entry-ftrace.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index 833d48c9acb5..a338f40e64d3 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -23,8 +23,9 @@
>   *
>   * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
>   *
> - * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
> - * live, and x9-x18 are safe to clobber.
> + * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are
> + * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to
> + * clobber.

I'd have called out x18 a bit more specifically:

| Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
| live, x18 is potentially live with a shadow call stack pointer, and
| x9-x17 are safe to clobber.

... but either way this looks good; thanks!

Mark.

Re: [PATCH 3/6] arm64: scs: Use 'scs_sp' register alias for x18

[Mark Rutland]

On Mon, May 18, 2020 at 02:03:36PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 12:55:47PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:53PM +0100, Will Deacon wrote:
> > > x18 holds the SCS stack pointer value, so introduce a register alias to
> > > make this easier to read in assembly code.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > I scanned through arm64 for all instances of x18, and it looks like
> > you've covered all the relevant uses here. In kvm we save/restore x18 a
> > bunch becasue it might be a platform register, but we do that
> > unconditionally and without knowledge of what it contains, so I think
> > that's fine to leave as-is. Therefore:
> > 
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > As an aside, the comment in entry-ftrace.S is now stale where it says
> > that x18 is safe to clobber. I can send a patch to clean that up, unless
> > you want to do that yourself.
> 
> Thanks, I'll fix that up (see below). Also, apologies for typo'ing your
> email address when I sent this out on Friday.

No worries; I'd already forgotten!

> 
> Will
> 
> --->8
> 
> From 7e86208cd6541c1229bc1fcd206596308d1727f8 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Mon, 18 May 2020 14:01:01 +0100
> Subject: [PATCH] arm64: entry-ftrace.S: Update comment to indicate that x18 is
>  live
> 
> The Shadow Call Stack pointer is held in x18, so update the ftrace
> entry comment to indicate that it cannot be safely clobbered.
> 
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/entry-ftrace.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index 833d48c9acb5..a338f40e64d3 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -23,8 +23,9 @@
>   *
>   * ... where <entry> is either ftrace_caller or ftrace_regs_caller.
>   *
> - * Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
> - * live, and x9-x18 are safe to clobber.
> + * Each instrumented function follows the AAPCS, so here x0-x8 and x18-x30 are
> + * live (x18 holds the Shadow Call Stack pointer), and x9-x17 are safe to
> + * clobber.

I'd have called out x18 a bit more specifically:

| Each instrumented function follows the AAPCS, so here x0-x8 and x19-x30 are
| live, x18 is potentially live with a shadow call stack pointer, and
| x9-x17 are safe to clobber.

... but either way this looks good; thanks!

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

There is nothing architecture-specific about scs_overflow_check() as
it's just a trivial wrapper around scs_corrupted().

For parity with task_stack_end_corrupted(), rename scs_corrupted() to
task_scs_end_corrupted() and call it from schedule_debug() when
CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
scs_overflow_check() function entirely.

This has absolutely no impact on architectures that do not support SCS
(currently arm64 only).

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h | 18 ------------------
 arch/arm64/kernel/process.c  |  2 --
 arch/arm64/kernel/scs.c      |  2 +-
 include/linux/scs.h          |  4 ++--
 kernel/sched/core.c          |  3 +++
 kernel/scs.c                 |  3 ++-
 6 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index d46efdd2060a..eaa2cd92e4c1 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -24,24 +24,6 @@
 	.endm
 #endif /* CONFIG_SHADOW_CALL_STACK */
 
-#else /* __ASSEMBLY__ */
-
-#include <linux/scs.h>
-
-#ifdef CONFIG_SHADOW_CALL_STACK
-
-static inline void scs_overflow_check(struct task_struct *tsk)
-{
-	if (unlikely(scs_corrupted(tsk)))
-		panic("corrupted shadow stack detected inside scheduler\n");
-}
-
-#else /* CONFIG_SHADOW_CALL_STACK */
-
-static inline void scs_overflow_check(struct task_struct *tsk) {}
-
-#endif /* CONFIG_SHADOW_CALL_STACK */
-
 #endif /* __ASSEMBLY __ */
 
 #endif /* _ASM_SCS_H */
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index a35d3318492c..56be4cbf771f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -52,7 +52,6 @@
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/pointer_auth.h>
-#include <asm/scs.h>
 #include <asm/stacktrace.h>
 
 #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
 	entry_task_switch(next);
 	uao_thread_switch(next);
 	ssbs_thread_switch(next);
-	scs_overflow_check(next);
 
 	/*
 	 * Complete any pending TLB or cache maintenance on this CPU in case
diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
index adc97f826fab..955875dff9e1 100644
--- a/arch/arm64/kernel/scs.c
+++ b/arch/arm64/kernel/scs.c
@@ -6,7 +6,7 @@
  */
 
 #include <linux/percpu.h>
-#include <asm/scs.h>
+#include <linux/scs.h>
 
 /* Allocate a static per-CPU shadow stack */
 #define DEFINE_SCS(name)						\
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 0eb2485ef832..2fd3df50e93e 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)
 	return (unsigned long *)(s + SCS_SIZE) - 1;
 }
 
-static inline bool scs_corrupted(struct task_struct *tsk)
+static inline bool task_scs_end_corrupted(struct task_struct *tsk)
 {
 	unsigned long *magic = __scs_magic(task_scs(tsk));
 	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
@@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)
 static inline void scs_init(void) {}
 static inline void scs_task_reset(struct task_struct *tsk) {}
 static inline int scs_prepare(struct task_struct *tsk, int node) { return 0; }
-static inline bool scs_corrupted(struct task_struct *tsk) { return false; }
 static inline void scs_release(struct task_struct *tsk) {}
+static inline bool task_scs_end_corrupted(struct task_struct *tsk) { return false; }
 
 #endif /* CONFIG_SHADOW_CALL_STACK */
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 934e03cfaec7..a1d815a11b90 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
 #ifdef CONFIG_SCHED_STACK_END_CHECK
 	if (task_stack_end_corrupted(prev))
 		panic("corrupted stack end detected inside scheduler\n");
+
+	if (task_scs_end_corrupted(prev))
+		panic("corrupted shadow stack detected inside scheduler\n");
 #endif
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
diff --git a/kernel/scs.c b/kernel/scs.c
index aea841cd7586..faf0ecd7b893 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)
 	if (!s)
 		return;
 
-	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
+	WARN(task_scs_end_corrupted(tsk),
+	     "corrupted shadow stack detected when freeing task\n");
 	scs_check_usage(tsk);
 	scs_free(s);
 }
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

There is nothing architecture-specific about scs_overflow_check() as
it's just a trivial wrapper around scs_corrupted().

For parity with task_stack_end_corrupted(), rename scs_corrupted() to
task_scs_end_corrupted() and call it from schedule_debug() when
CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
scs_overflow_check() function entirely.

This has absolutely no impact on architectures that do not support SCS
(currently arm64 only).

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/scs.h | 18 ------------------
 arch/arm64/kernel/process.c  |  2 --
 arch/arm64/kernel/scs.c      |  2 +-
 include/linux/scs.h          |  4 ++--
 kernel/sched/core.c          |  3 +++
 kernel/scs.c                 |  3 ++-
 6 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index d46efdd2060a..eaa2cd92e4c1 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -24,24 +24,6 @@
 	.endm
 #endif /* CONFIG_SHADOW_CALL_STACK */
 
-#else /* __ASSEMBLY__ */
-
-#include <linux/scs.h>
-
-#ifdef CONFIG_SHADOW_CALL_STACK
-
-static inline void scs_overflow_check(struct task_struct *tsk)
-{
-	if (unlikely(scs_corrupted(tsk)))
-		panic("corrupted shadow stack detected inside scheduler\n");
-}
-
-#else /* CONFIG_SHADOW_CALL_STACK */
-
-static inline void scs_overflow_check(struct task_struct *tsk) {}
-
-#endif /* CONFIG_SHADOW_CALL_STACK */
-
 #endif /* __ASSEMBLY __ */
 
 #endif /* _ASM_SCS_H */
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index a35d3318492c..56be4cbf771f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -52,7 +52,6 @@
 #include <asm/mmu_context.h>
 #include <asm/processor.h>
 #include <asm/pointer_auth.h>
-#include <asm/scs.h>
 #include <asm/stacktrace.h>
 
 #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
@@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
 	entry_task_switch(next);
 	uao_thread_switch(next);
 	ssbs_thread_switch(next);
-	scs_overflow_check(next);
 
 	/*
 	 * Complete any pending TLB or cache maintenance on this CPU in case
diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
index adc97f826fab..955875dff9e1 100644
--- a/arch/arm64/kernel/scs.c
+++ b/arch/arm64/kernel/scs.c
@@ -6,7 +6,7 @@
  */
 
 #include <linux/percpu.h>
-#include <asm/scs.h>
+#include <linux/scs.h>
 
 /* Allocate a static per-CPU shadow stack */
 #define DEFINE_SCS(name)						\
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 0eb2485ef832..2fd3df50e93e 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)
 	return (unsigned long *)(s + SCS_SIZE) - 1;
 }
 
-static inline bool scs_corrupted(struct task_struct *tsk)
+static inline bool task_scs_end_corrupted(struct task_struct *tsk)
 {
 	unsigned long *magic = __scs_magic(task_scs(tsk));
 	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
@@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)
 static inline void scs_init(void) {}
 static inline void scs_task_reset(struct task_struct *tsk) {}
 static inline int scs_prepare(struct task_struct *tsk, int node) { return 0; }
-static inline bool scs_corrupted(struct task_struct *tsk) { return false; }
 static inline void scs_release(struct task_struct *tsk) {}
+static inline bool task_scs_end_corrupted(struct task_struct *tsk) { return false; }
 
 #endif /* CONFIG_SHADOW_CALL_STACK */
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 934e03cfaec7..a1d815a11b90 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
 #ifdef CONFIG_SCHED_STACK_END_CHECK
 	if (task_stack_end_corrupted(prev))
 		panic("corrupted stack end detected inside scheduler\n");
+
+	if (task_scs_end_corrupted(prev))
+		panic("corrupted shadow stack detected inside scheduler\n");
 #endif
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
diff --git a/kernel/scs.c b/kernel/scs.c
index aea841cd7586..faf0ecd7b893 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)
 	if (!s)
 		return;
 
-	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
+	WARN(task_scs_end_corrupted(tsk),
+	     "corrupted shadow stack detected when freeing task\n");
 	scs_check_usage(tsk);
 	scs_free(s);
 }
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> There is nothing architecture-specific about scs_overflow_check() as
> it's just a trivial wrapper around scs_corrupted().
> 
> For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> task_scs_end_corrupted() and call it from schedule_debug() when
> CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> scs_overflow_check() function entirely.
> 
> This has absolutely no impact on architectures that do not support SCS
> (currently arm64 only).
> 
> Signed-off-by: Will Deacon <will@kernel.org>

Pulling this out of arch code seems sane to me, and the arch-specific
chanes look sound. However, I have a concern with the changes within the
scheduler context-switch.

> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index a35d3318492c..56be4cbf771f 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -52,7 +52,6 @@
>  #include <asm/mmu_context.h>
>  #include <asm/processor.h>
>  #include <asm/pointer_auth.h>
> -#include <asm/scs.h>
>  #include <asm/stacktrace.h>
>  
>  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
>  	entry_task_switch(next);
>  	uao_thread_switch(next);
>  	ssbs_thread_switch(next);
> -	scs_overflow_check(next);

Prior to this patch, we'd never switch to a task whose SCS had already
been corrupted.

With this patch, we only check that when switching away from a task, and
only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
glance seems to weaken that.

Arguably:

* If the next task's SCS was corrupted by that task while it was
  running, we had already lost at that point.

* If the next task's SCS was corrupted by another task, then that could
  also happen immediately after the check (though timing to avoid the
  check but affect the process could be harder).

... and a VMAP'd SCS would be much nicer in this regard.

Do we think this is weakening the check, or do we think it wasn't all
that helpful to begin with?

Mark.

>  
>  	/*
>  	 * Complete any pending TLB or cache maintenance on this CPU in case
> diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
> index adc97f826fab..955875dff9e1 100644
> --- a/arch/arm64/kernel/scs.c
> +++ b/arch/arm64/kernel/scs.c
> @@ -6,7 +6,7 @@
>   */
>  
>  #include <linux/percpu.h>
> -#include <asm/scs.h>
> +#include <linux/scs.h>
>  
>  /* Allocate a static per-CPU shadow stack */
>  #define DEFINE_SCS(name)						\
> diff --git a/include/linux/scs.h b/include/linux/scs.h
> index 0eb2485ef832..2fd3df50e93e 100644
> --- a/include/linux/scs.h
> +++ b/include/linux/scs.h
> @@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)
>  	return (unsigned long *)(s + SCS_SIZE) - 1;
>  }
>  
> -static inline bool scs_corrupted(struct task_struct *tsk)
> +static inline bool task_scs_end_corrupted(struct task_struct *tsk)
>  {
>  	unsigned long *magic = __scs_magic(task_scs(tsk));
>  	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
> @@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)
>  static inline void scs_init(void) {}
>  static inline void scs_task_reset(struct task_struct *tsk) {}
>  static inline int scs_prepare(struct task_struct *tsk, int node) { return 0; }
> -static inline bool scs_corrupted(struct task_struct *tsk) { return false; }
>  static inline void scs_release(struct task_struct *tsk) {}
> +static inline bool task_scs_end_corrupted(struct task_struct *tsk) { return false; }
>  
>  #endif /* CONFIG_SHADOW_CALL_STACK */
>  
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 934e03cfaec7..a1d815a11b90 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
>  #ifdef CONFIG_SCHED_STACK_END_CHECK
>  	if (task_stack_end_corrupted(prev))
>  		panic("corrupted stack end detected inside scheduler\n");
> +
> +	if (task_scs_end_corrupted(prev))
> +		panic("corrupted shadow stack detected inside scheduler\n");
>  #endif
>  
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> diff --git a/kernel/scs.c b/kernel/scs.c
> index aea841cd7586..faf0ecd7b893 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)
>  	if (!s)
>  		return;
>  
> -	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
> +	WARN(task_scs_end_corrupted(tsk),
> +	     "corrupted shadow stack detected when freeing task\n");
>  	scs_check_usage(tsk);
>  	scs_free(s);
>  }
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> There is nothing architecture-specific about scs_overflow_check() as
> it's just a trivial wrapper around scs_corrupted().
> 
> For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> task_scs_end_corrupted() and call it from schedule_debug() when
> CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> scs_overflow_check() function entirely.
> 
> This has absolutely no impact on architectures that do not support SCS
> (currently arm64 only).
> 
> Signed-off-by: Will Deacon <will@kernel.org>

Pulling this out of arch code seems sane to me, and the arch-specific
chanes look sound. However, I have a concern with the changes within the
scheduler context-switch.

> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index a35d3318492c..56be4cbf771f 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -52,7 +52,6 @@
>  #include <asm/mmu_context.h>
>  #include <asm/processor.h>
>  #include <asm/pointer_auth.h>
> -#include <asm/scs.h>
>  #include <asm/stacktrace.h>
>  
>  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
>  	entry_task_switch(next);
>  	uao_thread_switch(next);
>  	ssbs_thread_switch(next);
> -	scs_overflow_check(next);

Prior to this patch, we'd never switch to a task whose SCS had already
been corrupted.

With this patch, we only check that when switching away from a task, and
only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
glance seems to weaken that.

Arguably:

* If the next task's SCS was corrupted by that task while it was
  running, we had already lost at that point.

* If the next task's SCS was corrupted by another task, then that could
  also happen immediately after the check (though timing to avoid the
  check but affect the process could be harder).

... and a VMAP'd SCS would be much nicer in this regard.

Do we think this is weakening the check, or do we think it wasn't all
that helpful to begin with?

Mark.

>  
>  	/*
>  	 * Complete any pending TLB or cache maintenance on this CPU in case
> diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
> index adc97f826fab..955875dff9e1 100644
> --- a/arch/arm64/kernel/scs.c
> +++ b/arch/arm64/kernel/scs.c
> @@ -6,7 +6,7 @@
>   */
>  
>  #include <linux/percpu.h>
> -#include <asm/scs.h>
> +#include <linux/scs.h>
>  
>  /* Allocate a static per-CPU shadow stack */
>  #define DEFINE_SCS(name)						\
> diff --git a/include/linux/scs.h b/include/linux/scs.h
> index 0eb2485ef832..2fd3df50e93e 100644
> --- a/include/linux/scs.h
> +++ b/include/linux/scs.h
> @@ -47,7 +47,7 @@ static inline unsigned long *__scs_magic(void *s)
>  	return (unsigned long *)(s + SCS_SIZE) - 1;
>  }
>  
> -static inline bool scs_corrupted(struct task_struct *tsk)
> +static inline bool task_scs_end_corrupted(struct task_struct *tsk)
>  {
>  	unsigned long *magic = __scs_magic(task_scs(tsk));
>  	unsigned long sz = task_scs_sp(tsk) - task_scs(tsk);
> @@ -60,8 +60,8 @@ static inline bool scs_corrupted(struct task_struct *tsk)
>  static inline void scs_init(void) {}
>  static inline void scs_task_reset(struct task_struct *tsk) {}
>  static inline int scs_prepare(struct task_struct *tsk, int node) { return 0; }
> -static inline bool scs_corrupted(struct task_struct *tsk) { return false; }
>  static inline void scs_release(struct task_struct *tsk) {}
> +static inline bool task_scs_end_corrupted(struct task_struct *tsk) { return false; }
>  
>  #endif /* CONFIG_SHADOW_CALL_STACK */
>  
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 934e03cfaec7..a1d815a11b90 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3878,6 +3878,9 @@ static inline void schedule_debug(struct task_struct *prev, bool preempt)
>  #ifdef CONFIG_SCHED_STACK_END_CHECK
>  	if (task_stack_end_corrupted(prev))
>  		panic("corrupted stack end detected inside scheduler\n");
> +
> +	if (task_scs_end_corrupted(prev))
> +		panic("corrupted shadow stack detected inside scheduler\n");
>  #endif
>  
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
> diff --git a/kernel/scs.c b/kernel/scs.c
> index aea841cd7586..faf0ecd7b893 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -98,7 +98,8 @@ void scs_release(struct task_struct *tsk)
>  	if (!s)
>  		return;
>  
> -	WARN(scs_corrupted(tsk), "corrupted shadow stack detected when freeing task\n");
> +	WARN(task_scs_end_corrupted(tsk),
> +	     "corrupted shadow stack detected when freeing task\n");
>  	scs_check_usage(tsk);
>  	scs_free(s);
>  }
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > There is nothing architecture-specific about scs_overflow_check() as
> > it's just a trivial wrapper around scs_corrupted().
> > 
> > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > task_scs_end_corrupted() and call it from schedule_debug() when
> > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > scs_overflow_check() function entirely.
> > 
> > This has absolutely no impact on architectures that do not support SCS
> > (currently arm64 only).
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> Pulling this out of arch code seems sane to me, and the arch-specific
> chanes look sound. However, I have a concern with the changes within the
> scheduler context-switch.
> 
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index a35d3318492c..56be4cbf771f 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -52,7 +52,6 @@
> >  #include <asm/mmu_context.h>
> >  #include <asm/processor.h>
> >  #include <asm/pointer_auth.h>
> > -#include <asm/scs.h>
> >  #include <asm/stacktrace.h>
> >  
> >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> >  	entry_task_switch(next);
> >  	uao_thread_switch(next);
> >  	ssbs_thread_switch(next);
> > -	scs_overflow_check(next);
> 
> Prior to this patch, we'd never switch to a task whose SCS had already
> been corrupted.
> 
> With this patch, we only check that when switching away from a task, and
> only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> glance seems to weaken that.

Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.

> Arguably:
> 
> * If the next task's SCS was corrupted by that task while it was
>   running, we had already lost at that point.

With this change, we'll at least catch this one sooner, and that might be
useful if a bug has caused us to overflow the SCS but not the main stack.

> * If the next task's SCS was corrupted by another task, then that could
>   also happen immediately after the check (though timing to avoid the
>   check but affect the process could be harder).

We're only checking the magic end value, so the cross-task case is basically
if you overrun your own SCS as above, but then continue to overrun entire
SCSs for other tasks as well. It's probably not very useful in that case.

> ... and a VMAP'd SCS would be much nicer in this regard.
> 
> Do we think this is weakening the check, or do we think it wasn't all
> that helpful to begin with?

I see it as a debug check to catch SCS overflow, rather than a hardening
feature, and I agree that using something like vmap stack for the SCS would
be better because we could have a guard page instead. This is something I
would like to revisit, but we need more information from Sami about why
Android rejected the larger allocation size, since I don't think there's an
awful lot of point merging this series if Android doesn't pick it up.

Will

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > There is nothing architecture-specific about scs_overflow_check() as
> > it's just a trivial wrapper around scs_corrupted().
> > 
> > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > task_scs_end_corrupted() and call it from schedule_debug() when
> > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > scs_overflow_check() function entirely.
> > 
> > This has absolutely no impact on architectures that do not support SCS
> > (currently arm64 only).
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> Pulling this out of arch code seems sane to me, and the arch-specific
> chanes look sound. However, I have a concern with the changes within the
> scheduler context-switch.
> 
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index a35d3318492c..56be4cbf771f 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -52,7 +52,6 @@
> >  #include <asm/mmu_context.h>
> >  #include <asm/processor.h>
> >  #include <asm/pointer_auth.h>
> > -#include <asm/scs.h>
> >  #include <asm/stacktrace.h>
> >  
> >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> >  	entry_task_switch(next);
> >  	uao_thread_switch(next);
> >  	ssbs_thread_switch(next);
> > -	scs_overflow_check(next);
> 
> Prior to this patch, we'd never switch to a task whose SCS had already
> been corrupted.
> 
> With this patch, we only check that when switching away from a task, and
> only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> glance seems to weaken that.

Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.

> Arguably:
> 
> * If the next task's SCS was corrupted by that task while it was
>   running, we had already lost at that point.

With this change, we'll at least catch this one sooner, and that might be
useful if a bug has caused us to overflow the SCS but not the main stack.

> * If the next task's SCS was corrupted by another task, then that could
>   also happen immediately after the check (though timing to avoid the
>   check but affect the process could be harder).

We're only checking the magic end value, so the cross-task case is basically
if you overrun your own SCS as above, but then continue to overrun entire
SCSs for other tasks as well. It's probably not very useful in that case.

> ... and a VMAP'd SCS would be much nicer in this regard.
> 
> Do we think this is weakening the check, or do we think it wasn't all
> that helpful to begin with?

I see it as a debug check to catch SCS overflow, rather than a hardening
feature, and I agree that using something like vmap stack for the SCS would
be better because we could have a guard page instead. This is something I
would like to revisit, but we need more information from Sami about why
Android rejected the larger allocation size, since I don't think there's an
awful lot of point merging this series if Android doesn't pick it up.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Mark Rutland]

On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > > There is nothing architecture-specific about scs_overflow_check() as
> > > it's just a trivial wrapper around scs_corrupted().
> > > 
> > > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > > task_scs_end_corrupted() and call it from schedule_debug() when
> > > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > > scs_overflow_check() function entirely.
> > > 
> > > This has absolutely no impact on architectures that do not support SCS
> > > (currently arm64 only).
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > Pulling this out of arch code seems sane to me, and the arch-specific
> > chanes look sound. However, I have a concern with the changes within the
> > scheduler context-switch.
> > 
> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index a35d3318492c..56be4cbf771f 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -52,7 +52,6 @@
> > >  #include <asm/mmu_context.h>
> > >  #include <asm/processor.h>
> > >  #include <asm/pointer_auth.h>
> > > -#include <asm/scs.h>
> > >  #include <asm/stacktrace.h>
> > >  
> > >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> > >  	entry_task_switch(next);
> > >  	uao_thread_switch(next);
> > >  	ssbs_thread_switch(next);
> > > -	scs_overflow_check(next);
> > 
> > Prior to this patch, we'd never switch to a task whose SCS had already
> > been corrupted.
> > 
> > With this patch, we only check that when switching away from a task, and
> > only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> > glance seems to weaken that.
> 
> Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
> the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.
> 
> > Arguably:
> > 
> > * If the next task's SCS was corrupted by that task while it was
> >   running, we had already lost at that point.
> 
> With this change, we'll at least catch this one sooner, and that might be
> useful if a bug has caused us to overflow the SCS but not the main stack.

Sure, but only if CONFIG_SCHED_STACK_END_CHECK is selected.

> > * If the next task's SCS was corrupted by another task, then that could
> >   also happen immediately after the check (though timing to avoid the
> >   check but affect the process could be harder).
> 
> We're only checking the magic end value, so the cross-task case is basically
> if you overrun your own SCS as above, but then continue to overrun entire
> SCSs for other tasks as well. It's probably not very useful in that case.
> 
> > ... and a VMAP'd SCS would be much nicer in this regard.
> > 
> > Do we think this is weakening the check, or do we think it wasn't all
> > that helpful to begin with?
> 
> I see it as a debug check to catch SCS overflow, rather than a hardening
> feature, and I agree that using something like vmap stack for the SCS would
> be better because we could have a guard page instead.

Fair enough. Could we put something into the commit message that more
explicitly calls out debug-not-hardening? I agree that under that model
this patch looks fine, and with something to that effect:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> This is something I would like to revisit, but we need more
> information from Sami about why Android rejected the larger allocation
> size, since I don't think there's an awful lot of point merging this
> series if Android doesn't pick it up.

Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
that.

Mark.

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Mark Rutland]

On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > > There is nothing architecture-specific about scs_overflow_check() as
> > > it's just a trivial wrapper around scs_corrupted().
> > > 
> > > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > > task_scs_end_corrupted() and call it from schedule_debug() when
> > > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > > scs_overflow_check() function entirely.
> > > 
> > > This has absolutely no impact on architectures that do not support SCS
> > > (currently arm64 only).
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > Pulling this out of arch code seems sane to me, and the arch-specific
> > chanes look sound. However, I have a concern with the changes within the
> > scheduler context-switch.
> > 
> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index a35d3318492c..56be4cbf771f 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -52,7 +52,6 @@
> > >  #include <asm/mmu_context.h>
> > >  #include <asm/processor.h>
> > >  #include <asm/pointer_auth.h>
> > > -#include <asm/scs.h>
> > >  #include <asm/stacktrace.h>
> > >  
> > >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> > >  	entry_task_switch(next);
> > >  	uao_thread_switch(next);
> > >  	ssbs_thread_switch(next);
> > > -	scs_overflow_check(next);
> > 
> > Prior to this patch, we'd never switch to a task whose SCS had already
> > been corrupted.
> > 
> > With this patch, we only check that when switching away from a task, and
> > only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> > glance seems to weaken that.
> 
> Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
> the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.
> 
> > Arguably:
> > 
> > * If the next task's SCS was corrupted by that task while it was
> >   running, we had already lost at that point.
> 
> With this change, we'll at least catch this one sooner, and that might be
> useful if a bug has caused us to overflow the SCS but not the main stack.

Sure, but only if CONFIG_SCHED_STACK_END_CHECK is selected.

> > * If the next task's SCS was corrupted by another task, then that could
> >   also happen immediately after the check (though timing to avoid the
> >   check but affect the process could be harder).
> 
> We're only checking the magic end value, so the cross-task case is basically
> if you overrun your own SCS as above, but then continue to overrun entire
> SCSs for other tasks as well. It's probably not very useful in that case.
> 
> > ... and a VMAP'd SCS would be much nicer in this regard.
> > 
> > Do we think this is weakening the check, or do we think it wasn't all
> > that helpful to begin with?
> 
> I see it as a debug check to catch SCS overflow, rather than a hardening
> feature, and I agree that using something like vmap stack for the SCS would
> be better because we could have a guard page instead.

Fair enough. Could we put something into the commit message that more
explicitly calls out debug-not-hardening? I agree that under that model
this patch looks fine, and with something to that effect:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> This is something I would like to revisit, but we need more
> information from Sami about why Android rejected the larger allocation
> size, since I don't think there's an awful lot of point merging this
> series if Android doesn't pick it up.

Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
that.

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Kees Cook]

On Mon, May 18, 2020 at 02:32:31PM +0100, Mark Rutland wrote:
> On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> > On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> > > On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > > > There is nothing architecture-specific about scs_overflow_check() as
> > > > it's just a trivial wrapper around scs_corrupted().
> > > > 
> > > > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > > > task_scs_end_corrupted() and call it from schedule_debug() when
> > > > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > > > scs_overflow_check() function entirely.
> > > > 
> > > > This has absolutely no impact on architectures that do not support SCS
> > > > (currently arm64 only).
> > > > 
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > 
> > > Pulling this out of arch code seems sane to me, and the arch-specific
> > > chanes look sound. However, I have a concern with the changes within the
> > > scheduler context-switch.
> > > 
> > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > > index a35d3318492c..56be4cbf771f 100644
> > > > --- a/arch/arm64/kernel/process.c
> > > > +++ b/arch/arm64/kernel/process.c
> > > > @@ -52,7 +52,6 @@
> > > >  #include <asm/mmu_context.h>
> > > >  #include <asm/processor.h>
> > > >  #include <asm/pointer_auth.h>
> > > > -#include <asm/scs.h>
> > > >  #include <asm/stacktrace.h>
> > > >  
> > > >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > > > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> > > >  	entry_task_switch(next);
> > > >  	uao_thread_switch(next);
> > > >  	ssbs_thread_switch(next);
> > > > -	scs_overflow_check(next);
> > > 
> > > Prior to this patch, we'd never switch to a task whose SCS had already
> > > been corrupted.
> > > 
> > > With this patch, we only check that when switching away from a task, and
> > > only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> > > glance seems to weaken that.
> > 
> > Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
> > the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.
> > 
> > > Arguably:
> > > 
> > > * If the next task's SCS was corrupted by that task while it was
> > >   running, we had already lost at that point.
> > 
> > With this change, we'll at least catch this one sooner, and that might be
> > useful if a bug has caused us to overflow the SCS but not the main stack.
> 
> Sure, but only if CONFIG_SCHED_STACK_END_CHECK is selected.
> 
> > > * If the next task's SCS was corrupted by another task, then that could
> > >   also happen immediately after the check (though timing to avoid the
> > >   check but affect the process could be harder).
> > 
> > We're only checking the magic end value, so the cross-task case is basically
> > if you overrun your own SCS as above, but then continue to overrun entire
> > SCSs for other tasks as well. It's probably not very useful in that case.
> > 
> > > ... and a VMAP'd SCS would be much nicer in this regard.
> > > 
> > > Do we think this is weakening the check, or do we think it wasn't all
> > > that helpful to begin with?
> > 
> > I see it as a debug check to catch SCS overflow, rather than a hardening
> > feature, and I agree that using something like vmap stack for the SCS would
> > be better because we could have a guard page instead.
> 
> Fair enough. Could we put something into the commit message that more
> explicitly calls out debug-not-hardening? I agree that under that model
> this patch looks fine, and with something to that effect:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Mark.
> 
> > This is something I would like to revisit, but we need more
> > information from Sami about why Android rejected the larger allocation
> > size, since I don't think there's an awful lot of point merging this
> > series if Android doesn't pick it up.
> 
> Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
> that.

For smaller devices, the memory overhead was too high. (i.e. 4x more
memory allocated to kernel stacks -- 4k vs 1k per thread.) The series
is much more than just a stack exhaustion defense, so I don't think that
detail needs to block the entire series. FWIW, I'd like to have both modes
(contiguous and vmap) available so that system builders can choose their
trade-off. Both will gain return address corruption defense, but the
vmap case will protect against neighboring SCS corruption in the face
of very-unlikely-but-technically-possible stack exhaustion (remember
that with the elimination of VLAs, the stack depth compile time
checking, and the regular stack VMAP guard page, it will be quite
difficult to exhaust the SCS -- either because there is no code path to
accomplish it, or because it would trip the regular stack guard page
first).

-- 
Kees Cook

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Kees Cook]

On Mon, May 18, 2020 at 02:32:31PM +0100, Mark Rutland wrote:
> On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> > On Mon, May 18, 2020 at 01:12:10PM +0100, Mark Rutland wrote:
> > > On Fri, May 15, 2020 at 06:27:54PM +0100, Will Deacon wrote:
> > > > There is nothing architecture-specific about scs_overflow_check() as
> > > > it's just a trivial wrapper around scs_corrupted().
> > > > 
> > > > For parity with task_stack_end_corrupted(), rename scs_corrupted() to
> > > > task_scs_end_corrupted() and call it from schedule_debug() when
> > > > CONFIG_SCHED_STACK_END_CHECK_is enabled. Finally, remove the unused
> > > > scs_overflow_check() function entirely.
> > > > 
> > > > This has absolutely no impact on architectures that do not support SCS
> > > > (currently arm64 only).
> > > > 
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > 
> > > Pulling this out of arch code seems sane to me, and the arch-specific
> > > chanes look sound. However, I have a concern with the changes within the
> > > scheduler context-switch.
> > > 
> > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > > index a35d3318492c..56be4cbf771f 100644
> > > > --- a/arch/arm64/kernel/process.c
> > > > +++ b/arch/arm64/kernel/process.c
> > > > @@ -52,7 +52,6 @@
> > > >  #include <asm/mmu_context.h>
> > > >  #include <asm/processor.h>
> > > >  #include <asm/pointer_auth.h>
> > > > -#include <asm/scs.h>
> > > >  #include <asm/stacktrace.h>
> > > >  
> > > >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> > > > @@ -516,7 +515,6 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
> > > >  	entry_task_switch(next);
> > > >  	uao_thread_switch(next);
> > > >  	ssbs_thread_switch(next);
> > > > -	scs_overflow_check(next);
> > > 
> > > Prior to this patch, we'd never switch to a task whose SCS had already
> > > been corrupted.
> > > 
> > > With this patch, we only check that when switching away from a task, and
> > > only when CONFIG_SCHED_STACK_END_CHECK is selected, which at first
> > > glance seems to weaken that.
> > 
> > Yes, ignoring vmap'd stacks, this patch brings the SCS checking in-line with
> > the main stack checking when CONFIG_SCHED_STACK_END_CHECK=y.
> > 
> > > Arguably:
> > > 
> > > * If the next task's SCS was corrupted by that task while it was
> > >   running, we had already lost at that point.
> > 
> > With this change, we'll at least catch this one sooner, and that might be
> > useful if a bug has caused us to overflow the SCS but not the main stack.
> 
> Sure, but only if CONFIG_SCHED_STACK_END_CHECK is selected.
> 
> > > * If the next task's SCS was corrupted by another task, then that could
> > >   also happen immediately after the check (though timing to avoid the
> > >   check but affect the process could be harder).
> > 
> > We're only checking the magic end value, so the cross-task case is basically
> > if you overrun your own SCS as above, but then continue to overrun entire
> > SCSs for other tasks as well. It's probably not very useful in that case.
> > 
> > > ... and a VMAP'd SCS would be much nicer in this regard.
> > > 
> > > Do we think this is weakening the check, or do we think it wasn't all
> > > that helpful to begin with?
> > 
> > I see it as a debug check to catch SCS overflow, rather than a hardening
> > feature, and I agree that using something like vmap stack for the SCS would
> > be better because we could have a guard page instead.
> 
> Fair enough. Could we put something into the commit message that more
> explicitly calls out debug-not-hardening? I agree that under that model
> this patch looks fine, and with something to that effect:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> Mark.
> 
> > This is something I would like to revisit, but we need more
> > information from Sami about why Android rejected the larger allocation
> > size, since I don't think there's an awful lot of point merging this
> > series if Android doesn't pick it up.
> 
> Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
> that.

For smaller devices, the memory overhead was too high. (i.e. 4x more
memory allocated to kernel stacks -- 4k vs 1k per thread.) The series
is much more than just a stack exhaustion defense, so I don't think that
detail needs to block the entire series. FWIW, I'd like to have both modes
(contiguous and vmap) available so that system builders can choose their
trade-off. Both will gain return address corruption defense, but the
vmap case will protect against neighboring SCS corruption in the face
of very-unlikely-but-technically-possible stack exhaustion (remember
that with the elimination of VLAs, the stack depth compile time
checking, and the regular stack VMAP guard page, it will be quite
difficult to exhaust the SCS -- either because there is no code path to
accomplish it, or because it would trip the regular stack guard page
first).

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

On Mon, May 18, 2020 at 08:31:49AM -0700, Kees Cook wrote:
> On Mon, May 18, 2020 at 02:32:31PM +0100, Mark Rutland wrote:
> > On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> > > This is something I would like to revisit, but we need more
> > > information from Sami about why Android rejected the larger allocation
> > > size, since I don't think there's an awful lot of point merging this
> > > series if Android doesn't pick it up.
> > 
> > Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
> > that.
> 
> For smaller devices, the memory overhead was too high. (i.e. 4x more
> memory allocated to kernel stacks -- 4k vs 1k per thread.)

I just don't see an extra 3k per thread as being a real issue (the main
stack is 16k already). Even just the CPU register state is around 1k.

But I'd be very keen to see numbers/performance data that proves me wrong.

Will

Re: [PATCH 4/6] scs: Move scs_overflow_check() out of architecture code

[Will Deacon]

On Mon, May 18, 2020 at 08:31:49AM -0700, Kees Cook wrote:
> On Mon, May 18, 2020 at 02:32:31PM +0100, Mark Rutland wrote:
> > On Mon, May 18, 2020 at 02:23:47PM +0100, Will Deacon wrote:
> > > This is something I would like to revisit, but we need more
> > > information from Sami about why Android rejected the larger allocation
> > > size, since I don't think there's an awful lot of point merging this
> > > series if Android doesn't pick it up.
> > 
> > Indeed. I'd certainly prefer the robustness of a VMAP'd SCS if we can do
> > that.
> 
> For smaller devices, the memory overhead was too high. (i.e. 4x more
> memory allocated to kernel stacks -- 4k vs 1k per thread.)

I just don't see an extra 3k per thread as being a real issue (the main
stack is 16k already). Even just the CPU register state is around 1k.

But I'd be very keen to see numbers/performance data that proves me wrong.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] scs: Remove references to asm/scs.h from core code

[Will Deacon]

asm/scs.h is no longer needed by the core code, so remove a redundant
header inclusion and update the stale Kconfig text.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/Kconfig | 4 ++--
 kernel/scs.c | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 45dfca9a98d3..2e6f843d87c4 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK
 	bool
 	help
 	  An architecture should select this if it supports Clang's Shadow
-	  Call Stack, has asm/scs.h, and implements runtime support for shadow
-	  stack switching.
+	  Call Stack and implements runtime support for shadow stack
+	  switching.
 
 config SHADOW_CALL_STACK
 	bool "Clang Shadow Call Stack"
diff --git a/kernel/scs.c b/kernel/scs.c
index faf0ecd7b893..222a7a9ad543 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -10,7 +10,6 @@
 #include <linux/scs.h>
 #include <linux/slab.h>
 #include <linux/vmstat.h>
-#include <asm/scs.h>
 
 static struct kmem_cache *scs_cache;
 
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 5/6] scs: Remove references to asm/scs.h from core code

[Will Deacon]

asm/scs.h is no longer needed by the core code, so remove a redundant
header inclusion and update the stale Kconfig text.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/Kconfig | 4 ++--
 kernel/scs.c | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 45dfca9a98d3..2e6f843d87c4 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK
 	bool
 	help
 	  An architecture should select this if it supports Clang's Shadow
-	  Call Stack, has asm/scs.h, and implements runtime support for shadow
-	  stack switching.
+	  Call Stack and implements runtime support for shadow stack
+	  switching.
 
 config SHADOW_CALL_STACK
 	bool "Clang Shadow Call Stack"
diff --git a/kernel/scs.c b/kernel/scs.c
index faf0ecd7b893..222a7a9ad543 100644
--- a/kernel/scs.c
+++ b/kernel/scs.c
@@ -10,7 +10,6 @@
 #include <linux/scs.h>
 #include <linux/slab.h>
 #include <linux/vmstat.h>
-#include <asm/scs.h>
 
 static struct kmem_cache *scs_cache;
 
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] scs: Remove references to asm/scs.h from core code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:55PM +0100, Will Deacon wrote:
> asm/scs.h is no longer needed by the core code, so remove a redundant
> header inclusion and update the stale Kconfig text.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

With the corruption checks moved out of arch code this looks sound to
me, so modulo my comments on the prior patch, assuming we factor that
out:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/Kconfig | 4 ++--
>  kernel/scs.c | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 45dfca9a98d3..2e6f843d87c4 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK
>  	bool
>  	help
>  	  An architecture should select this if it supports Clang's Shadow
> -	  Call Stack, has asm/scs.h, and implements runtime support for shadow
> -	  stack switching.
> +	  Call Stack and implements runtime support for shadow stack
> +	  switching.
>  
>  config SHADOW_CALL_STACK
>  	bool "Clang Shadow Call Stack"
> diff --git a/kernel/scs.c b/kernel/scs.c
> index faf0ecd7b893..222a7a9ad543 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -10,7 +10,6 @@
>  #include <linux/scs.h>
>  #include <linux/slab.h>
>  #include <linux/vmstat.h>
> -#include <asm/scs.h>
>  
>  static struct kmem_cache *scs_cache;
>  
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 5/6] scs: Remove references to asm/scs.h from core code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:55PM +0100, Will Deacon wrote:
> asm/scs.h is no longer needed by the core code, so remove a redundant
> header inclusion and update the stale Kconfig text.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

With the corruption checks moved out of arch code this looks sound to
me, so modulo my comments on the prior patch, assuming we factor that
out:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/Kconfig | 4 ++--
>  kernel/scs.c | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 45dfca9a98d3..2e6f843d87c4 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -537,8 +537,8 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK
>  	bool
>  	help
>  	  An architecture should select this if it supports Clang's Shadow
> -	  Call Stack, has asm/scs.h, and implements runtime support for shadow
> -	  stack switching.
> +	  Call Stack and implements runtime support for shadow stack
> +	  switching.
>  
>  config SHADOW_CALL_STACK
>  	bool "Clang Shadow Call Stack"
> diff --git a/kernel/scs.c b/kernel/scs.c
> index faf0ecd7b893..222a7a9ad543 100644
> --- a/kernel/scs.c
> +++ b/kernel/scs.c
> @@ -10,7 +10,6 @@
>  #include <linux/scs.h>
>  #include <linux/slab.h>
>  #include <linux/vmstat.h>
> -#include <asm/scs.h>
>  
>  static struct kmem_cache *scs_cache;
>  
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Will Deacon]

Defining static shadow call stacks is not architecture-specific, so move
the DEFINE_SCS() macro into the core header file.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/scs.c | 4 ----
 include/linux/scs.h     | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
index 955875dff9e1..e8f7ff45dd8f 100644
--- a/arch/arm64/kernel/scs.c
+++ b/arch/arm64/kernel/scs.c
@@ -8,10 +8,6 @@
 #include <linux/percpu.h>
 #include <linux/scs.h>
 
-/* Allocate a static per-CPU shadow stack */
-#define DEFINE_SCS(name)						\
-	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
-
 DEFINE_SCS(irq_shadow_call_stack);
 
 #ifdef CONFIG_ARM_SDE_INTERFACE
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 2fd3df50e93e..6dec390cf154 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -26,6 +26,10 @@
 /* An illegal pointer value to mark the end of the shadow stack. */
 #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
 
+/* Allocate a static per-CPU shadow stack */
+#define DEFINE_SCS(name)						\
+	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
+
 #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
 #define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
 
-- 
2.26.2.761.g0e0b3e54be-goog

[PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Will Deacon]

Defining static shadow call stacks is not architecture-specific, so move
the DEFINE_SCS() macro into the core header file.

Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/scs.c | 4 ----
 include/linux/scs.h     | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
index 955875dff9e1..e8f7ff45dd8f 100644
--- a/arch/arm64/kernel/scs.c
+++ b/arch/arm64/kernel/scs.c
@@ -8,10 +8,6 @@
 #include <linux/percpu.h>
 #include <linux/scs.h>
 
-/* Allocate a static per-CPU shadow stack */
-#define DEFINE_SCS(name)						\
-	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
-
 DEFINE_SCS(irq_shadow_call_stack);
 
 #ifdef CONFIG_ARM_SDE_INTERFACE
diff --git a/include/linux/scs.h b/include/linux/scs.h
index 2fd3df50e93e..6dec390cf154 100644
--- a/include/linux/scs.h
+++ b/include/linux/scs.h
@@ -26,6 +26,10 @@
 /* An illegal pointer value to mark the end of the shadow stack. */
 #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
 
+/* Allocate a static per-CPU shadow stack */
+#define DEFINE_SCS(name)						\
+	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
+
 #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
 #define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
 
-- 
2.26.2.761.g0e0b3e54be-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> Defining static shadow call stacks is not architecture-specific, so move
> the DEFINE_SCS() macro into the core header file.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

I think that we'll have to pull this back into arch code if/when we deal
with VMAP'd stacks, so I'm not sure this is worthwhile given the
diffstat is balanced.

Mark.

> ---
>  arch/arm64/kernel/scs.c | 4 ----
>  include/linux/scs.h     | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
> index 955875dff9e1..e8f7ff45dd8f 100644
> --- a/arch/arm64/kernel/scs.c
> +++ b/arch/arm64/kernel/scs.c
> @@ -8,10 +8,6 @@
>  #include <linux/percpu.h>
>  #include <linux/scs.h>
>  
> -/* Allocate a static per-CPU shadow stack */
> -#define DEFINE_SCS(name)						\
> -	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
> -
>  DEFINE_SCS(irq_shadow_call_stack);
>  
>  #ifdef CONFIG_ARM_SDE_INTERFACE
> diff --git a/include/linux/scs.h b/include/linux/scs.h
> index 2fd3df50e93e..6dec390cf154 100644
> --- a/include/linux/scs.h
> +++ b/include/linux/scs.h
> @@ -26,6 +26,10 @@
>  /* An illegal pointer value to mark the end of the shadow stack. */
>  #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
>  
> +/* Allocate a static per-CPU shadow stack */
> +#define DEFINE_SCS(name)						\
> +	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
> +
>  #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
>  #define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
>  
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Mark Rutland]

On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> Defining static shadow call stacks is not architecture-specific, so move
> the DEFINE_SCS() macro into the core header file.
> 
> Signed-off-by: Will Deacon <will@kernel.org>

I think that we'll have to pull this back into arch code if/when we deal
with VMAP'd stacks, so I'm not sure this is worthwhile given the
diffstat is balanced.

Mark.

> ---
>  arch/arm64/kernel/scs.c | 4 ----
>  include/linux/scs.h     | 4 ++++
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/scs.c b/arch/arm64/kernel/scs.c
> index 955875dff9e1..e8f7ff45dd8f 100644
> --- a/arch/arm64/kernel/scs.c
> +++ b/arch/arm64/kernel/scs.c
> @@ -8,10 +8,6 @@
>  #include <linux/percpu.h>
>  #include <linux/scs.h>
>  
> -/* Allocate a static per-CPU shadow stack */
> -#define DEFINE_SCS(name)						\
> -	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
> -
>  DEFINE_SCS(irq_shadow_call_stack);
>  
>  #ifdef CONFIG_ARM_SDE_INTERFACE
> diff --git a/include/linux/scs.h b/include/linux/scs.h
> index 2fd3df50e93e..6dec390cf154 100644
> --- a/include/linux/scs.h
> +++ b/include/linux/scs.h
> @@ -26,6 +26,10 @@
>  /* An illegal pointer value to mark the end of the shadow stack. */
>  #define SCS_END_MAGIC		(0x5f6UL + POISON_POINTER_DELTA)
>  
> +/* Allocate a static per-CPU shadow stack */
> +#define DEFINE_SCS(name)						\
> +	DEFINE_PER_CPU(unsigned long [SCS_SIZE/sizeof(long)], name)	\
> +
>  #define task_scs(tsk)		(task_thread_info(tsk)->scs_base)
>  #define task_scs_sp(tsk)	(task_thread_info(tsk)->scs_sp)
>  
> -- 
> 2.26.2.761.g0e0b3e54be-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Will Deacon]

On Mon, May 18, 2020 at 01:14:41PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> > Defining static shadow call stacks is not architecture-specific, so move
> > the DEFINE_SCS() macro into the core header file.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> I think that we'll have to pull this back into arch code if/when we deal
> with VMAP'd stacks, so I'm not sure this is worthwhile given the
> diffstat is balanced.

I dunno, if another architecture wants to use this then having the stuff
in the core code makes sense to me. I also want to kill asm/scs.h entirely
and move our asm macros somewhere else where they're not mixed up with the
C headers.

Will

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Will Deacon]

On Mon, May 18, 2020 at 01:14:41PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> > Defining static shadow call stacks is not architecture-specific, so move
> > the DEFINE_SCS() macro into the core header file.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> I think that we'll have to pull this back into arch code if/when we deal
> with VMAP'd stacks, so I'm not sure this is worthwhile given the
> diffstat is balanced.

I dunno, if another architecture wants to use this then having the stuff
in the core code makes sense to me. I also want to kill asm/scs.h entirely
and move our asm macros somewhere else where they're not mixed up with the
C headers.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Mark Rutland]

On Mon, May 18, 2020 at 02:26:12PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 01:14:41PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> > > Defining static shadow call stacks is not architecture-specific, so move
> > > the DEFINE_SCS() macro into the core header file.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > I think that we'll have to pull this back into arch code if/when we deal
> > with VMAP'd stacks, so I'm not sure this is worthwhile given the
> > diffstat is balanced.
> 
> I dunno, if another architecture wants to use this then having the stuff
> in the core code makes sense to me. I also want to kill asm/scs.h entirely
> and move our asm macros somewhere else where they're not mixed up with the
> C headers.

Thinking about it a bit further, we'd have to make bigger changes anyhow
(to dynamically allocate), but given we can do that for regular stacks
we can probably do something similar here.

So no strong feelings either way on this patch.

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

Re: [PATCH 6/6] scs: Move DEFINE_SCS macro into core code

[Mark Rutland]

On Mon, May 18, 2020 at 02:26:12PM +0100, Will Deacon wrote:
> On Mon, May 18, 2020 at 01:14:41PM +0100, Mark Rutland wrote:
> > On Fri, May 15, 2020 at 06:27:56PM +0100, Will Deacon wrote:
> > > Defining static shadow call stacks is not architecture-specific, so move
> > > the DEFINE_SCS() macro into the core header file.
> > > 
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > 
> > I think that we'll have to pull this back into arch code if/when we deal
> > with VMAP'd stacks, so I'm not sure this is worthwhile given the
> > diffstat is balanced.
> 
> I dunno, if another architecture wants to use this then having the stuff
> in the core code makes sense to me. I also want to kill asm/scs.h entirely
> and move our asm macros somewhere else where they're not mixed up with the
> C headers.

Thinking about it a bit further, we'd have to make bigger changes anyhow
(to dynamically allocate), but given we can do that for regular stacks
we can probably do something similar here.

So no strong feelings either way on this patch.

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Sami Tolvanen]

On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
>
> Hi all,
>
> Here's a series of cleanups I hacked together on top of a modified v13
> of the Shadow Call Stack patches from Sami:
>
>   https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck
>
> The main changes are:
>
>   * Move code out of arch/arm64 and into the core implementation
>   * Store the full SCS stack pointer instead of the offset
>   * Code simplification and general style things
>
> I'd like to queue this on top early next week so that it can spend some
> quality time in linux-next.
>
> Cheers,
>
> Will
>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@am.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: <kernel-team@android.com>
>
> --->8
>
> Will Deacon (6):
>   arm64: scs: Store absolute SCS stack pointer value in thread_info
>   scs: Move accounting into alloc/free functions
>   arm64: scs: Use 'scs_sp' register alias for x18
>   scs: Move scs_overflow_check() out of architecture code
>   scs: Remove references to asm/scs.h from core code
>   scs: Move DEFINE_SCS macro into core code
>
>  arch/Kconfig                         |  4 +--
>  arch/arm64/include/asm/scs.h         | 29 ++++------------
>  arch/arm64/include/asm/thread_info.h |  4 +--
>  arch/arm64/kernel/asm-offsets.c      |  2 +-
>  arch/arm64/kernel/entry.S            | 10 +++---
>  arch/arm64/kernel/head.S             |  2 +-
>  arch/arm64/kernel/process.c          |  2 --
>  arch/arm64/kernel/scs.c              |  6 +---
>  include/linux/scs.h                  | 16 +++++----
>  kernel/sched/core.c                  |  3 ++
>  kernel/scs.c                         | 52 +++++++++++++---------------
>  11 files changed, 55 insertions(+), 75 deletions(-)
>
> --
> 2.26.2.761.g0e0b3e54be-goog

Thanks, Will. I tested these on my SCS tree and didn't run into any
issues. Looks good to me.

Sami

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Sami Tolvanen]

On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
>
> Hi all,
>
> Here's a series of cleanups I hacked together on top of a modified v13
> of the Shadow Call Stack patches from Sami:
>
>   https://lore.kernel.org/r/20200515172355.GD23334@willie-the-truck
>
> The main changes are:
>
>   * Move code out of arch/arm64 and into the core implementation
>   * Store the full SCS stack pointer instead of the offset
>   * Code simplification and general style things
>
> I'd like to queue this on top early next week so that it can spend some
> quality time in linux-next.
>
> Cheers,
>
> Will
>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Mark Rutland <mark.rutland@am.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: <kernel-team@android.com>
>
> --->8
>
> Will Deacon (6):
>   arm64: scs: Store absolute SCS stack pointer value in thread_info
>   scs: Move accounting into alloc/free functions
>   arm64: scs: Use 'scs_sp' register alias for x18
>   scs: Move scs_overflow_check() out of architecture code
>   scs: Remove references to asm/scs.h from core code
>   scs: Move DEFINE_SCS macro into core code
>
>  arch/Kconfig                         |  4 +--
>  arch/arm64/include/asm/scs.h         | 29 ++++------------
>  arch/arm64/include/asm/thread_info.h |  4 +--
>  arch/arm64/kernel/asm-offsets.c      |  2 +-
>  arch/arm64/kernel/entry.S            | 10 +++---
>  arch/arm64/kernel/head.S             |  2 +-
>  arch/arm64/kernel/process.c          |  2 --
>  arch/arm64/kernel/scs.c              |  6 +---
>  include/linux/scs.h                  | 16 +++++----
>  kernel/sched/core.c                  |  3 ++
>  kernel/scs.c                         | 52 +++++++++++++---------------
>  11 files changed, 55 insertions(+), 75 deletions(-)
>
> --
> 2.26.2.761.g0e0b3e54be-goog

Thanks, Will. I tested these on my SCS tree and didn't run into any
issues. Looks good to me.

Sami

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > Will Deacon (6):
> >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> >   scs: Move accounting into alloc/free functions
> >   arm64: scs: Use 'scs_sp' register alias for x18
> >   scs: Move scs_overflow_check() out of architecture code
> >   scs: Remove references to asm/scs.h from core code
> >   scs: Move DEFINE_SCS macro into core code
> >
> >  arch/Kconfig                         |  4 +--
> >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> >  arch/arm64/include/asm/thread_info.h |  4 +--
> >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> >  arch/arm64/kernel/entry.S            | 10 +++---
> >  arch/arm64/kernel/head.S             |  2 +-
> >  arch/arm64/kernel/process.c          |  2 --
> >  arch/arm64/kernel/scs.c              |  6 +---
> >  include/linux/scs.h                  | 16 +++++----
> >  kernel/sched/core.c                  |  3 ++
> >  kernel/scs.c                         | 52 +++++++++++++---------------
> >  11 files changed, 55 insertions(+), 75 deletions(-)
> >
> > --
> > 2.26.2.761.g0e0b3e54be-goog
> 
> Thanks, Will. I tested these on my SCS tree and didn't run into any
> issues. Looks good to me.

Cheers, Sami. Can I add your 'Tested-by' to the patches, please?

Will

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > Will Deacon (6):
> >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> >   scs: Move accounting into alloc/free functions
> >   arm64: scs: Use 'scs_sp' register alias for x18
> >   scs: Move scs_overflow_check() out of architecture code
> >   scs: Remove references to asm/scs.h from core code
> >   scs: Move DEFINE_SCS macro into core code
> >
> >  arch/Kconfig                         |  4 +--
> >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> >  arch/arm64/include/asm/thread_info.h |  4 +--
> >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> >  arch/arm64/kernel/entry.S            | 10 +++---
> >  arch/arm64/kernel/head.S             |  2 +-
> >  arch/arm64/kernel/process.c          |  2 --
> >  arch/arm64/kernel/scs.c              |  6 +---
> >  include/linux/scs.h                  | 16 +++++----
> >  kernel/sched/core.c                  |  3 ++
> >  kernel/scs.c                         | 52 +++++++++++++---------------
> >  11 files changed, 55 insertions(+), 75 deletions(-)
> >
> > --
> > 2.26.2.761.g0e0b3e54be-goog
> 
> Thanks, Will. I tested these on my SCS tree and didn't run into any
> issues. Looks good to me.

Cheers, Sami. Can I add your 'Tested-by' to the patches, please?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Sami Tolvanen]

On Mon, May 18, 2020 at 6:52 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> > On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > > Will Deacon (6):
> > >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> > >   scs: Move accounting into alloc/free functions
> > >   arm64: scs: Use 'scs_sp' register alias for x18
> > >   scs: Move scs_overflow_check() out of architecture code
> > >   scs: Remove references to asm/scs.h from core code
> > >   scs: Move DEFINE_SCS macro into core code
> > >
> > >  arch/Kconfig                         |  4 +--
> > >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> > >  arch/arm64/include/asm/thread_info.h |  4 +--
> > >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> > >  arch/arm64/kernel/entry.S            | 10 +++---
> > >  arch/arm64/kernel/head.S             |  2 +-
> > >  arch/arm64/kernel/process.c          |  2 --
> > >  arch/arm64/kernel/scs.c              |  6 +---
> > >  include/linux/scs.h                  | 16 +++++----
> > >  kernel/sched/core.c                  |  3 ++
> > >  kernel/scs.c                         | 52 +++++++++++++---------------
> > >  11 files changed, 55 insertions(+), 75 deletions(-)
> > >
> > > --
> > > 2.26.2.761.g0e0b3e54be-goog
> >
> > Thanks, Will. I tested these on my SCS tree and didn't run into any
> > issues. Looks good to me.
>
> Cheers, Sami. Can I add your 'Tested-by' to the patches, please?

Sure, please feel free to add Tested-by tags.

Sami

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Sami Tolvanen]

On Mon, May 18, 2020 at 6:52 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> > On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > > Will Deacon (6):
> > >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> > >   scs: Move accounting into alloc/free functions
> > >   arm64: scs: Use 'scs_sp' register alias for x18
> > >   scs: Move scs_overflow_check() out of architecture code
> > >   scs: Remove references to asm/scs.h from core code
> > >   scs: Move DEFINE_SCS macro into core code
> > >
> > >  arch/Kconfig                         |  4 +--
> > >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> > >  arch/arm64/include/asm/thread_info.h |  4 +--
> > >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> > >  arch/arm64/kernel/entry.S            | 10 +++---
> > >  arch/arm64/kernel/head.S             |  2 +-
> > >  arch/arm64/kernel/process.c          |  2 --
> > >  arch/arm64/kernel/scs.c              |  6 +---
> > >  include/linux/scs.h                  | 16 +++++----
> > >  kernel/sched/core.c                  |  3 ++
> > >  kernel/scs.c                         | 52 +++++++++++++---------------
> > >  11 files changed, 55 insertions(+), 75 deletions(-)
> > >
> > > --
> > > 2.26.2.761.g0e0b3e54be-goog
> >
> > Thanks, Will. I tested these on my SCS tree and didn't run into any
> > issues. Looks good to me.
>
> Cheers, Sami. Can I add your 'Tested-by' to the patches, please?

Sure, please feel free to add Tested-by tags.

Sami

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

On Mon, May 18, 2020 at 08:43:16AM -0700, Sami Tolvanen wrote:
> On Mon, May 18, 2020 at 6:52 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> > > On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > > > Will Deacon (6):
> > > >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> > > >   scs: Move accounting into alloc/free functions
> > > >   arm64: scs: Use 'scs_sp' register alias for x18
> > > >   scs: Move scs_overflow_check() out of architecture code
> > > >   scs: Remove references to asm/scs.h from core code
> > > >   scs: Move DEFINE_SCS macro into core code
> > > >
> > > >  arch/Kconfig                         |  4 +--
> > > >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> > > >  arch/arm64/include/asm/thread_info.h |  4 +--
> > > >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> > > >  arch/arm64/kernel/entry.S            | 10 +++---
> > > >  arch/arm64/kernel/head.S             |  2 +-
> > > >  arch/arm64/kernel/process.c          |  2 --
> > > >  arch/arm64/kernel/scs.c              |  6 +---
> > > >  include/linux/scs.h                  | 16 +++++----
> > > >  kernel/sched/core.c                  |  3 ++
> > > >  kernel/scs.c                         | 52 +++++++++++++---------------
> > > >  11 files changed, 55 insertions(+), 75 deletions(-)
> > > >
> > > > --
> > > > 2.26.2.761.g0e0b3e54be-goog
> > >
> > > Thanks, Will. I tested these on my SCS tree and didn't run into any
> > > issues. Looks good to me.
> >
> > Cheers, Sami. Can I add your 'Tested-by' to the patches, please?
> 
> Sure, please feel free to add Tested-by tags.

Thanks. I've updated the for-next/scs branch with that.

Will

Re: [PATCH 0/6] Clean up Shadow Call Stack patches for 5.8

[Will Deacon]

On Mon, May 18, 2020 at 08:43:16AM -0700, Sami Tolvanen wrote:
> On Mon, May 18, 2020 at 6:52 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Fri, May 15, 2020 at 01:42:40PM -0700, Sami Tolvanen wrote:
> > > On Fri, May 15, 2020 at 10:28 AM Will Deacon <will@kernel.org> wrote:
> > > > Will Deacon (6):
> > > >   arm64: scs: Store absolute SCS stack pointer value in thread_info
> > > >   scs: Move accounting into alloc/free functions
> > > >   arm64: scs: Use 'scs_sp' register alias for x18
> > > >   scs: Move scs_overflow_check() out of architecture code
> > > >   scs: Remove references to asm/scs.h from core code
> > > >   scs: Move DEFINE_SCS macro into core code
> > > >
> > > >  arch/Kconfig                         |  4 +--
> > > >  arch/arm64/include/asm/scs.h         | 29 ++++------------
> > > >  arch/arm64/include/asm/thread_info.h |  4 +--
> > > >  arch/arm64/kernel/asm-offsets.c      |  2 +-
> > > >  arch/arm64/kernel/entry.S            | 10 +++---
> > > >  arch/arm64/kernel/head.S             |  2 +-
> > > >  arch/arm64/kernel/process.c          |  2 --
> > > >  arch/arm64/kernel/scs.c              |  6 +---
> > > >  include/linux/scs.h                  | 16 +++++----
> > > >  kernel/sched/core.c                  |  3 ++
> > > >  kernel/scs.c                         | 52 +++++++++++++---------------
> > > >  11 files changed, 55 insertions(+), 75 deletions(-)
> > > >
> > > > --
> > > > 2.26.2.761.g0e0b3e54be-goog
> > >
> > > Thanks, Will. I tested these on my SCS tree and didn't run into any
> > > issues. Looks good to me.
> >
> > Cheers, Sami. Can I add your 'Tested-by' to the patches, please?
> 
> Sure, please feel free to add Tested-by tags.

Thanks. I've updated the for-next/scs branch with that.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel