[PATCH RFC 0/8] hardened usercopy and stacktrace improvement

[PATCH RFC 0/8] hardened usercopy and stacktrace improvement

[He Zhe]

This series is to add copy-beyond-the-frame detection for hardened usercopy
for arm64 by adding more arguments to callbacks of stackstrace and by the way
more potential checks in the future can be done. This is done by:

- Change stacktrace callback prototype to pass more information which currently
  includes pc, current stack's fp and previous frame's fp, so copying beyond the
  frame can be detected.
- Add arch_within_stack_frames for arm64 based on existing x86 algorithm, but
  via stacktrace.
- Make other arch_stack_walk callbacks use the new prototype.
- Tune lkdtm usercopy stack frame case. Currently the way to generate bad frame
  use depends on stack layout of the architecture and compiler. It happens to
  work with x86 but does not work for arm64 and it ruins the stack in
  USERCOPY_STACK_FRAME_FROM and thus causes the call chain to return to NULL
  address.

He Zhe (8):
  stacktrace: Change callback prototype to pass more information
  arm64: stacktrace: Add arch_within_stack_frames
  arm64: stacktrace: Make callbacks use new prototype with frame info
  powerpc: stacktrace: Make callbacks use new prototype with frame info
  riscv: stacktrace: Make callbacks use new prototype with frame info
  s390: stacktrace: Make callbacks use new prototype with frame info
  x86: stacktrace: Make callbacks use new prototype with frame info
  lkdtm: usercopy: Make USERCOPY_STACK_FRAME_x able to work for all
    archs

 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h | 12 +++++
 arch/arm64/kernel/perf_callchain.c   |  4 +-
 arch/arm64/kernel/process.c          |  6 +--
 arch/arm64/kernel/return_address.c   |  4 +-
 arch/arm64/kernel/stacktrace.c       | 76 ++++++++++++++++++++++++++--
 arch/arm64/kernel/time.c             |  6 +--
 arch/powerpc/kernel/stacktrace.c     | 18 ++++---
 arch/riscv/include/asm/stacktrace.h  |  2 +-
 arch/riscv/kernel/perf_callchain.c   |  4 +-
 arch/riscv/kernel/stacktrace.c       | 22 ++++----
 arch/s390/kernel/stacktrace.c        | 14 ++---
 arch/x86/kernel/stacktrace.c         | 28 ++++++----
 drivers/misc/lkdtm/usercopy.c        | 24 +--------
 include/linux/stacktrace.h           |  9 +++-
 kernel/stacktrace.c                  | 10 ++--
 16 files changed, 161 insertions(+), 79 deletions(-)

-- 
2.25.1

[PATCH RFC 1/8] stacktrace: Change callback prototype to pass more information

[He Zhe]

Currently stack_trace_consume_fn can only have pc of each frame of the
stack. Copying-beyond-the-frame-detection also needs fp of current and
previous frame. Other detection algorithm in the future may need more
information of the frame.

We define a frame_info to include them all.


Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 include/linux/stacktrace.h |  9 ++++++++-
 kernel/stacktrace.c        | 10 +++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
index 97455880ac41..5a61bfafe6f0 100644
--- a/include/linux/stacktrace.h
+++ b/include/linux/stacktrace.h
@@ -10,15 +10,22 @@ struct pt_regs;
 
 #ifdef CONFIG_ARCH_STACKWALK
 
+struct frame_info {
+	unsigned long pc;
+	unsigned long fp;
+	unsigned long prev_fp;
+};
+
 /**
  * stack_trace_consume_fn - Callback for arch_stack_walk()
  * @cookie:	Caller supplied pointer handed back by arch_stack_walk()
  * @addr:	The stack entry address to consume
+ * @fi:	The frame information to consume
  *
  * Return:	True, if the entry was consumed or skipped
  *		False, if there is no space left to store
  */
-typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr);
+typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
 /**
  * arch_stack_walk - Architecture specific function to walk the stack
  * @consume_entry:	Callback which is invoked by the architecture code for
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 9ed5ce989415..2d0a2812e92b 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -79,7 +79,7 @@ struct stacktrace_cookie {
 	unsigned int	len;
 };
 
-static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
+static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi)
 {
 	struct stacktrace_cookie *c = cookie;
 
@@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
 		c->skip--;
 		return true;
 	}
-	c->store[c->len++] = addr;
+	c->store[c->len++] = fi->pc;
 	return c->len < c->size;
 }
 
-static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr)
+static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi)
 {
-	if (in_sched_functions(addr))
+	if (in_sched_functions(fi->pc))
 		return true;
-	return stack_trace_consume_entry(cookie, addr);
+	return stack_trace_consume_entry(cookie, fi);
 }
 
 /**
-- 
2.25.1

Re: [PATCH RFC 1/8] stacktrace: Change callback prototype to pass more information

[Mark Rutland]

On Mon, Apr 18, 2022 at 09:22:10PM +0800, He Zhe wrote:
> Currently stack_trace_consume_fn can only have pc of each frame of the
> stack. Copying-beyond-the-frame-detection also needs fp of current and
> previous frame. Other detection algorithm in the future may need more
> information of the frame.
> 
> We define a frame_info to include them all.
> 
> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  include/linux/stacktrace.h |  9 ++++++++-
>  kernel/stacktrace.c        | 10 +++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
> index 97455880ac41..5a61bfafe6f0 100644
> --- a/include/linux/stacktrace.h
> +++ b/include/linux/stacktrace.h
> @@ -10,15 +10,22 @@ struct pt_regs;
>  
>  #ifdef CONFIG_ARCH_STACKWALK
>  
> +struct frame_info {
> +	unsigned long pc;
> +	unsigned long fp;
> +	unsigned long prev_fp;
> +};

I don't think this should be exposed through a generic interface; the `fp` and
`prev_fp` values are only meaningful with arch-specific knowledge, and they're
*very* easy to misuse (e.g. when transitioning from one stack to another).
There's also a bunch of other information one may or may not want, depending on
what you're trying to achieve.

I am happy to have an arch-specific internal unwinder where we can access this
information, and *maybe* it makes sense to have a generic API that passes some
opaque token, but I don't think we should make the structure generic.

Thanks,
Mark.

> +
>  /**
>   * stack_trace_consume_fn - Callback for arch_stack_walk()
>   * @cookie:	Caller supplied pointer handed back by arch_stack_walk()
>   * @addr:	The stack entry address to consume
> + * @fi:	The frame information to consume
>   *
>   * Return:	True, if the entry was consumed or skipped
>   *		False, if there is no space left to store
>   */
> -typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr);
> +typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
>  /**
>   * arch_stack_walk - Architecture specific function to walk the stack
>   * @consume_entry:	Callback which is invoked by the architecture code for
> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
> index 9ed5ce989415..2d0a2812e92b 100644
> --- a/kernel/stacktrace.c
> +++ b/kernel/stacktrace.c
> @@ -79,7 +79,7 @@ struct stacktrace_cookie {
>  	unsigned int	len;
>  };
>  
> -static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
> +static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi)
>  {
>  	struct stacktrace_cookie *c = cookie;
>  
> @@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
>  		c->skip--;
>  		return true;
>  	}
> -	c->store[c->len++] = addr;
> +	c->store[c->len++] = fi->pc;
>  	return c->len < c->size;
>  }
>  
> -static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr)
> +static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi)
>  {
> -	if (in_sched_functions(addr))
> +	if (in_sched_functions(fi->pc))
>  		return true;
> -	return stack_trace_consume_entry(cookie, addr);
> +	return stack_trace_consume_entry(cookie, fi);
>  }
>  
>  /**
> -- 
> 2.25.1
> 

Re: [PATCH RFC 1/8] stacktrace: Change callback prototype to pass more information

[He Zhe]

On 4/19/22 21:09, Mark Rutland wrote:
> On Mon, Apr 18, 2022 at 09:22:10PM +0800, He Zhe wrote:
>> Currently stack_trace_consume_fn can only have pc of each frame of the
>> stack. Copying-beyond-the-frame-detection also needs fp of current and
>> previous frame. Other detection algorithm in the future may need more
>> information of the frame.
>>
>> We define a frame_info to include them all.
>>
>>
>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>> ---
>>  include/linux/stacktrace.h |  9 ++++++++-
>>  kernel/stacktrace.c        | 10 +++++-----
>>  2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
>> index 97455880ac41..5a61bfafe6f0 100644
>> --- a/include/linux/stacktrace.h
>> +++ b/include/linux/stacktrace.h
>> @@ -10,15 +10,22 @@ struct pt_regs;
>>  
>>  #ifdef CONFIG_ARCH_STACKWALK
>>  
>> +struct frame_info {
>> +	unsigned long pc;
>> +	unsigned long fp;
>> +	unsigned long prev_fp;
>> +};
> I don't think this should be exposed through a generic interface; the `fp` and
> `prev_fp` values are only meaningful with arch-specific knowledge, and they're
> *very* easy to misuse (e.g. when transitioning from one stack to another).
> There's also a bunch of other information one may or may not want, depending on
> what you're trying to achieve.
>
> I am happy to have an arch-specific internal unwinder where we can access this
> information, and *maybe* it makes sense to have a generic API that passes some
> opaque token, but I don't think we should make the structure generic.

Thanks for thoughts. I saw unwind_frame and etc was made private earlier and
took that as a hint that all further stack walk things should be based on those
functions and came up with this. But OK, good to know that arch-specific unwind
would be fine, I'll redo this series in that way.

Thanks,
Zhe

>
> Thanks,
> Mark.
>
>> +
>>  /**
>>   * stack_trace_consume_fn - Callback for arch_stack_walk()
>>   * @cookie:	Caller supplied pointer handed back by arch_stack_walk()
>>   * @addr:	The stack entry address to consume
>> + * @fi:	The frame information to consume
>>   *
>>   * Return:	True, if the entry was consumed or skipped
>>   *		False, if there is no space left to store
>>   */
>> -typedef bool (*stack_trace_consume_fn)(void *cookie, unsigned long addr);
>> +typedef bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
>>  /**
>>   * arch_stack_walk - Architecture specific function to walk the stack
>>   * @consume_entry:	Callback which is invoked by the architecture code for
>> diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
>> index 9ed5ce989415..2d0a2812e92b 100644
>> --- a/kernel/stacktrace.c
>> +++ b/kernel/stacktrace.c
>> @@ -79,7 +79,7 @@ struct stacktrace_cookie {
>>  	unsigned int	len;
>>  };
>>  
>> -static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
>> +static bool stack_trace_consume_entry(void *cookie, struct frame_info *fi)
>>  {
>>  	struct stacktrace_cookie *c = cookie;
>>  
>> @@ -90,15 +90,15 @@ static bool stack_trace_consume_entry(void *cookie, unsigned long addr)
>>  		c->skip--;
>>  		return true;
>>  	}
>> -	c->store[c->len++] = addr;
>> +	c->store[c->len++] = fi->pc;
>>  	return c->len < c->size;
>>  }
>>  
>> -static bool stack_trace_consume_entry_nosched(void *cookie, unsigned long addr)
>> +static bool stack_trace_consume_entry_nosched(void *cookie, struct frame_info *fi)
>>  {
>> -	if (in_sched_functions(addr))
>> +	if (in_sched_functions(fi->pc))
>>  		return true;
>> -	return stack_trace_consume_entry(cookie, addr);
>> +	return stack_trace_consume_entry(cookie, fi);
>>  }
>>  
>>  /**
>> -- 
>> 2.25.1
>>

[PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[He Zhe]

This function checks if the given address range crosses frame boundary.
It is based on the existing x86 algorithm, but implemented via stacktrace.
This can be tested by USERCOPY_STACK_FRAME_FROM and
USERCOPY_STACK_FRAME_TO in lkdtm.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h | 12 +++++
 arch/arm64/kernel/stacktrace.c       | 76 ++++++++++++++++++++++++++--
 3 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 57c4c995965f..0f52a83d7771 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -165,6 +165,7 @@ config ARM64
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	select HAVE_ARCH_VMAP_STACK
+	select HAVE_ARCH_WITHIN_STACK_FRAMES
 	select HAVE_ARM_SMCCC
 	select HAVE_ASM_MODVERSIONS
 	select HAVE_EBPF_JIT
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index e1317b7c4525..b839ad9f2248 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -58,6 +58,18 @@ void arch_setup_new_exec(void);
 void arch_release_task_struct(struct task_struct *tsk);
 int arch_dup_task_struct(struct task_struct *dst,
 				struct task_struct *src);
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ *	GOOD_FRAME	if within a frame
+ *	BAD_STACK	if placed across a frame boundary (or outside stack)
+ *	NOT_STACK	unable to determine (no frame pointers, etc)
+ */
+int arch_within_stack_frames(const void * const stack,
+		const void * const stackend,
+		const void *obj, unsigned long len);
 
 #endif
 
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e4103e085681..219b90c1de12 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -145,12 +145,17 @@ NOKPROBE_SYMBOL(unwind_frame);
 
 static void notrace walk_stackframe(struct task_struct *tsk,
 				    struct stackframe *frame,
-				    bool (*fn)(void *, unsigned long), void *data)
+				    stack_trace_consume_fn fn, void *data)
 {
+	struct frame_info fi;
+
 	while (1) {
 		int ret;
 
-		if (!fn(data, frame->pc))
+		fi.pc = frame->pc;
+		fi.fp = frame->fp;
+		fi.prev_fp = frame->prev_fp;
+		if (!fn(data, &fi))
 			break;
 		ret = unwind_frame(tsk, frame);
 		if (ret < 0)
@@ -159,10 +164,10 @@ static void notrace walk_stackframe(struct task_struct *tsk,
 }
 NOKPROBE_SYMBOL(walk_stackframe);
 
-static bool dump_backtrace_entry(void *arg, unsigned long where)
+static bool dump_backtrace_entry(void *arg, struct frame_info *fi)
 {
 	char *loglvl = arg;
-	printk("%s %pSb\n", loglvl, (void *)where);
+	printk("%s %pSb\n", loglvl, (void *)fi->pc);
 	return true;
 }
 
@@ -210,3 +215,66 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 
 	walk_stackframe(task, &frame, consume_entry, cookie);
 }
+
+struct arch_stack_object {
+	unsigned long start;
+	unsigned long len;
+	int flag;
+};
+
+static bool arch_stack_object_check(void *data, struct frame_info *fi)
+{
+	struct arch_stack_object *obj = (struct arch_stack_object *)data;
+
+	/* Skip the frame of arch_within_stack_frames itself */
+	if (fi->prev_fp == 0)
+		return true;
+
+	/*
+	 * low ----------------------------------------------> high
+	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
+	 *                     ^----------------^
+	 *               allow copies only within here
+	 */
+	if (obj->start + obj->len <= fi->fp) {
+		obj->flag = obj->start >=
+			fi->prev_fp + 2 * sizeof(void *) ?
+			GOOD_FRAME : BAD_STACK;
+		return false;
+	} else
+		return true;
+}
+
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ *	GOOD_FRAME	if within a frame
+ *	BAD_STACK	if placed across a frame boundary (or outside stack)
+ *	NOT_STACK	unable to determine (no frame pointers, etc)
+ */
+int arch_within_stack_frames(const void * const stack,
+		const void * const stackend,
+		const void *obj, unsigned long len)
+{
+#if defined(CONFIG_FRAME_POINTER)
+	struct arch_stack_object object;
+	struct pt_regs regs;
+
+	if (__builtin_frame_address(1) == 0)
+		return NOT_STACK;
+
+	object.start = (unsigned long)obj;
+	object.len = len;
+	object.flag = NOT_STACK;
+
+	regs.regs[29] = (u64)__builtin_frame_address(1);
+
+	arch_stack_walk(arch_stack_object_check, (void *)&object, NULL, &regs);
+
+	return object.flag;
+#else
+	return NOT_STACK;
+#endif
+}
-- 
2.25.1

Re: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[Kees Cook]

On Mon, Apr 18, 2022 at 09:22:11PM +0800, He Zhe wrote:
> This function checks if the given address range crosses frame boundary.
> It is based on the existing x86 algorithm, but implemented via stacktrace.
> This can be tested by USERCOPY_STACK_FRAME_FROM and
> USERCOPY_STACK_FRAME_TO in lkdtm.

Hi,

Thanks for doing this implementation! One reason usercopy hardening
didn't persue doing a "full" stacktrace was because it seemed relatively
expensive. Did you do any usercopy-heavily workload testing to see if
there was a noticeable performance impact?

It would be nice to block the exposure of canaries and PAC bits, though,
so I'm not opposed, but I'd like to get a better sense of how "heavy"
this might be.

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[He Zhe]

On 4/19/22 05:59, Kees Cook wrote:
> On Mon, Apr 18, 2022 at 09:22:11PM +0800, He Zhe wrote:
>> This function checks if the given address range crosses frame boundary.
>> It is based on the existing x86 algorithm, but implemented via stacktrace.
>> This can be tested by USERCOPY_STACK_FRAME_FROM and
>> USERCOPY_STACK_FRAME_TO in lkdtm.
> Hi,
>
> Thanks for doing this implementation! One reason usercopy hardening
> didn't persue doing a "full" stacktrace was because it seemed relatively
> expensive. Did you do any usercopy-heavily workload testing to see if
> there was a noticeable performance impact?
>
> It would be nice to block the exposure of canaries and PAC bits, though,
> so I'm not opposed, but I'd like to get a better sense of how "heavy"
> this might be.

I just did some rough tests:
hackbench -s 512 -l 200 -g 15 -f 25 -P
Such line would hit arch_within_stack_frames at least 5000 times in my environment.
With hardened_usercopy=on, the execution time would be around 2.121 seconds(average for 30 times)
With hardened_usercopy=off, the execution time would be around 2.011 seconds(average for 30 times)

I'll test the original x86 way for arm64 tomorrow.

Any other workload needed to be run?

Thanks,
Zhe


>
> Thanks!
>
> -Kees
>

RE: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[David Laight]

> > Thanks for doing this implementation! One reason usercopy hardening
> > didn't persue doing a "full" stacktrace was because it seemed relatively
> > expensive. Did you do any usercopy-heavily workload testing to see if
> > there was a noticeable performance impact?

Look at anything that uses sendmsg().
It is noticeably more expensive than sendto().
All the extra copy_from_user() cause measurable slow slowdowns.
Using __copy_from_user()(to avoid 'hardending') in the socket code
and when reading the iov[] gives a measurable improvement.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[Mark Rutland]

Hi,

On Mon, Apr 18, 2022 at 09:22:11PM +0800, He Zhe wrote:
> This function checks if the given address range crosses frame boundary.

I don't think that's quite true, becuase arm64's procedure call standard
(AAPCS64) doesn't give us enough information to determine this without
additional metadata from the compiler, which we simply don't have today.

Since there's a lot of confusion in this area, I've made a bit of an info dump
below, before review on the patch itself, but TBH I'm struggling to see that
this is all that useful.

On arm64, we use a calling convention called AAPCS64, (in full: "Procedure Call
Standard for the Arm® 64-bit Architecture (AArch64)"). That's maintained at:

  https://github.com/ARM-software/abi-aa

... with the latest release (as of today) at:

  https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst
  https://github.com/ARM-software/abi-aa/releases/download/2022Q1/aapcs64.pdf

In AAPCS64, there are two related but distinct things to be aware of:

* The "stack frame" of a function, which is the entire contiguous region of
  stack memory used by a function.

* The "frame record", which is the saved FP and LR placed *somewhere* within
  the function's stack frame. The FP points at the most recent frame record on
  the stack, and at function call boundaries points at the caller's frame
  record.

AAPCS64 doesn't say *where* a frame record is placed within a stack frame, and
there are reasons for compilers to place above and below it. So in genral, a
functionss stack frame looks like:
      
        +=========+
        |  above  |
        |---------|
        | FP | LR |
        |---------|
        |  below  |
        +=========+

... where the "above" or "below" portions might be any size (even 0 bytes).

Typical code generation today means for most functions that the "below" portion
is 0 bytes in size, but this is not guaranteed, and even today there are cases
where this is not true.

When one function calls another without a stack transition, that looks like:

        +=========+ ___
        |  above  |    \
        |---------|    |
     ,->| FP | LR |    +-- Caller's stack frame
     |  |---------|    |
     |  |  below  | ___/
     |  +=========+ ___ 
     |  |  above  |    \
     |  |---------|    |
     '--| FP | LR |    +-- Callee's stack frame
        |---------|    |
        |  below  | ___/
        +=========+

Where there's a stack transition, and the new stack is at a *lower* VA than the
old stack, that looks like:

        +=========+ ___
        |  above  |    \
        |---------|    |
     ,->| FP | LR |    +-- Caller's stack frame
     |  |---------|    |
     |  |  below  | ___/
     |  +=========+
     | 
     |  ~~~~~~~~~~~
     |  Arbitrarily 
     |  large gap,
     |  potentially
     |  including
     |  other data
     |  ~~~~~~~~~~~
     |
     |  +=========+ ___ 
     |  |  above  |    \
     |  |---------|    |
     '--| FP | LR |    +-- Callee's stack frame
        |---------|    |
        |  below  | ___/
        +=========+

Where there's a stack transition, and the new stack is at a *higher* VA than
the old stack, that looks like:

        +=========+ ___ 
        |  above  |    \
        |---------|    |
     ,--| FP | LR |    +-- Callee's stack frame
     |  |---------|    |
     |  |  below  | ___/
     |  +=========+
     |
     |  ~~~~~~~~~~~
     |  Arbitrarily 
     |  large gap,
     |  potentially
     |  including
     |  other data
     |  ~~~~~~~~~~~
     | 
     |  +=========+ ___
     |  |  above  |    \
     |  |---------|    |
     '->| FP | LR |    +-- Caller's stack frame
        |---------|    |
        |  below  | ___/
        +=========+
 
In all of these cases, we *cannot* identify the boundary between the two stack
frames, we can *only* identify where something overlaps a frame record. That
might itself be a good thing, but it's not the same thing as what you describe
in the commit message.

> It is based on the existing x86 algorithm, but implemented via stacktrace.
> This can be tested by USERCOPY_STACK_FRAME_FROM and
> USERCOPY_STACK_FRAME_TO in lkdtm.

Can you please explain *why* we'd want this?

Who do we expect to use this?

What's the overhead in practice?

Has this passed a more realistic stress test (e.g. running some userspace
applications which make intensive use of copies to/from the kernel)?

> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/thread_info.h | 12 +++++
>  arch/arm64/kernel/stacktrace.c       | 76 ++++++++++++++++++++++++++--
>  3 files changed, 85 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 57c4c995965f..0f52a83d7771 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -165,6 +165,7 @@ config ARM64
>  	select HAVE_ARCH_TRACEHOOK
>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>  	select HAVE_ARCH_VMAP_STACK
> +	select HAVE_ARCH_WITHIN_STACK_FRAMES
>  	select HAVE_ARM_SMCCC
>  	select HAVE_ASM_MODVERSIONS
>  	select HAVE_EBPF_JIT
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index e1317b7c4525..b839ad9f2248 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -58,6 +58,18 @@ void arch_setup_new_exec(void);
>  void arch_release_task_struct(struct task_struct *tsk);
>  int arch_dup_task_struct(struct task_struct *dst,
>  				struct task_struct *src);
> +/*
> + * Walks up the stack frames to make sure that the specified object is
> + * entirely contained by a single stack frame.
> + *
> + * Returns:
> + *	GOOD_FRAME	if within a frame

As above, we cannot identify this reliably.

> + *	BAD_STACK	if placed across a frame boundary (or outside stack)
> + *	NOT_STACK	unable to determine (no frame pointers, etc)

On arm64 we always have frame pointers enabled, so this is a confusing comment.
Is this a copy-paste from x86?

> + */
> +int arch_within_stack_frames(const void * const stack,
> +		const void * const stackend,
> +		const void *obj, unsigned long len);
>  
>  #endif
>  
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e4103e085681..219b90c1de12 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -145,12 +145,17 @@ NOKPROBE_SYMBOL(unwind_frame);
>  
>  static void notrace walk_stackframe(struct task_struct *tsk,
>  				    struct stackframe *frame,
> -				    bool (*fn)(void *, unsigned long), void *data)
> +				    stack_trace_consume_fn fn, void *data)
>  {
> +	struct frame_info fi;
> +
>  	while (1) {
>  		int ret;
>  
> -		if (!fn(data, frame->pc))
> +		fi.pc = frame->pc;
> +		fi.fp = frame->fp;
> +		fi.prev_fp = frame->prev_fp;
> +		if (!fn(data, &fi))
>  			break;
>  		ret = unwind_frame(tsk, frame);
>  		if (ret < 0)

As on my prior comment, I don't think we want to alter our generic stack walker
in this way. If we need more info, I'd prefer to expose this in layers, keeping
arch_stack_walk unchanged, but having an arm64_stack_walk that can pass some
arm64-specific data.

> @@ -159,10 +164,10 @@ static void notrace walk_stackframe(struct task_struct
> *tsk,
>  }
>  NOKPROBE_SYMBOL(walk_stackframe);
>  
> -static bool dump_backtrace_entry(void *arg, unsigned long where)
> +static bool dump_backtrace_entry(void *arg, struct frame_info *fi)
>  {
>  	char *loglvl = arg;
> -	printk("%s %pSb\n", loglvl, (void *)where);
> +	printk("%s %pSb\n", loglvl, (void *)fi->pc);
>  	return true;
>  }
>  
> @@ -210,3 +215,66 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  
>  	walk_stackframe(task, &frame, consume_entry, cookie);
>  }
> +
> +struct arch_stack_object {
> +	unsigned long start;
> +	unsigned long len;
> +	int flag;

What is "flag" ?

> +};
> +
> +static bool arch_stack_object_check(void *data, struct frame_info *fi)
> +{
> +	struct arch_stack_object *obj = (struct arch_stack_object *)data;
> +
> +	/* Skip the frame of arch_within_stack_frames itself */
> +	if (fi->prev_fp == 0)
> +		return true;

That's not what this is skipping. The first time arch_stack_walk() is called,
it gives the PC of its caller (i.e. arch_within_stack_frames), and it's own
synthetic FP. The next time around it gives the FP of it's caller.

> +
> +	/*
> +	 * low ----------------------------------------------> high
> +	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> +	 *                     ^----------------^
> +	 *               allow copies only within here
> +	 */

This diagram is not valid for arm64. There is no "bp" or "ip", and each stack
frame can be laid out more arbitrarily relative to the frame record.

> +	if (obj->start + obj->len <= fi->fp) {
> +		obj->flag = obj->start >=
> +			fi->prev_fp + 2 * sizeof(void *) ?
> +			GOOD_FRAME : BAD_STACK;

This check is broken in several ways if there's a stack transition, since the
placement of fp and prev_fp is legitimately arbitrary.

This would also be a lot clearer if you bailed out early rather than nesting
checks.

The best thing you can realistically do is check that the object falls entirely
within a given stack, then check that that no frames intersect the object. You
don't need prev_fp for that, since you can just check each frame record in
turn, then bail out once the object has been passed (i.e. once we've hit the
correct stack, and either seen an FP above it or transitioned to another
stack).

> +		return false;
> +	} else
> +		return true;
> +}
> +
> +/*
> + * Walks up the stack frames to make sure that the specified object is
> + * entirely contained by a single stack frame.
> + *
> + * Returns:
> + *	GOOD_FRAME	if within a frame
> + *	BAD_STACK	if placed across a frame boundary (or outside stack)
> + *	NOT_STACK	unable to determine (no frame pointers, etc)
> + */

This is the exact same comment as in the header. My comments from there apply
here, and one of the two should disappear.

> +int arch_within_stack_frames(const void * const stack,
> +		const void * const stackend,
> +		const void *obj, unsigned long len)
> +{
> +#if defined(CONFIG_FRAME_POINTER)

As above, this *cannot* be selected on arm64.

> +	struct arch_stack_object object;
> +	struct pt_regs regs;
> +
> +	if (__builtin_frame_address(1) == 0)
> +		return NOT_STACK;

When do you expect this to happen?

> +
> +	object.start = (unsigned long)obj;
> +	object.len = len;
> +	object.flag = NOT_STACK;
> +
> +	regs.regs[29] = (u64)__builtin_frame_address(1);

NAK to making a synthetic pt_regs like this. That an abuse of the existing API,
and you don't need to do this in the first place.

> +
> +	arch_stack_walk(arch_stack_object_check, (void *)&object, NULL, &regs);

A void pointer cast is not necessary.

Thanks,
Mark.

> +
> +	return object.flag;
> +#else
> +	return NOT_STACK;
> +#endif
> +}
> -- 
> 2.25.1
> 

Re: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames

[He Zhe]

On 4/19/22 22:40, Mark Rutland wrote:
> Hi,
>
> On Mon, Apr 18, 2022 at 09:22:11PM +0800, He Zhe wrote:
>> This function checks if the given address range crosses frame boundary.
> I don't think that's quite true, becuase arm64's procedure call standard
> (AAPCS64) doesn't give us enough information to determine this without
> additional metadata from the compiler, which we simply don't have today.
>
> Since there's a lot of confusion in this area, I've made a bit of an info dump
> below, before review on the patch itself, but TBH I'm struggling to see that
> this is all that useful.

Thanks for the exhaustive explanation and info dump here. I've read through all
your comments, very helpful.

>
> On arm64, we use a calling convention called AAPCS64, (in full: "Procedure Call
> Standard for the Arm® 64-bit Architecture (AArch64)"). That's maintained at:
>
>   https://github.com/ARM-software/abi-aa
>
> ... with the latest release (as of today) at:
>
>   https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst
>   https://github.com/ARM-software/abi-aa/releases/download/2022Q1/aapcs64.pdf
>
> In AAPCS64, there are two related but distinct things to be aware of:
>
> * The "stack frame" of a function, which is the entire contiguous region of
>   stack memory used by a function.
>
> * The "frame record", which is the saved FP and LR placed *somewhere* within
>   the function's stack frame. The FP points at the most recent frame record on
>   the stack, and at function call boundaries points at the caller's frame
>   record.
>
> AAPCS64 doesn't say *where* a frame record is placed within a stack frame, and
> there are reasons for compilers to place above and below it. So in genral, a
> functionss stack frame looks like:
>       
>         +=========+
>         |  above  |
>         |---------|
>         | FP | LR |
>         |---------|
>         |  below  |
>         +=========+
>
> ... where the "above" or "below" portions might be any size (even 0 bytes).
>
> Typical code generation today means for most functions that the "below" portion
> is 0 bytes in size, but this is not guaranteed, and even today there are cases
> where this is not true.
>
> When one function calls another without a stack transition, that looks like:
>
>         +=========+ ___
>         |  above  |    \
>         |---------|    |
>      ,->| FP | LR |    +-- Caller's stack frame
>      |  |---------|    |
>      |  |  below  | ___/
>      |  +=========+ ___ 
>      |  |  above  |    \
>      |  |---------|    |
>      '--| FP | LR |    +-- Callee's stack frame
>         |---------|    |
>         |  below  | ___/
>         +=========+
>
> Where there's a stack transition, and the new stack is at a *lower* VA than the
> old stack, that looks like:
>
>         +=========+ ___
>         |  above  |    \
>         |---------|    |
>      ,->| FP | LR |    +-- Caller's stack frame
>      |  |---------|    |
>      |  |  below  | ___/
>      |  +=========+
>      | 
>      |  ~~~~~~~~~~~
>      |  Arbitrarily 
>      |  large gap,
>      |  potentially
>      |  including
>      |  other data
>      |  ~~~~~~~~~~~
>      |
>      |  +=========+ ___ 
>      |  |  above  |    \
>      |  |---------|    |
>      '--| FP | LR |    +-- Callee's stack frame
>         |---------|    |
>         |  below  | ___/
>         +=========+
>
> Where there's a stack transition, and the new stack is at a *higher* VA than
> the old stack, that looks like:
>
>         +=========+ ___ 
>         |  above  |    \
>         |---------|    |
>      ,--| FP | LR |    +-- Callee's stack frame
>      |  |---------|    |
>      |  |  below  | ___/
>      |  +=========+
>      |
>      |  ~~~~~~~~~~~
>      |  Arbitrarily 
>      |  large gap,
>      |  potentially
>      |  including
>      |  other data
>      |  ~~~~~~~~~~~
>      | 
>      |  +=========+ ___
>      |  |  above  |    \
>      |  |---------|    |
>      '->| FP | LR |    +-- Caller's stack frame
>         |---------|    |
>         |  below  | ___/
>         +=========+
>  
> In all of these cases, we *cannot* identify the boundary between the two stack
> frames, we can *only* identify where something overlaps a frame record. That
> might itself be a good thing, but it's not the same thing as what you describe
> in the commit message.
>
>> It is based on the existing x86 algorithm, but implemented via stacktrace.
>> This can be tested by USERCOPY_STACK_FRAME_FROM and
>> USERCOPY_STACK_FRAME_TO in lkdtm.
> Can you please explain *why* we'd want this?

We are trying to use the hardened usercopy feature on arm64 hardware and found
that the lkdtm can help validate the feature. But USERCOPY_STACK_FRAME_FROM/TO
checks, which were originally added for x86, are not supported for arm64. I
thought it would be good if we can enhance such hardening for arm64 and tried to
add the basic frame check like on x86 in this series. And yes, with all the arm64 details
provided above, this surely needs to be reconsidered.

>
> Who do we expect to use this?
>
> What's the overhead in practice?
>
> Has this passed a more realistic stress test (e.g. running some userspace
> applications which make intensive use of copies to/from the kernel)?

Just did some very rough performance impact test as in the other reply, But not
sure what needs to be done further.

Thanks,
Zhe

>
>> Signed-off-by: He Zhe <zhe.he@windriver.com>
>> ---
>>  arch/arm64/Kconfig                   |  1 +
>>  arch/arm64/include/asm/thread_info.h | 12 +++++
>>  arch/arm64/kernel/stacktrace.c       | 76 ++++++++++++++++++++++++++--
>>  3 files changed, 85 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 57c4c995965f..0f52a83d7771 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -165,6 +165,7 @@ config ARM64
>>  	select HAVE_ARCH_TRACEHOOK
>>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>>  	select HAVE_ARCH_VMAP_STACK
>> +	select HAVE_ARCH_WITHIN_STACK_FRAMES
>>  	select HAVE_ARM_SMCCC
>>  	select HAVE_ASM_MODVERSIONS
>>  	select HAVE_EBPF_JIT
>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>> index e1317b7c4525..b839ad9f2248 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -58,6 +58,18 @@ void arch_setup_new_exec(void);
>>  void arch_release_task_struct(struct task_struct *tsk);
>>  int arch_dup_task_struct(struct task_struct *dst,
>>  				struct task_struct *src);
>> +/*
>> + * Walks up the stack frames to make sure that the specified object is
>> + * entirely contained by a single stack frame.
>> + *
>> + * Returns:
>> + *	GOOD_FRAME	if within a frame
> As above, we cannot identify this reliably.
>
>> + *	BAD_STACK	if placed across a frame boundary (or outside stack)
>> + *	NOT_STACK	unable to determine (no frame pointers, etc)
> On arm64 we always have frame pointers enabled, so this is a confusing comment.
> Is this a copy-paste from x86?
>
>> + */
>> +int arch_within_stack_frames(const void * const stack,
>> +		const void * const stackend,
>> +		const void *obj, unsigned long len);
>>  
>>  #endif
>>  
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index e4103e085681..219b90c1de12 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -145,12 +145,17 @@ NOKPROBE_SYMBOL(unwind_frame);
>>  
>>  static void notrace walk_stackframe(struct task_struct *tsk,
>>  				    struct stackframe *frame,
>> -				    bool (*fn)(void *, unsigned long), void *data)
>> +				    stack_trace_consume_fn fn, void *data)
>>  {
>> +	struct frame_info fi;
>> +
>>  	while (1) {
>>  		int ret;
>>  
>> -		if (!fn(data, frame->pc))
>> +		fi.pc = frame->pc;
>> +		fi.fp = frame->fp;
>> +		fi.prev_fp = frame->prev_fp;
>> +		if (!fn(data, &fi))
>>  			break;
>>  		ret = unwind_frame(tsk, frame);
>>  		if (ret < 0)
> As on my prior comment, I don't think we want to alter our generic stack walker
> in this way. If we need more info, I'd prefer to expose this in layers, keeping
> arch_stack_walk unchanged, but having an arm64_stack_walk that can pass some
> arm64-specific data.
>
>> @@ -159,10 +164,10 @@ static void notrace walk_stackframe(struct task_struct
>> *tsk,
>>  }
>>  NOKPROBE_SYMBOL(walk_stackframe);
>>  
>> -static bool dump_backtrace_entry(void *arg, unsigned long where)
>> +static bool dump_backtrace_entry(void *arg, struct frame_info *fi)
>>  {
>>  	char *loglvl = arg;
>> -	printk("%s %pSb\n", loglvl, (void *)where);
>> +	printk("%s %pSb\n", loglvl, (void *)fi->pc);
>>  	return true;
>>  }
>>  
>> @@ -210,3 +215,66 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>>  
>>  	walk_stackframe(task, &frame, consume_entry, cookie);
>>  }
>> +
>> +struct arch_stack_object {
>> +	unsigned long start;
>> +	unsigned long len;
>> +	int flag;
> What is "flag" ?
>
>> +};
>> +
>> +static bool arch_stack_object_check(void *data, struct frame_info *fi)
>> +{
>> +	struct arch_stack_object *obj = (struct arch_stack_object *)data;
>> +
>> +	/* Skip the frame of arch_within_stack_frames itself */
>> +	if (fi->prev_fp == 0)
>> +		return true;
> That's not what this is skipping. The first time arch_stack_walk() is called,
> it gives the PC of its caller (i.e. arch_within_stack_frames), and it's own
> synthetic FP. The next time around it gives the FP of it's caller.
>
>> +
>> +	/*
>> +	 * low ----------------------------------------------> high
>> +	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> +	 *                     ^----------------^
>> +	 *               allow copies only within here
>> +	 */
> This diagram is not valid for arm64. There is no "bp" or "ip", and each stack
> frame can be laid out more arbitrarily relative to the frame record.
>
>> +	if (obj->start + obj->len <= fi->fp) {
>> +		obj->flag = obj->start >=
>> +			fi->prev_fp + 2 * sizeof(void *) ?
>> +			GOOD_FRAME : BAD_STACK;
> This check is broken in several ways if there's a stack transition, since the
> placement of fp and prev_fp is legitimately arbitrary.
>
> This would also be a lot clearer if you bailed out early rather than nesting
> checks.
>
> The best thing you can realistically do is check that the object falls entirely
> within a given stack, then check that that no frames intersect the object. You
> don't need prev_fp for that, since you can just check each frame record in
> turn, then bail out once the object has been passed (i.e. once we've hit the
> correct stack, and either seen an FP above it or transitioned to another
> stack).
>
>> +		return false;
>> +	} else
>> +		return true;
>> +}
>> +
>> +/*
>> + * Walks up the stack frames to make sure that the specified object is
>> + * entirely contained by a single stack frame.
>> + *
>> + * Returns:
>> + *	GOOD_FRAME	if within a frame
>> + *	BAD_STACK	if placed across a frame boundary (or outside stack)
>> + *	NOT_STACK	unable to determine (no frame pointers, etc)
>> + */
> This is the exact same comment as in the header. My comments from there apply
> here, and one of the two should disappear.
>
>> +int arch_within_stack_frames(const void * const stack,
>> +		const void * const stackend,
>> +		const void *obj, unsigned long len)
>> +{
>> +#if defined(CONFIG_FRAME_POINTER)
> As above, this *cannot* be selected on arm64.
>
>> +	struct arch_stack_object object;
>> +	struct pt_regs regs;
>> +
>> +	if (__builtin_frame_address(1) == 0)
>> +		return NOT_STACK;
> When do you expect this to happen?
>
>> +
>> +	object.start = (unsigned long)obj;
>> +	object.len = len;
>> +	object.flag = NOT_STACK;
>> +
>> +	regs.regs[29] = (u64)__builtin_frame_address(1);
> NAK to making a synthetic pt_regs like this. That an abuse of the existing API,
> and you don't need to do this in the first place.
>
>> +
>> +	arch_stack_walk(arch_stack_object_check, (void *)&object, NULL, &regs);
> A void pointer cast is not necessary.
>
> Thanks,
> Mark.
>
>> +
>> +	return object.flag;
>> +#else
>> +	return NOT_STACK;
>> +#endif
>> +}
>> -- 
>> 2.25.1
>>

[PATCH RFC 3/8] arm64: stacktrace: Make callbacks use new prototype with frame info

[He Zhe]

stack_trace_consume_fn has been changed to
bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
to be able to pass more information.

Turn to use pc in struct frame_info in arch_stack_walk callbacks without
functinoal change.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/arm64/kernel/perf_callchain.c | 4 ++--
 arch/arm64/kernel/process.c        | 6 +++---
 arch/arm64/kernel/return_address.c | 4 ++--
 arch/arm64/kernel/time.c           | 6 +++---
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 65b196e3ca6c..d32918b34c0c 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -132,10 +132,10 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
 	}
 }
 
-static bool callchain_trace(void *data, unsigned long pc)
+static bool callchain_trace(void *data, struct frame_info *fi)
 {
 	struct perf_callchain_entry_ctx *entry = data;
-	return perf_callchain_store(entry, pc) == 0;
+	return perf_callchain_store(entry, fi->pc) == 0;
 }
 
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 7fa97df55e3a..e96a8a614a1a 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -527,12 +527,12 @@ struct wchan_info {
 	int		count;
 };
 
-static bool get_wchan_cb(void *arg, unsigned long pc)
+static bool get_wchan_cb(void *arg, struct frame_info *fi)
 {
 	struct wchan_info *wchan_info = arg;
 
-	if (!in_sched_functions(pc)) {
-		wchan_info->pc = pc;
+	if (!in_sched_functions(fi->pc)) {
+		wchan_info->pc = fi->pc;
 		return false;
 	}
 	return wchan_info->count++ < 16;
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 68330017d04f..ec9be91623c4 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -18,12 +18,12 @@ struct return_address_data {
 	void *addr;
 };
 
-static bool save_return_addr(void *d, unsigned long pc)
+static bool save_return_addr(void *d, struct frame_info *fi)
 {
 	struct return_address_data *data = d;
 
 	if (!data->level) {
-		data->addr = (void *)pc;
+		data->addr = (void *)fi->pc;
 		return false;
 	} else {
 		--data->level;
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index b5855eb7435d..6b1a9de7c852 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -32,13 +32,13 @@
 #include <asm/thread_info.h>
 #include <asm/paravirt.h>
 
-static bool profile_pc_cb(void *arg, unsigned long pc)
+static bool profile_pc_cb(void *arg, struct frame_info *fi)
 {
 	unsigned long *prof_pc = arg;
 
-	if (in_lock_functions(pc))
+	if (in_lock_functions(fi->pc))
 		return true;
-	*prof_pc = pc;
+	*prof_pc = fi->pc;
 	return false;
 }
 
-- 
2.25.1

[PATCH RFC 4/8] powerpc: stacktrace: Make callbacks use new prototype with frame info

[He Zhe]

stack_trace_consume_fn has been changed to
bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
to be able to pass more information.

Turn to use pc in struct frame_info in arch_stack_walk callbacks without
functinoal change.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/powerpc/kernel/stacktrace.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index a2443d61728e..8640e9acfa29 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -28,9 +28,13 @@ void __no_sanitize_address arch_stack_walk(stack_trace_consume_fn consume_entry,
 					   struct task_struct *task, struct pt_regs *regs)
 {
 	unsigned long sp;
+	struct frame_info fi;
 
-	if (regs && !consume_entry(cookie, regs->nip))
-		return;
+	if (regs) {
+		fi.pc = regs->nip;
+		if (!consume_entry(cookie, &fi))
+			return;
+	}
 
 	if (regs)
 		sp = regs->gpr[1];
@@ -41,15 +45,15 @@ void __no_sanitize_address arch_stack_walk(stack_trace_consume_fn consume_entry,
 
 	for (;;) {
 		unsigned long *stack = (unsigned long *) sp;
-		unsigned long newsp, ip;
+		unsigned long newsp;
 
 		if (!validate_sp(sp, task, STACK_FRAME_OVERHEAD))
 			return;
 
 		newsp = stack[0];
-		ip = stack[STACK_FRAME_LR_SAVE];
+		fi.pc = stack[STACK_FRAME_LR_SAVE];
 
-		if (!consume_entry(cookie, ip))
+		if (!consume_entry(cookie, &fi))
 			return;
 
 		sp = newsp;
@@ -71,6 +75,7 @@ int __no_sanitize_address arch_stack_walk_reliable(stack_trace_consume_fn consum
 	unsigned long stack_end;
 	int graph_idx = 0;
 	bool firstframe;
+	struct frame_info fi;
 
 	stack_end = stack_page + THREAD_SIZE;
 	if (!is_idle_task(task)) {
@@ -159,7 +164,8 @@ int __no_sanitize_address arch_stack_walk_reliable(stack_trace_consume_fn consum
 			return -EINVAL;
 #endif
 
-		if (!consume_entry(cookie, ip))
+		fi.pc = ip;
+		if (!consume_entry(cookie, &fi))
 			return -EINVAL;
 	}
 	return 0;
-- 
2.25.1

[PATCH RFC 5/8] riscv: stacktrace: Make callbacks use new prototype with frame info

[He Zhe]

stack_trace_consume_fn has been changed to
bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
to be able to pass more information.

Turn to use pc in struct frame_info in arch_stack_walk callbacks without
functinoal change.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/riscv/include/asm/stacktrace.h |  2 +-
 arch/riscv/kernel/perf_callchain.c  |  4 ++--
 arch/riscv/kernel/stacktrace.c      | 22 +++++++++++++---------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index 3450c1912afd..486ed6a96b89 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -12,7 +12,7 @@ struct stackframe {
 };
 
 extern void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-				    bool (*fn)(void *, unsigned long), void *arg);
+				    stack_trace_consume_fn fn, void *arg);
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 			   const char *loglvl);
 
diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
index 3348a61de7d9..413abf094818 100644
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
@@ -66,9 +66,9 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
 		fp = user_backtrace(entry, fp, 0);
 }
 
-static bool fill_callchain(void *entry, unsigned long pc)
+static bool fill_callchain(void *entry, struct frame_info *fi)
 {
-	return perf_callchain_store(entry, pc) == 0;
+	return perf_callchain_store(entry, fi->pc) == 0;
 }
 
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 08d11a53f39e..9cc5013a5ea4 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -17,10 +17,11 @@
 #ifdef CONFIG_FRAME_POINTER
 
 void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-			     bool (*fn)(void *, unsigned long), void *arg)
+			     stack_trace_consume_fn fn, void *arg)
 {
 	unsigned long fp, sp, pc;
 	int level = 0;
+	struct frame_info fi;
 
 	if (regs) {
 		fp = frame_pointer(regs);
@@ -41,7 +42,8 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 		unsigned long low, high;
 		struct stackframe *frame;
 
-		if (unlikely(!__kernel_text_address(pc) || (level++ >= 1 && !fn(arg, pc))))
+		fi.pc = pc;
+		if (unlikely(!__kernel_text_address(pc) || (level++ >= 1 && !fn(arg, &fi))))
 			break;
 
 		/* Validate frame pointer */
@@ -67,10 +69,11 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 #else /* !CONFIG_FRAME_POINTER */
 
 void notrace walk_stackframe(struct task_struct *task,
-	struct pt_regs *regs, bool (*fn)(void *, unsigned long), void *arg)
+	struct pt_regs *regs, stack_trace_consume_fn fn, void *arg)
 {
 	unsigned long sp, pc;
 	unsigned long *ksp;
+	struct frame_info fi;
 
 	if (regs) {
 		sp = user_stack_pointer(regs);
@@ -89,7 +92,8 @@ void notrace walk_stackframe(struct task_struct *task,
 
 	ksp = (unsigned long *)sp;
 	while (!kstack_end(ksp)) {
-		if (__kernel_text_address(pc) && unlikely(!fn(arg, pc)))
+		fi.pc = pc;
+		if (__kernel_text_address(pc) && unlikely(!fn(arg, &fi)))
 			break;
 		pc = (*ksp++) - 0x4;
 	}
@@ -97,11 +101,11 @@ void notrace walk_stackframe(struct task_struct *task,
 
 #endif /* CONFIG_FRAME_POINTER */
 
-static bool print_trace_address(void *arg, unsigned long pc)
+static bool print_trace_address(void *arg, struct frame_info *fi)
 {
 	const char *loglvl = arg;
 
-	print_ip_sym(loglvl, pc);
+	print_ip_sym(loglvl, fi->pc);
 	return true;
 }
 
@@ -117,11 +121,11 @@ void show_stack(struct task_struct *task, unsigned long *sp, const char *loglvl)
 	dump_backtrace(NULL, task, loglvl);
 }
 
-static bool save_wchan(void *arg, unsigned long pc)
+static bool save_wchan(void *arg, struct frame_info *fi)
 {
-	if (!in_sched_functions(pc)) {
+	if (!in_sched_functions(fi->pc)) {
 		unsigned long *p = arg;
-		*p = pc;
+		*p = fi->pc;
 		return false;
 	}
 	return true;
-- 
2.25.1

[PATCH RFC 6/8] s390: stacktrace: Make callbacks use new prototype with frame info

[He Zhe]

stack_trace_consume_fn has been changed to
bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
to be able to pass more information.

Turn to use pc in struct frame_info in arch_stack_walk callbacks without
functinoal change.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/s390/kernel/stacktrace.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kernel/stacktrace.c b/arch/s390/kernel/stacktrace.c
index 7ee455e8e3d5..66aa7da0d974 100644
--- a/arch/s390/kernel/stacktrace.c
+++ b/arch/s390/kernel/stacktrace.c
@@ -14,11 +14,11 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 		     struct task_struct *task, struct pt_regs *regs)
 {
 	struct unwind_state state;
-	unsigned long addr;
+	struct frame_info fi;
 
 	unwind_for_each_frame(&state, task, regs, 0) {
-		addr = unwind_get_return_address(&state);
-		if (!addr || !consume_entry(cookie, addr))
+		fi.pc = unwind_get_return_address(&state);
+		if (!fi.pc || !consume_entry(cookie, &fi))
 			break;
 	}
 }
@@ -27,7 +27,7 @@ int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
 			     void *cookie, struct task_struct *task)
 {
 	struct unwind_state state;
-	unsigned long addr;
+	struct frame_info fi;
 
 	unwind_for_each_frame(&state, task, NULL, 0) {
 		if (state.stack_info.type != STACK_TYPE_TASK)
@@ -36,8 +36,8 @@ int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
 		if (state.regs)
 			return -EINVAL;
 
-		addr = unwind_get_return_address(&state);
-		if (!addr)
+		fi.pc = unwind_get_return_address(&state);
+		if (!fi.pc)
 			return -EINVAL;
 
 #ifdef CONFIG_KPROBES
@@ -49,7 +49,7 @@ int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
 			return -EINVAL;
 #endif
 
-		if (!consume_entry(cookie, addr))
+		if (!consume_entry(cookie, &fi))
 			return -EINVAL;
 	}
 
-- 
2.25.1

[PATCH RFC 7/8] x86: stacktrace: Make callbacks use new prototype with frame info

[He Zhe]

stack_trace_consume_fn has been changed to
bool (*stack_trace_consume_fn)(void *cookie, struct frame_info *fi);
to be able to pass more information.

Turn to use pc in struct frame_info in arch_stack_walk callbacks without
functinoal change.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 arch/x86/kernel/stacktrace.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index ee117fcf46ed..c88bfbb5f157 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -16,15 +16,18 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 		     struct task_struct *task, struct pt_regs *regs)
 {
 	struct unwind_state state;
-	unsigned long addr;
+	struct frame_info fi;
 
-	if (regs && !consume_entry(cookie, regs->ip))
-		return;
+	if (regs) {
+		fi.pc = regs->ip;
+		if (!consume_entry(cookie, &fi))
+			return;
+	}
 
 	for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
 	     unwind_next_frame(&state)) {
-		addr = unwind_get_return_address(&state);
-		if (!addr || !consume_entry(cookie, addr))
+		fi.pc = unwind_get_return_address(&state);
+		if (!fi.pc || !consume_entry(cookie, &fi))
 			break;
 	}
 }
@@ -34,7 +37,7 @@ int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
 {
 	struct unwind_state state;
 	struct pt_regs *regs;
-	unsigned long addr;
+	struct frame_info fi;
 
 	for (unwind_start(&state, task, NULL, NULL);
 	     !unwind_done(&state) && !unwind_error(&state);
@@ -56,17 +59,17 @@ int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
 				return -EINVAL;
 		}
 
-		addr = unwind_get_return_address(&state);
+		fi.pc = unwind_get_return_address(&state);
 
 		/*
 		 * A NULL or invalid return address probably means there's some
 		 * generated code which __kernel_text_address() doesn't know
 		 * about.
 		 */
-		if (!addr)
+		if (!fi.pc)
 			return -EINVAL;
 
-		if (!consume_entry(cookie, addr))
+		if (!consume_entry(cookie, &fi))
 			return -EINVAL;
 	}
 
@@ -107,8 +110,10 @@ void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
 			  const struct pt_regs *regs)
 {
 	const void __user *fp = (const void __user *)regs->bp;
+	struct frame_info fi;
 
-	if (!consume_entry(cookie, regs->ip))
+	fi.pc = regs->ip;
+	if (!consume_entry(cookie, &fi))
 		return;
 
 	while (1) {
@@ -122,7 +127,8 @@ void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
 			break;
 		if (!frame.ret_addr)
 			break;
-		if (!consume_entry(cookie, frame.ret_addr))
+		fi.pc = frame.ret_addr;
+		if (!consume_entry(cookie, &fi))
 			break;
 		fp = frame.next_fp;
 	}
-- 
2.25.1

[PATCH RFC 8/8] lkdtm: usercopy: Make USERCOPY_STACK_FRAME_x able to work for all archs

[He Zhe]

Currently the way to generate bad frame use depends on stack layout of the
architecture and compiler. It happens to work with x86 but does not work
with arm64 since it ruins the stack.

On x86, the original do_usercopy_stack_callee returns the start address of
its local buffer which is located deeply enough in the stack, so that the
following arch_within_stack_frames finds that the address is out of the
frame under check and gives the warning.

While on arm64, the local buffer of do_usercopy_stack_callee is just not
deep enough and happens to lands in the range of a frame so that
arch_within_stack_frames cannot detect it with current algorithm. And worse
in USERCOPY_STACK_FRAME_FROM, the following writing 0 to the formed buffer
just overwrites the return address of the call chain.

$ echo USERCOPY_STACK_FRAME_FROM > /sys/kernel/debug/provoke-crash/DIRECT
lkdtm: Performing direct entry USERCOPY_STACK_FRAME_FROM
lkdtm: attempting good copy_from_user of local stack
lkdtm: attempting bad copy_from_user of distant stack
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000

The detection algorithm may be improved in the future, but before that
let's tune the test case to validate current one. We can simply use
__builtin_frame_address(0) as a bad start address that can be detected for
all architectures.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 drivers/misc/lkdtm/usercopy.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c
index 9161ce7ed47a..000a1be15200 100644
--- a/drivers/misc/lkdtm/usercopy.c
+++ b/drivers/misc/lkdtm/usercopy.c
@@ -24,28 +24,6 @@ static struct kmem_cache *whitelist_cache;
 
 static const unsigned char test_text[] = "This is a test.\n";
 
-/*
- * Instead of adding -Wno-return-local-addr, just pass the stack address
- * through a function to obfuscate it from the compiler.
- */
-static noinline unsigned char *trick_compiler(unsigned char *stack)
-{
-	return stack + 0;
-}
-
-static noinline unsigned char *do_usercopy_stack_callee(int value)
-{
-	unsigned char buf[32];
-	int i;
-
-	/* Exercise stack to avoid everything living in registers. */
-	for (i = 0; i < sizeof(buf); i++) {
-		buf[i] = value & 0xff;
-	}
-
-	return trick_compiler(buf);
-}
-
 static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 {
 	unsigned long user_addr;
@@ -59,7 +37,7 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 
 	/* This is a pointer to outside our current stack frame. */
 	if (bad_frame) {
-		bad_stack = do_usercopy_stack_callee((uintptr_t)&bad_stack);
+		bad_stack = __builtin_frame_address(0);
 	} else {
 		/* Put start address just inside stack. */
 		bad_stack = task_stack_page(current) + THREAD_SIZE;
-- 
2.25.1