Hi,

On 9/2/20 9:09 PM, Dave Martin wrote:

On Tue, Aug 18, 2020 at 12:41:06PM +0530, Amit Daniel Kachhap wrote:

quoted

Currently the ARMv8.3-PAuth combined branch instructions (braa, retaa
etc.) are not simulated. However the uprobe of such instructions leads
to kernel warnings in a loop as they are not explicitly checked.

This is fixed by adding definitions of all such instructions and
rejecting their probes.

For my understanding, this patch addresses that fact that uprobes
expects the pc to be at xol_addr + 4 after a stepped instruction, except
for branch instructions that it explicitly knows about?

If so, this indeed makes sense as a fix to the existing code.

ok

LDRAA/LDRAB are not affected by this, right?

I'm wondering what happens if these generate an FPAC exception rather
than the data abort that would be expected in v8.3.  I think the ptrauth
enhancements allow this, but don't require it.

LDRAA/LDRAB can be simulated by uprobe. As per the Arm Arm(C6.2.134) for 
LDRAA/LDRBB,
"If the authentication fails, a Translation fault is generated"

This is similar to ldr behaviour so did't do anything for those 
instructions. Although, I should explicitly mention it.

If uprobes can't cope with SIGILLs in xol instructions then we probably
have bigger problems.

I _think_ the logic in handle_singlestep(), uprobe_deny_signal(),
arch_uprobe_xol_was_trapped() etc. does handle this scenario, but I
confess I don't fully understand all the details of it.

I verified with a SIGILL generating xol instruction and it behaved 
properly with a SIGILL signal so probably it is fine.

Modulo that:

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Thanks,
Amit

Cheers
---Dave

quoted

Warning log:
  WARNING: CPU: 5 PID: 249 at arch/arm64/kernel/probes/uprobes.c:182 uprobe_single_step_handler+0x34/0x50
  Modules linked in:
  CPU: 5 PID: 249 Comm: func Tainted: G        W         5.8.0-rc4-00005-ge658591d66d1-dirty #160
  Hardware name: Foundation-v8A (DT)
  pstate: 204003c9 (nzCv DAIF +PAN -UAO BTYPE=--)
  pc : uprobe_single_step_handler+0x34/0x50
  lr : single_step_handler+0x70/0xf8
  sp : ffff800012afbe30
  x29: ffff800012afbe30 x28: ffff000879f00ec0
  x27: 0000000000000000 x26: 0000000000000000
  x25: 0000000000000000 x24: 0000000000000000
  x23: 0000000060001000 x22: 00000000cb000022
  x21: ffff800011fc5a68 x20: ffff800012afbec0
  x19: ffff800011fc86c0 x18: 0000000000000000
  x17: 0000000000000000 x16: 0000000000000000
  x15: 0000000000000000 x14: 0000000000000000
  x13: 0000000000000000 x12: 0000000000000000
  x11: 0000000000000000 x10: 0000000000000000
  x9 : ffff800010085d50 x8 : 0000000000000000
  x7 : 0000000000000000 x6 : ffff800011fba9c0
  x5 : ffff800011fba000 x4 : ffff800012283070
  x3 : ffff8000100a78e0 x2 : 00000000004005f0
  x1 : 0000fffffffff008 x0 : ffff800012afbec0
  Call trace:
   uprobe_single_step_handler+0x34/0x50
   single_step_handler+0x70/0xf8
   do_debug_exception+0xb8/0x130
   el0_sync_handler+0x7c/0x188
   el0_sync+0x158/0x180

Fixes: 74afda4016a7 ("arm64: compile the kernel with ptrauth return address signing")
Fixes: 04ca3204fa09 ("arm64: enable pointer authentication")
Signed-off-by: Amit Daniel Kachhap <redacted>
---
Changes since v4:
* New patch.

  arch/arm64/include/asm/insn.h          | 12 ++++++++++++
  arch/arm64/kernel/insn.c               | 14 ++++++++++++--
  arch/arm64/kernel/probes/decode-insn.c |  4 +++-
  3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 0bc46149e491..324234068fee 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h

@@ -359,9 +359,21 @@ __AARCH64_INSN_FUNCS(brk,	0xFFE0001F, 0xD4200000)
  __AARCH64_INSN_FUNCS(exception,	0xFF000000, 0xD4000000)
  __AARCH64_INSN_FUNCS(hint,	0xFFFFF01F, 0xD503201F)
  __AARCH64_INSN_FUNCS(br,	0xFFFFFC1F, 0xD61F0000)
+__AARCH64_INSN_FUNCS(braaz,	0xFFFFFC1F, 0xD61F081F)
+__AARCH64_INSN_FUNCS(brabz,	0xFFFFFC1F, 0xD61F0C1F)
+__AARCH64_INSN_FUNCS(braa,	0xFFFFFC00, 0xD71F0800)
+__AARCH64_INSN_FUNCS(brab,	0xFFFFFC00, 0xD71F0C00)
  __AARCH64_INSN_FUNCS(blr,	0xFFFFFC1F, 0xD63F0000)
+__AARCH64_INSN_FUNCS(blraaz,	0xFFFFFC1F, 0xD63F081F)
+__AARCH64_INSN_FUNCS(blrabz,	0xFFFFFC1F, 0xD63F0C1F)
+__AARCH64_INSN_FUNCS(blraa,	0xFFFFFC00, 0xD73F0800)
+__AARCH64_INSN_FUNCS(blrab,	0xFFFFFC00, 0xD73F0C00)
  __AARCH64_INSN_FUNCS(ret,	0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(retaa,	0xFFFFFFFF, 0xD65F0BFF)
+__AARCH64_INSN_FUNCS(retab,	0xFFFFFFFF, 0xD65F0FFF)
  __AARCH64_INSN_FUNCS(eret,	0xFFFFFFFF, 0xD69F03E0)
+__AARCH64_INSN_FUNCS(eretaa,	0xFFFFFFFF, 0xD69F0BFF)
+__AARCH64_INSN_FUNCS(eretab,	0xFFFFFFFF, 0xD69F0FFF)
  __AARCH64_INSN_FUNCS(mrs,	0xFFF00000, 0xD5300000)
  __AARCH64_INSN_FUNCS(msr_imm,	0xFFF8F01F, 0xD500401F)
  __AARCH64_INSN_FUNCS(msr_reg,	0xFFF00000, 0xD5100000)

diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index a107375005bc..27d5a52d5058 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c

@@ -176,7 +176,7 @@ bool __kprobes aarch64_insn_uses_literal(u32 insn)
  
  bool __kprobes aarch64_insn_is_branch(u32 insn)
  {
-	/* b, bl, cb*, tb*, b.cond, br, blr */
+	/* b, bl, cb*, tb*, ret*, b.cond, br*, blr* */
  
  	return aarch64_insn_is_b(insn) ||
  		aarch64_insn_is_bl(insn) ||

@@ -185,9 +185,19 @@ bool __kprobes aarch64_insn_is_branch(u32 insn)
  		aarch64_insn_is_tbz(insn) ||
  		aarch64_insn_is_tbnz(insn) ||
  		aarch64_insn_is_ret(insn) ||
+		aarch64_insn_is_retaa(insn) ||
+		aarch64_insn_is_retab(insn) ||
  		aarch64_insn_is_br(insn) ||
  		aarch64_insn_is_blr(insn) ||
-		aarch64_insn_is_bcond(insn);
+		aarch64_insn_is_bcond(insn) ||
+		aarch64_insn_is_braaz(insn) ||
+		aarch64_insn_is_brabz(insn) ||
+		aarch64_insn_is_braa(insn) ||
+		aarch64_insn_is_brab(insn) ||
+		aarch64_insn_is_blraaz(insn) ||
+		aarch64_insn_is_blrabz(insn) ||
+		aarch64_insn_is_blraa(insn) ||
+		aarch64_insn_is_blrab(insn);
  }
  
  int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)

diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c
index 263d5fba4c8a..f9eb8210d6d3 100644
--- a/arch/arm64/kernel/probes/decode-insn.c
+++ b/arch/arm64/kernel/probes/decode-insn.c

@@ -29,7 +29,9 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn)
  		    aarch64_insn_is_msr_imm(insn) ||
  		    aarch64_insn_is_msr_reg(insn) ||
  		    aarch64_insn_is_exception(insn) ||
-		    aarch64_insn_is_eret(insn))
+		    aarch64_insn_is_eret(insn) ||
+		    aarch64_insn_is_eretaa(insn) ||
+		    aarch64_insn_is_eretab(insn))
  			return false;
  
  		/*

-- 
2.17.1


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

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