diff mbox series

[v6,10/11] ARM: hw_breakpoint: Handle CFI breakpoints

Message ID 20240417-arm32-cfi-v6-10-6486385eb136@linaro.org (mailing list archive)
State New, archived
Headers show
Series CFI for ARM32 using LLVM | expand

Commit Message

Linus Walleij April 17, 2024, 8:30 a.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:

> 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 ...
CFI failure at lkdtm_indirect_call+0x40/0x4c (target: 0x0; expected type: 0x00000000)
WARNING: CPU: 1 PID: 112 at lkdtm_indirect_call+0x40/0x4c
CPU: 1 PID: 112 Comm: sh Not tainted 6.8.0-rc1+ #150
Hardware name: ARM-Versatile Express
(...)
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.

We are currently not implementing target and type for the CFI
breakpoint as this requires additional operand bundling compiler
extensions.

CPUs without breakpoint support cannot handle breakpoints naturally,
in these cases the permissive mode will not work, CFI will fall over
on an undefined instruction:

Internal error: Oops - undefined instruction: 0 [#1] PREEMPT ARM
CPU: 0 PID: 186 Comm: ash Tainted: G        W          6.9.0-rc1+ #7
Hardware name: Gemini (Device Tree)
PC is at lkdtm_indirect_call+0x38/0x4c
LR is at lkdtm_CFI_FORWARD_PROTO+0x30/0x6c

This is reasonable I think: it's the best CFI can do to ascertain
the the control flow is not broken on these CPUs.

Reviewed-by: Kees Cook <keescook@chromium.org>
Tested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/include/asm/hw_breakpoint.h |  1 +
 arch/arm/kernel/hw_breakpoint.c      | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

Comments

Sami Tolvanen April 18, 2024, 4:12 p.m. UTC | #1
Hi Linus,

On Wed, Apr 17, 2024 at 1:31 AM Linus Walleij <linus.walleij@linaro.org> 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:
>
> > 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 ...
> CFI failure at lkdtm_indirect_call+0x40/0x4c (target: 0x0; expected type: 0x00000000)
> WARNING: CPU: 1 PID: 112 at lkdtm_indirect_call+0x40/0x4c
> CPU: 1 PID: 112 Comm: sh Not tainted 6.8.0-rc1+ #150
> Hardware name: ARM-Versatile Express
> (...)
> 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.
>
> We are currently not implementing target and type for the CFI
> breakpoint as this requires additional operand bundling compiler
> extensions.
>
> CPUs without breakpoint support cannot handle breakpoints naturally,
> in these cases the permissive mode will not work, CFI will fall over
> on an undefined instruction:
>
> Internal error: Oops - undefined instruction: 0 [#1] PREEMPT ARM
> CPU: 0 PID: 186 Comm: ash Tainted: G        W          6.9.0-rc1+ #7
> Hardware name: Gemini (Device Tree)
> PC is at lkdtm_indirect_call+0x38/0x4c
> LR is at lkdtm_CFI_FORWARD_PROTO+0x30/0x6c
>
> This is reasonable I think: it's the best CFI can do to ascertain
> the the control flow is not broken on these CPUs.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  arch/arm/include/asm/hw_breakpoint.h |  1 +
>  arch/arm/kernel/hw_breakpoint.c      | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 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..ce7c152dd6e9 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -17,6 +17,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/hw_breakpoint.h>
>  #include <linux/smp.h>
> +#include <linux/cfi.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/coresight.h>
>
> @@ -903,6 +904,32 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
>         watchpoint_single_step_handler(addr);
>  }
>
> +#ifdef CONFIG_CFI_CLANG
> +static void hw_breakpoint_cfi_handler(struct pt_regs *regs)
> +{
> +       /* TODO: implementing target and type requires compiler work */
> +       unsigned long target = 0;
> +       u32 type = 0;
> +
> +       switch (report_cfi_failure(regs, instruction_pointer(regs), &target, type)) {

Nit: To make the error message a bit cleaner, you can use
report_cfi_failure_noaddr(...) instead, and maybe you can expand the
comment to explain why target information isn't trivially available
right now?

Sami
Linus Walleij April 19, 2024, 12:56 p.m. UTC | #2
On Thu, Apr 18, 2024 at 6:13 PM Sami Tolvanen <samitolvanen@google.com> wrote:

> > +       switch (report_cfi_failure(regs, instruction_pointer(regs), &target, type)) {
>
> Nit: To make the error message a bit cleaner, you can use
> report_cfi_failure_noaddr(...) instead,

OK, fixed it!

> and maybe you can expand the
> comment to explain why target information isn't trivially available
> right now?

Sure, but I guess I would need you to explain it to me so I don't get
it wrong :D

Is it correct to say:

"TODO: To be able to properly extract target information the compiler
needs to be extended with operand bundling lowering into the 32-bit
ARM targets, and currently no compiler has implemented this."

?

Yours,
Linus Walleij
Sami Tolvanen April 19, 2024, 9:25 p.m. UTC | #3
On Fri, Apr 19, 2024 at 5:56 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 18, 2024 at 6:13 PM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> > > +       switch (report_cfi_failure(regs, instruction_pointer(regs), &target, type)) {
> >
> > Nit: To make the error message a bit cleaner, you can use
> > report_cfi_failure_noaddr(...) instead,
>
> OK, fixed it!
>
> > and maybe you can expand the
> > comment to explain why target information isn't trivially available
> > right now?
>
> Sure, but I guess I would need you to explain it to me so I don't get
> it wrong :D
>
> Is it correct to say:
>
> "TODO: To be able to properly extract target information the compiler
> needs to be extended with operand bundling lowering into the 32-bit
> ARM targets, and currently no compiler has implemented this."
>
> ?

I think operand bundles are specific to the LLVM implementation, so
they're probably not worth mentioning. I would just mention that the
reason we can't trivially figure out the target address and the
expected type hash when handling KCFI traps on 32-bit ARM is that the
current compilers don't generate a stable instruction sequence for
KCFI checks that would allow us to decode the instructions preceding
the trap and look up which registers were used.

Sami
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..ce7c152dd6e9 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -17,6 +17,7 @@ 
 #include <linux/perf_event.h>
 #include <linux/hw_breakpoint.h>
 #include <linux/smp.h>
+#include <linux/cfi.h>
 #include <linux/cpu_pm.h>
 #include <linux/coresight.h>
 
@@ -903,6 +904,32 @@  static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
 	watchpoint_single_step_handler(addr);
 }
 
+#ifdef CONFIG_CFI_CLANG
+static void hw_breakpoint_cfi_handler(struct pt_regs *regs)
+{
+	/* TODO: implementing target and type requires compiler work */
+	unsigned long target = 0;
+	u32 type = 0;
+
+	switch (report_cfi_failure(regs, instruction_pointer(regs), &target, type)) {
+	case BUG_TRAP_TYPE_BUG:
+		die("Oops - CFI", regs, 0);
+		break;
+	case BUG_TRAP_TYPE_WARN:
+		/* Skip the breaking instruction */
+		instruction_pointer(regs) += 4;
+		break;
+	default:
+		die("Unknown CFI error", regs, 0);
+		break;
+	}
+}
+#else
+static void hw_breakpoint_cfi_handler(struct pt_regs *regs)
+{
+}
+#endif
+
 /*
  * Called from either the Data Abort Handler [watchpoint] or the
  * Prefetch Abort Handler [breakpoint] with interrupts disabled.
@@ -932,6 +959,9 @@  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:
+		hw_breakpoint_cfi_handler(regs);
+		break;
 	default:
 		ret = 1; /* Unhandled fault. */
 	}