diff mbox series

[v2,9/9] ARM: KCFI: Allow permissive CFI mode

Message ID 20240307-arm32-cfi-v2-9-cc74ea0306b3@linaro.org (mailing list archive)
State New, archived
Headers show
Series CFI for ARM32 using LLVM | expand

Commit Message

Linus Walleij March 7, 2024, 2:22 p.m. UTC
This registers a breakpoint handler for the new breakpoint type
(0x03) inserted by LLVM CLANG for CFI breakpoints.

If we are in permissive mode, just print a backtrace and continue.

Example with CONFIG_CFI_PERMISSIVE enabled:

root@Vexpress:/ echo CFI_FORWARD_PROTO > /sys/kernel/debug/provoke-crash/DIRECT
lkdtm: Performing direct entry CFI_FORWARD_PROTO
lkdtm: Calling matched prototype ...
lkdtm: Calling mismatched prototype ...
hw-breakpoint: Permissive CFI breakpoint
CPU: 0 PID: 114 Comm: sh Not tainted 6.8.0-rc1+ #111
Hardware name: ARM-Versatile Express
 unwind_backtrace from show_stack+0x28/0x30
 (...)
lkdtm: FAIL: survived mismatched prototype function call!
lkdtm: Unexpected! This kernel (6.8.0-rc1+ armv7l) was
       built with CONFIG_CFI_CLANG=y

As you can see the LKDTM test fails, but I expect that this would be
expected behaviour in the permissive mode.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/include/asm/hw_breakpoint.h |  1 +
 arch/arm/kernel/hw_breakpoint.c      | 10 ++++++++++
 2 files changed, 11 insertions(+)

Comments

Kees Cook March 7, 2024, 6:58 p.m. UTC | #1
On Thu, Mar 07, 2024 at 03:22:08PM +0100, Linus Walleij wrote:
> This registers a breakpoint handler for the new breakpoint type
> (0x03) inserted by LLVM CLANG for CFI breakpoints.
> 
> If we are in permissive mode, just print a backtrace and continue.
> 
> Example with CONFIG_CFI_PERMISSIVE enabled:
> 
> root@Vexpress:/ echo CFI_FORWARD_PROTO > /sys/kernel/debug/provoke-crash/DIRECT
> lkdtm: Performing direct entry CFI_FORWARD_PROTO
> lkdtm: Calling matched prototype ...
> lkdtm: Calling mismatched prototype ...
> hw-breakpoint: Permissive CFI breakpoint
> CPU: 0 PID: 114 Comm: sh Not tainted 6.8.0-rc1+ #111
> Hardware name: ARM-Versatile Express
>  unwind_backtrace from show_stack+0x28/0x30
>  (...)
> lkdtm: FAIL: survived mismatched prototype function call!
> lkdtm: Unexpected! This kernel (6.8.0-rc1+ armv7l) was
>        built with CONFIG_CFI_CLANG=y
> 
> As you can see the LKDTM test fails, but I expect that this would be
> expected behaviour in the permissive mode.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  arch/arm/include/asm/hw_breakpoint.h |  1 +
>  arch/arm/kernel/hw_breakpoint.c      | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
> index 62358d3ca0a8..e7f9961c53b2 100644
> --- a/arch/arm/include/asm/hw_breakpoint.h
> +++ b/arch/arm/include/asm/hw_breakpoint.h
> @@ -84,6 +84,7 @@ static inline void decode_ctrl_reg(u32 reg,
>  #define ARM_DSCR_MOE(x)			((x >> 2) & 0xf)
>  #define ARM_ENTRY_BREAKPOINT		0x1
>  #define ARM_ENTRY_ASYNC_WATCHPOINT	0x2
> +#define ARM_ENTRY_CFI_BREAKPOINT	0x3
>  #define ARM_ENTRY_SYNC_WATCHPOINT	0xa
>  
>  /* DSCR monitor/halting bits. */
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index dc0fb7a81371..256146684813 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -932,6 +932,16 @@ static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
>  	case ARM_ENTRY_SYNC_WATCHPOINT:
>  		watchpoint_handler(addr, fsr, regs);
>  		break;
> +	case ARM_ENTRY_CFI_BREAKPOINT:
> +		if (IS_ENABLED(CONFIG_CFI_PERMISSIVE)) {
> +			pr_err("Permissive CFI breakpoint\n");
> +			dump_stack();
> +			/* Skip the breaking instruction */

Instead of open-coding this, can you make a call to report_cfi_failure()
instead? This will keep the failure output the same across
architectures. I think it would look something like:

		if (report_cfi_failure(regs, addr, ...) == BUG_TRAP_TYPE_WARN)
			instruction_pointer(regs) += 4;
		else
			die("Oops - CFI", regs, 0);

-Kees
diff mbox series

Patch

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index 62358d3ca0a8..e7f9961c53b2 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -84,6 +84,7 @@  static inline void decode_ctrl_reg(u32 reg,
 #define ARM_DSCR_MOE(x)			((x >> 2) & 0xf)
 #define ARM_ENTRY_BREAKPOINT		0x1
 #define ARM_ENTRY_ASYNC_WATCHPOINT	0x2
+#define ARM_ENTRY_CFI_BREAKPOINT	0x3
 #define ARM_ENTRY_SYNC_WATCHPOINT	0xa
 
 /* DSCR monitor/halting bits. */
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index dc0fb7a81371..256146684813 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -932,6 +932,16 @@  static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
 	case ARM_ENTRY_SYNC_WATCHPOINT:
 		watchpoint_handler(addr, fsr, regs);
 		break;
+	case ARM_ENTRY_CFI_BREAKPOINT:
+		if (IS_ENABLED(CONFIG_CFI_PERMISSIVE)) {
+			pr_err("Permissive CFI breakpoint\n");
+			dump_stack();
+			/* Skip the breaking instruction */
+			instruction_pointer(regs) += 4;
+		} else {
+			die("Oops - CFI", regs, 0);
+		}
+		break;
 	default:
 		ret = 1; /* Unhandled fault. */
 	}