[08/11] arm64: unwind: strip PAC from kernel addresses

Commit Message

Amit Daniel Kachhap Oct. 17, 2019, 8:14 a.m. UTC

From: Kristina Martsenko <[e-mail]>

When we enable pointer authentication in the kernel, LR values saved to
the stack will have a PAC which we must strip in order to retrieve the
real return address.

Strip PACs when unwinding the stack in order to account for this.

Reviewed-by: Kees Cook <[e-mail]>
Signed-off-by: Mark Rutland <[e-mail]>
Signed-off-by: Kristina Martsenko <[e-mail]>
Signed-off-by: Amit Daniel Kachhap <[e-mail]>
---
Changes since RFC v2:
 - None

 arch/arm64/include/asm/pointer_auth.h | 9 ++++++---
 arch/arm64/kernel/stacktrace.c        | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

James Morse Oct. 23, 2019, 5:36 p.m. UTC | #1

Hi Amit,

On 17/10/2019 09:14, Amit Daniel Kachhap wrote:
> From: Kristina Martsenko <[e-mail]>
> 
> When we enable pointer authentication in the kernel, LR values saved to
> the stack will have a PAC which we must strip in order to retrieve the
> real return address.
> 
> Strip PACs when unwinding the stack in order to account for this.
> 
> Reviewed-by: Kees Cook <[e-mail]>
> Signed-off-by: Mark Rutland <[e-mail]>
> Signed-off-by: Kristina Martsenko <[e-mail]>
> Signed-off-by: Amit Daniel Kachhap <[e-mail]>

Sign-off chain Nit:
These Signed-off-by are supposed to be a chain of who handled the patch before it got to
Linus' tree. The first entry should match the 'From', the last should match the person
posting the patch.


I suspect the __builtin_return_address() patch should appear before this one, as
start_backtrace() callers pass that in as the first 'pc' value.

> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index 599dd09..a75dc89 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -59,12 +59,15 @@ extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg);
>   * The EL0 pointer bits used by a pointer authentication code.
>   * This is dependent on TBI0 being enabled, or bits 63:56 would also apply.

It might be worth updating the comment now we have the kernel version too.


>   */
> -#define ptrauth_user_pac_mask()	GENMASK(54, vabits_actual)
> +#define ptrauth_user_pac_mask()		GENMASK(54, vabits_actual)
> +#define ptrauth_kernel_pac_mask()	(GENMASK(63, 56) | GENMASK(54, VA_BITS))

(I see everywhere else we use GENMASK_ULL() for >32 bit values. It seems to work without it)


> -/* Only valid for EL0 TTBR0 instruction pointers */

Hmm, I suspect this is because the psuedo code's AArch64.BranchAddr removes Tags and PAC.
If you get a value from the LR, it should have been a PC, so it can't have a tag. It might
have been signed, so has a PAC that this function removes.

If you gave this a Tagged pointer, it would keep the tag. Is that intended?
(If not, can we fix the comment instead of removing it.)


>  static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr)
>  {
> -	return ptr & ~ptrauth_user_pac_mask();
> +	if (ptr & BIT_ULL(55))
> +		return ptr | ptrauth_kernel_pac_mask();
> +	else
> +		return ptr & ~ptrauth_user_pac_mask();
>  }
>  
>  #define ptrauth_thread_init_user(tsk)					\
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index a336cb1..49eb1c3 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -14,6 +14,7 @@
>  #include <linux/stacktrace.h>
>  
>  #include <asm/irq.h>
> +#include <asm/pointer_auth.h>
>  #include <asm/stack_pointer.h>
>  #include <asm/stacktrace.h>
>  
> @@ -84,6 +85,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	frame->prev_fp = fp;
>  	frame->prev_type = info.type;
>  
> +	frame->pc = ptrauth_strip_insn_pac(frame->pc);

Could this be against the frame->pc assignment? (Its evidently far enough away that diff
would trim this line out if someone adds something just after!)


Do you need to fixup __show_regs()? This reads regs->regs[30], and passes it to printk()s
%pS which will try to find the entry in kallsyms.


Thanks,

James

Amit Daniel Kachhap Oct. 30, 2019, 4:02 a.m. UTC | #2

Hi James,

On 10/23/19 11:06 PM, James Morse wrote:
> Hi Amit,
> 
> On 17/10/2019 09:14, Amit Daniel Kachhap wrote:
>> From: Kristina Martsenko <[e-mail]>
>>
>> When we enable pointer authentication in the kernel, LR values saved to
>> the stack will have a PAC which we must strip in order to retrieve the
>> real return address.
>>
>> Strip PACs when unwinding the stack in order to account for this.
>>
>> Reviewed-by: Kees Cook <[e-mail]>
>> Signed-off-by: Mark Rutland <[e-mail]>
>> Signed-off-by: Kristina Martsenko <[e-mail]>
>> Signed-off-by: Amit Daniel Kachhap <[e-mail]>
> 
> Sign-off chain Nit:
> These Signed-off-by are supposed to be a chain of who handled the patch before it got to
> Linus' tree. The first entry should match the 'From', the last should match the person
> posting the patch.
ok will do.
> 
> 
> I suspect the __builtin_return_address() patch should appear before this one, as
> start_backtrace() callers pass that in as the first 'pc' value.
> 
>> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
>> index 599dd09..a75dc89 100644
>> --- a/arch/arm64/include/asm/pointer_auth.h
>> +++ b/arch/arm64/include/asm/pointer_auth.h
>> @@ -59,12 +59,15 @@ extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg);
>>    * The EL0 pointer bits used by a pointer authentication code.
>>    * This is dependent on TBI0 being enabled, or bits 63:56 would also apply.
> 
> It might be worth updating the comment now we have the kernel version too.
ok.
> 
> 
>>    */
>> -#define ptrauth_user_pac_mask()	GENMASK(54, vabits_actual)
>> +#define ptrauth_user_pac_mask()		GENMASK(54, vabits_actual)
>> +#define ptrauth_kernel_pac_mask()	(GENMASK(63, 56) | GENMASK(54, VA_BITS))
> 
> (I see everywhere else we use GENMASK_ULL() for >32 bit values. It seems to work without it)
ok.
> 
> 
>> -/* Only valid for EL0 TTBR0 instruction pointers */
> 
> Hmm, I suspect this is because the psuedo code's AArch64.BranchAddr removes Tags and PAC.
> If you get a value from the LR, it should have been a PC, so it can't have a tag. It might
> have been signed, so has a PAC that this function removes.
yes.
> 
> If you gave this a Tagged pointer, it would keep the tag. Is that intended?
> (If not, can we fix the comment instead of removing it.)
I will fix the comment.
> 
> 
>>   static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr)
>>   {
>> -	return ptr & ~ptrauth_user_pac_mask();
>> +	if (ptr & BIT_ULL(55))
>> +		return ptr | ptrauth_kernel_pac_mask();
>> +	else
>> +		return ptr & ~ptrauth_user_pac_mask();
>>   }
>>   
>>   #define ptrauth_thread_init_user(tsk)					\
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index a336cb1..49eb1c3 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/stacktrace.h>
>>   
>>   #include <asm/irq.h>
>> +#include <asm/pointer_auth.h>
>>   #include <asm/stack_pointer.h>
>>   #include <asm/stacktrace.h>
>>   
>> @@ -84,6 +85,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>   	frame->prev_fp = fp;
>>   	frame->prev_type = info.type;
>>   
>> +	frame->pc = ptrauth_strip_insn_pac(frame->pc);
> 
> Could this be against the frame->pc assignment? (Its evidently far enough away that diff
> would trim this line out if someone adds something just after!)
Yes there is some re-assignment later. I will check this one.
> 
> 
> Do you need to fixup __show_regs()? This reads regs->regs[30], and passes it to printk()s
> %pS which will try to find the entry in kallsyms.
Good pointer. I will check it.

Thanks,
Amit Daniel

> 
> 
> Thanks,
> 
> James
>

Patch

diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
index 599dd09..a75dc89 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -59,12 +59,15 @@  extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg);
  * The EL0 pointer bits used by a pointer authentication code.
  * This is dependent on TBI0 being enabled, or bits 63:56 would also apply.
  */
-#define ptrauth_user_pac_mask()	GENMASK(54, vabits_actual)
+#define ptrauth_user_pac_mask()		GENMASK(54, vabits_actual)
+#define ptrauth_kernel_pac_mask()	(GENMASK(63, 56) | GENMASK(54, VA_BITS))
 
-/* Only valid for EL0 TTBR0 instruction pointers */
 static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr)
 {
-	return ptr & ~ptrauth_user_pac_mask();
+	if (ptr & BIT_ULL(55))
+		return ptr | ptrauth_kernel_pac_mask();
+	else
+		return ptr & ~ptrauth_user_pac_mask();
 }
 
 #define ptrauth_thread_init_user(tsk)					\
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index a336cb1..49eb1c3 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -14,6 +14,7 @@ 
 #include <linux/stacktrace.h>
 
 #include <asm/irq.h>
+#include <asm/pointer_auth.h>
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
 
@@ -84,6 +85,8 @@  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	frame->prev_fp = fp;
 	frame->prev_type = info.type;
 
+	frame->pc = ptrauth_strip_insn_pac(frame->pc);
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (tsk->ret_stack &&
 			(frame->pc == (unsigned long)return_to_handler)) {