linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] arm64: Change the on_*stack functions to take a size argument
@ 2020-10-26 20:59 Peter Collingbourne
  2020-10-26 20:59 ` [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes Peter Collingbourne
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Collingbourne @ 2020-10-26 20:59 UTC (permalink / raw)
  To: Mark Brown, Mark Rutland, Will Deacon, Catalin Marinas
  Cc: Andrey Konovalov, Peter Collingbourne, Linux ARM

unwind_frame() was previously implicitly checking that the frame
record is in bounds of the stack by enforcing that FP is both aligned
to 16 and in bounds of the stack. Once the FP alignment requirement
is relaxed to 8 this will not be sufficient because it does not
account for the case where FP points to 8 bytes before the end of the
stack.

Make the check explicit by changing the on_*stack functions to take a
size argument and adjusting the callers to pass the appropriate sizes.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/Ib7a3eb3eea41b0687ffaba045ceb2012d077d8b4
---
 arch/arm64/include/asm/processor.h  | 12 +++++------
 arch/arm64/include/asm/sdei.h       |  7 ++++---
 arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++---------------
 arch/arm64/kernel/ptrace.c          |  2 +-
 arch/arm64/kernel/sdei.c            | 16 ++++++++-------
 arch/arm64/kernel/stacktrace.c      |  2 +-
 6 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index fce8cbecd6bc..bf3e07f67e79 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -318,13 +318,13 @@ long get_tagged_addr_ctrl(struct task_struct *task);
  * of header definitions for the use of task_stack_page.
  */
 
-#define current_top_of_stack()							\
-({										\
-	struct stack_info _info;						\
-	BUG_ON(!on_accessible_stack(current, current_stack_pointer, &_info));	\
-	_info.high;								\
+#define current_top_of_stack()								\
+({											\
+	struct stack_info _info;							\
+	BUG_ON(!on_accessible_stack(current, current_stack_pointer, 1, &_info));	\
+	_info.high;									\
 })
-#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, NULL))
+#define on_thread_stack()	(on_task_stack(current, current_stack_pointer, 1, NULL))
 
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
index 63e0b92a5fbb..8bc30a5c4569 100644
--- a/arch/arm64/include/asm/sdei.h
+++ b/arch/arm64/include/asm/sdei.h
@@ -42,8 +42,9 @@ unsigned long sdei_arch_get_entry_point(int conduit);
 
 struct stack_info;
 
-bool _on_sdei_stack(unsigned long sp, struct stack_info *info);
-static inline bool on_sdei_stack(unsigned long sp,
+bool _on_sdei_stack(unsigned long sp, unsigned long size,
+		    struct stack_info *info);
+static inline bool on_sdei_stack(unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
@@ -51,7 +52,7 @@ static inline bool on_sdei_stack(unsigned long sp,
 	if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
 		return false;
 	if (in_nmi())
-		return _on_sdei_stack(sp, info);
+		return _on_sdei_stack(sp, size, info);
 
 	return false;
 }
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index eb29b1fe8255..77192df232bf 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -69,14 +69,14 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
 
-static inline bool on_stack(unsigned long sp, unsigned long low,
-				unsigned long high, enum stack_type type,
-				struct stack_info *info)
+static inline bool on_stack(unsigned long sp, unsigned long size,
+			    unsigned long low, unsigned long high,
+			    enum stack_type type, struct stack_info *info)
 {
 	if (!low)
 		return false;
 
-	if (sp < low || sp >= high)
+	if (sp < low || sp + size < sp || sp + size > high)
 		return false;
 
 	if (info) {
@@ -87,38 +87,38 @@ static inline bool on_stack(unsigned long sp, unsigned long low,
 	return true;
 }
 
-static inline bool on_irq_stack(unsigned long sp,
+static inline bool on_irq_stack(unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
 	unsigned long high = low + IRQ_STACK_SIZE;
 
-	return on_stack(sp, low, high, STACK_TYPE_IRQ, info);
+	return on_stack(sp, size, low, high, STACK_TYPE_IRQ, info);
 }
 
 static inline bool on_task_stack(const struct task_struct *tsk,
-				 unsigned long sp,
+				 unsigned long sp, unsigned long size,
 				 struct stack_info *info)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
 
-	return on_stack(sp, low, high, STACK_TYPE_TASK, info);
+	return on_stack(sp, size, low, high, STACK_TYPE_TASK, info);
 }
 
 #ifdef CONFIG_VMAP_STACK
 DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
 
-static inline bool on_overflow_stack(unsigned long sp,
+static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
 				struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
 	unsigned long high = low + OVERFLOW_STACK_SIZE;
 
-	return on_stack(sp, low, high, STACK_TYPE_OVERFLOW, info);
+	return on_stack(sp, size, low, high, STACK_TYPE_OVERFLOW, info);
 }
 #else
-static inline bool on_overflow_stack(unsigned long sp,
+static inline bool on_overflow_stack(unsigned long sp, unsigned long size,
 			struct stack_info *info) { return false; }
 #endif
 
@@ -128,21 +128,21 @@ static inline bool on_overflow_stack(unsigned long sp,
  * context.
  */
 static inline bool on_accessible_stack(const struct task_struct *tsk,
-				       unsigned long sp,
+				       unsigned long sp, unsigned long size,
 				       struct stack_info *info)
 {
 	if (info)
 		info->type = STACK_TYPE_UNKNOWN;
 
-	if (on_task_stack(tsk, sp, info))
+	if (on_task_stack(tsk, sp, size, info))
 		return true;
 	if (tsk != current || preemptible())
 		return false;
-	if (on_irq_stack(sp, info))
+	if (on_irq_stack(sp, size, info))
 		return true;
-	if (on_overflow_stack(sp, info))
+	if (on_overflow_stack(sp, size, info))
 		return true;
-	if (on_sdei_stack(sp, info))
+	if (on_sdei_stack(sp, size, info))
 		return true;
 
 	return false;
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index f49b349e16a3..b44d6981decb 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -122,7 +122,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
 {
 	return ((addr & ~(THREAD_SIZE - 1))  ==
 		(kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
-		on_irq_stack(addr, NULL);
+		on_irq_stack(addr, sizeof(unsigned long), NULL);
 }
 
 /**
diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
index 7689f2031c0c..c5cf2f98013d 100644
--- a/arch/arm64/kernel/sdei.c
+++ b/arch/arm64/kernel/sdei.c
@@ -90,31 +90,33 @@ static int init_sdei_stacks(void)
 	return err;
 }
 
-static bool on_sdei_normal_stack(unsigned long sp, struct stack_info *info)
+static bool on_sdei_normal_stack(unsigned long sp, unsigned long size,
+				 struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
 	unsigned long high = low + SDEI_STACK_SIZE;
 
-	return on_stack(sp, low, high, STACK_TYPE_SDEI_NORMAL, info);
+	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_NORMAL, info);
 }
 
-static bool on_sdei_critical_stack(unsigned long sp, struct stack_info *info)
+static bool on_sdei_critical_stack(unsigned long sp, unsigned long size,
+				   struct stack_info *info)
 {
 	unsigned long low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
 	unsigned long high = low + SDEI_STACK_SIZE;
 
-	return on_stack(sp, low, high, STACK_TYPE_SDEI_CRITICAL, info);
+	return on_stack(sp, size, low, high, STACK_TYPE_SDEI_CRITICAL, info);
 }
 
-bool _on_sdei_stack(unsigned long sp, struct stack_info *info)
+bool _on_sdei_stack(unsigned long sp, unsigned long size, struct stack_info *info)
 {
 	if (!IS_ENABLED(CONFIG_VMAP_STACK))
 		return false;
 
-	if (on_sdei_critical_stack(sp, info))
+	if (on_sdei_critical_stack(sp, size, info))
 		return true;
 
-	if (on_sdei_normal_stack(sp, info))
+	if (on_sdei_normal_stack(sp, size, info))
 		return true;
 
 	return false;
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index fa56af1a59c3..ce613e22b58b 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,7 +50,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	if (!tsk)
 		tsk = current;
 
-	if (!on_accessible_stack(tsk, fp, &info))
+	if (!on_accessible_stack(tsk, fp, 16, &info))
 		return -EINVAL;
 
 	if (test_bit(info.type, frame->stacks_done))
-- 
2.29.0.rc2.309.g374f81d7ae-goog


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes
  2020-10-26 20:59 [PATCH v2 1/2] arm64: Change the on_*stack functions to take a size argument Peter Collingbourne
@ 2020-10-26 20:59 ` Peter Collingbourne
  2020-11-21  2:32   ` Peter Collingbourne
  2020-11-23 18:21   ` Andrey Konovalov
  0 siblings, 2 replies; 6+ messages in thread
From: Peter Collingbourne @ 2020-10-26 20:59 UTC (permalink / raw)
  To: Mark Brown, Mark Rutland, Will Deacon, Catalin Marinas
  Cc: Andrey Konovalov, Peter Collingbourne, Linux ARM

The AAPCS places no requirements on the alignment of the frame
record. In theory it could be placed anywhere, although it seems
sensible to require it to be aligned to 8 bytes. With an upcoming
enhancement to tag-based KASAN Clang will begin creating frame records
located at an address that is only aligned to 8 bytes. Accommodate
such frame records in the stack unwinding code.

As pointed out by Mark Rutland, the userspace stack unwinding code
has the same problem, so fix it there as well.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/Ia22c375230e67ca055e9e4bb639383567f7ad268
---
v2:
- fix it in the userspace unwinding code as well

 arch/arm64/kernel/perf_callchain.c | 2 +-
 arch/arm64/kernel/stacktrace.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 88ff471b0bce..4a72c2727309 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -116,7 +116,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
 		tail = (struct frame_tail __user *)regs->regs[29];
 
 		while (entry->nr < entry->max_stack &&
-		       tail && !((unsigned long)tail & 0xf))
+		       tail && !((unsigned long)tail & 0x7))
 			tail = user_backtrace(tail, entry);
 	} else {
 #ifdef CONFIG_COMPAT
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ce613e22b58b..3bc1c44b7910 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -44,7 +44,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	unsigned long fp = frame->fp;
 	struct stack_info info;
 
-	if (fp & 0xf)
+	if (fp & 0x7)
 		return -EINVAL;
 
 	if (!tsk)
-- 
2.29.0.rc2.309.g374f81d7ae-goog


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes
  2020-10-26 20:59 ` [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes Peter Collingbourne
@ 2020-11-21  2:32   ` Peter Collingbourne
  2020-12-01 17:28     ` Catalin Marinas
  2020-11-23 18:21   ` Andrey Konovalov
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Collingbourne @ 2020-11-21  2:32 UTC (permalink / raw)
  To: Mark Brown, Mark Rutland, Will Deacon, Catalin Marinas
  Cc: Andrey Konovalov, Linux ARM

On Mon, Oct 26, 2020 at 2:00 PM Peter Collingbourne <pcc@google.com> wrote:
>
> The AAPCS places no requirements on the alignment of the frame
> record. In theory it could be placed anywhere, although it seems
> sensible to require it to be aligned to 8 bytes. With an upcoming
> enhancement to tag-based KASAN Clang will begin creating frame records
> located at an address that is only aligned to 8 bytes. Accommodate
> such frame records in the stack unwinding code.
>
> As pointed out by Mark Rutland, the userspace stack unwinding code
> has the same problem, so fix it there as well.

As a reminder, this series is required in order to avoid breaking
stack traces once [1] is applied.

Peter

[1] https://lore.kernel.org/linux-arm-kernel/20201120230211.584929-1-pcc@google.com/T/

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes
  2020-10-26 20:59 ` [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes Peter Collingbourne
  2020-11-21  2:32   ` Peter Collingbourne
@ 2020-11-23 18:21   ` Andrey Konovalov
  1 sibling, 0 replies; 6+ messages in thread
From: Andrey Konovalov @ 2020-11-23 18:21 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Mark Rutland, Catalin Marinas, Mark Brown, Will Deacon, Linux ARM

On Mon, Oct 26, 2020 at 10:00 PM Peter Collingbourne <pcc@google.com> wrote:
>
> The AAPCS places no requirements on the alignment of the frame
> record. In theory it could be placed anywhere, although it seems
> sensible to require it to be aligned to 8 bytes. With an upcoming
> enhancement to tag-based KASAN Clang will begin creating frame records
> located at an address that is only aligned to 8 bytes. Accommodate
> such frame records in the stack unwinding code.
>
> As pointed out by Mark Rutland, the userspace stack unwinding code
> has the same problem, so fix it there as well.
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/Ia22c375230e67ca055e9e4bb639383567f7ad268
> ---
> v2:
> - fix it in the userspace unwinding code as well
>
>  arch/arm64/kernel/perf_callchain.c | 2 +-
>  arch/arm64/kernel/stacktrace.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
> index 88ff471b0bce..4a72c2727309 100644
> --- a/arch/arm64/kernel/perf_callchain.c
> +++ b/arch/arm64/kernel/perf_callchain.c
> @@ -116,7 +116,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
>                 tail = (struct frame_tail __user *)regs->regs[29];
>
>                 while (entry->nr < entry->max_stack &&
> -                      tail && !((unsigned long)tail & 0xf))
> +                      tail && !((unsigned long)tail & 0x7))
>                         tail = user_backtrace(tail, entry);
>         } else {
>  #ifdef CONFIG_COMPAT
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index ce613e22b58b..3bc1c44b7910 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -44,7 +44,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>         unsigned long fp = frame->fp;
>         struct stack_info info;
>
> -       if (fp & 0xf)
> +       if (fp & 0x7)
>                 return -EINVAL;
>
>         if (!tsk)
> --
> 2.29.0.rc2.309.g374f81d7ae-goog
>

Acked-by: Andrey Konovalov <andreyknvl@google.com>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes
  2020-11-21  2:32   ` Peter Collingbourne
@ 2020-12-01 17:28     ` Catalin Marinas
  2020-12-03  5:15       ` Peter Collingbourne
  0 siblings, 1 reply; 6+ messages in thread
From: Catalin Marinas @ 2020-12-01 17:28 UTC (permalink / raw)
  To: Peter Collingbourne
  Cc: Mark Rutland, Andrey Konovalov, Mark Brown, Will Deacon, Linux ARM

On Fri, Nov 20, 2020 at 06:32:35PM -0800, Peter Collingbourne wrote:
> On Mon, Oct 26, 2020 at 2:00 PM Peter Collingbourne <pcc@google.com> wrote:
> > The AAPCS places no requirements on the alignment of the frame
> > record. In theory it could be placed anywhere, although it seems
> > sensible to require it to be aligned to 8 bytes. With an upcoming
> > enhancement to tag-based KASAN Clang will begin creating frame records
> > located at an address that is only aligned to 8 bytes. Accommodate
> > such frame records in the stack unwinding code.
> >
> > As pointed out by Mark Rutland, the userspace stack unwinding code
> > has the same problem, so fix it there as well.
> 
> As a reminder, this series is required in order to avoid breaking
> stack traces once [1] is applied.
> 
> Peter
> 
> [1] https://lore.kernel.org/linux-arm-kernel/20201120230211.584929-1-pcc@google.com/T/

While this series looks fine, on its own it doesn't solve any issue we
currently have. So, could you please post this series together with the
outlined tags mismatch checks patch? I think they should be merged
together.

Thanks.

-- 
Catalin

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes
  2020-12-01 17:28     ` Catalin Marinas
@ 2020-12-03  5:15       ` Peter Collingbourne
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Collingbourne @ 2020-12-03  5:15 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Mark Rutland, Andrey Konovalov, Mark Brown, Will Deacon, Linux ARM

On Tue, Dec 1, 2020 at 9:28 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Nov 20, 2020 at 06:32:35PM -0800, Peter Collingbourne wrote:
> > On Mon, Oct 26, 2020 at 2:00 PM Peter Collingbourne <pcc@google.com> wrote:
> > > The AAPCS places no requirements on the alignment of the frame
> > > record. In theory it could be placed anywhere, although it seems
> > > sensible to require it to be aligned to 8 bytes. With an upcoming
> > > enhancement to tag-based KASAN Clang will begin creating frame records
> > > located at an address that is only aligned to 8 bytes. Accommodate
> > > such frame records in the stack unwinding code.
> > >
> > > As pointed out by Mark Rutland, the userspace stack unwinding code
> > > has the same problem, so fix it there as well.
> >
> > As a reminder, this series is required in order to avoid breaking
> > stack traces once [1] is applied.
> >
> > Peter
> >
> > [1] https://lore.kernel.org/linux-arm-kernel/20201120230211.584929-1-pcc@google.com/T/
>
> While this series looks fine, on its own it doesn't solve any issue we
> currently have. So, could you please post this series together with the
> outlined tags mismatch checks patch? I think they should be merged
> together.
>
> Thanks.

Done (as also requested by Mark Rutland) in v3 of that series.

Peter

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-03  5:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 20:59 [PATCH v2 1/2] arm64: Change the on_*stack functions to take a size argument Peter Collingbourne
2020-10-26 20:59 ` [PATCH v2 2/2] arm64: stacktrace: Relax frame record alignment requirement to 8 bytes Peter Collingbourne
2020-11-21  2:32   ` Peter Collingbourne
2020-12-01 17:28     ` Catalin Marinas
2020-12-03  5:15       ` Peter Collingbourne
2020-11-23 18:21   ` Andrey Konovalov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).