Re: [PATCH] cfi: rust: pass -Zpatchable-function-entry on all architectures

[Mark Rutland <mark.rutland@arm.com>]

Hi Alice,

On Tue, Oct 08, 2024 at 05:37:16PM +0000, Alice Ryhl wrote:
> The KCFI sanitizer stores the CFI tag of a function just before its
> machine code. However, the patchable-function-entry flag can be used to
> introduce additional nop instructions before the machine code, taking up
> the space that normally holds the CFI tag.

To clarify, when you say "before the machine code", do you mean when
NOPs are placed before the function entry point? e.g. if we compiled
with -fpatchable-function-entry=M,N where N > 0? I'll refer tho this as
"pre-function NOPs" below.

There's an existing incompatibility between CFI and pre-function NOPs
for C code, because we override -fpatchable-function-entry on a
per-function basis (e.g. for noinstr and notrace), and we don't
currently have a mechanism to ensure the CFI tag is in the same place
regardless. This is why arm64 has CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
depend on !CFI.

For C code at least, just using regular -fpatchable-function-entry=M or
-fpatchable-function-entry=M,0 shouldn't change the location of the CFI
tag relative to the function entrypoint, and so should have no adverse
effect on CFI.

Is Rust any different here?

> In this case, a backwards offset is applied to the CFI tag to move
> them out of the way of the nop instructions. To ensure that C and Rust
> agree on the offset used by CFI tags, pass the
> -Zpatchable-function-entry to rustc whenever it is passed to the C
> compiler.

As above, I suspect this isn't necessary to make CFI work, for any case
that works with C today, due to -fpatchable-funtion-entry being
overridden on a per-function basis. Are you seeing a problem in
practice, or was this found by inspection?

However IIUC this will allow rust to be traced via ftrace (assuming rust
records the instrumented locations as gcc and clang do); is that the
case? Assuming so, is there any ABI difference that might bite us? On
arm64 we require that anything marked instrumented with
patchable-function-entry strictly follows the AAPCS64 calling convention
and our ftrace trampolines save/restore the minimal set of necessary
registers, and I don't know how rust whether rust will behave the same
or e.g. use specialized calling conventions internally.

Mark.

> The required rustc version is bumped to 1.81.0 to ensure that the
> -Zpatchable-function-entry flag is available when CFI is used.
> 
> Fixes: ca627e636551 ("rust: cfi: add support for CFI_CLANG with Rust")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> Note that this fix uses rustc-option which has a pending fix:
> https://lore.kernel.org/all/20241008-rustc-option-bootstrap-v2-1-e6e155b8f9f3@google.com/
> ---
>  arch/arm64/Makefile     | 2 ++
>  arch/loongarch/Makefile | 1 +
>  arch/riscv/Makefile     | 2 ++
>  init/Kconfig            | 2 +-
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 9efd3f37c2fd..d7ec0bb09fc4 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -143,9 +143,11 @@ CHECKFLAGS	+= -D__aarch64__
>  ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS),y)
>    KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>    CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
> +  KBUILD_RUSTFLAGS += $(call rustc-option,-Zpatchable-function-entry=4$(comma)2)
>  else ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_ARGS),y)
>    KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>    CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> +  KBUILD_RUSTFLAGS += $(call rustc-option,-Zpatchable-function-entry=2)
>  endif
>  
>  ifeq ($(CONFIG_KASAN_SW_TAGS), y)
> diff --git a/arch/loongarch/Makefile b/arch/loongarch/Makefile
> index ae3f80622f4c..f9cef31d1f0e 100644
> --- a/arch/loongarch/Makefile
> +++ b/arch/loongarch/Makefile
> @@ -44,6 +44,7 @@ endif
>  ifdef CONFIG_DYNAMIC_FTRACE
>  KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>  CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> +KBUILD_RUSTFLAGS += $(call rustc-option,-Zpatchable-function-entry=2)
>  endif
>  
>  ifdef CONFIG_64BIT
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index d469db9f46f4..65d4dcba309a 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -16,8 +16,10 @@ ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
>  	KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>  ifeq ($(CONFIG_RISCV_ISA_C),y)
>  	CC_FLAGS_FTRACE := -fpatchable-function-entry=4
> +	KBUILD_RUSTFLAGS += $(call rustc-option,-Zpatchable-function-entry=4)
>  else
>  	CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> +	KBUILD_RUSTFLAGS += $(call rustc-option,-Zpatchable-function-entry=2)
>  endif
>  endif
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 530a382ee0fe..43434b681c3f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1946,7 +1946,7 @@ config RUST
>  	depends on !GCC_PLUGIN_RANDSTRUCT
>  	depends on !RANDSTRUCT
>  	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> -	depends on !CFI_CLANG || RUSTC_VERSION >= 107900 && HAVE_CFI_ICALL_NORMALIZE_INTEGERS
> +	depends on !CFI_CLANG || RUSTC_VERSION >= 108100 && HAVE_CFI_ICALL_NORMALIZE_INTEGERS
>  	select CFI_ICALL_NORMALIZE_INTEGERS if CFI_CLANG
>  	depends on !CALL_PADDING || RUSTC_VERSION >= 108100
>  	depends on !KASAN_SW_TAGS
> 
> ---
> base-commit: 4a335f920bc78e51b1d7d216d11f2ecbb6dd949f
> change-id: 20241008-cfi-patchable-all-ddd6275eaf4f
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
> 
>