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

From: Kees Cook <keescook@chromium.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King <linux@armlinux.org.uk>,
	Sami Tolvanen <samitolvanen@google.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Ard Biesheuvel <ardb@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	linux-arm-kernel@lists.infradead.org, llvm@lists.linux.dev
Subject: Re: [PATCH v2 9/9] ARM: KCFI: Allow permissive CFI mode
Date: Thu, 7 Mar 2024 10:58:25 -0800	[thread overview]
Message-ID: <202403071002.542D167D65@keescook> (raw)
In-Reply-To: <20240307-arm32-cfi-v2-9-cc74ea0306b3@linaro.org>

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

-- 
Kees Cook

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