On Thu, 14 Mar 2024 20:25:43 +0000,
Pierre-Clément Tosi <ptosi@xxxxxxxxxx> wrote:
>
> The compiler implements KCFI by adding type information (u32) above
> every function that might be indirectly called and, whenever a function
> pointer is called, injects a read-and-compare of that u32 against the
> value corresponding to the expected type. In case of a mismatch, a BRK
> instruction gets executed. When the hypervisor triggers such an
> exception, it panics.
Importantly, this triggers an exception return to EL1. If you don't
explain that, then nobody can really understand how you end-up in
nvhe_hyp_panic_handler() the first place.
>
> Therefore, teach hyp_panic() to detect KCFI errors from the ESR and
nvhe_hyp_panic_handler() instead hyp_panic()?
> report them. If necessary, remind the user that CONFIG_CFI_PERMISSIVE
> doesn't affect EL2 KCFI.
>
> Pass $(CC_FLAGS_CFI) to the compiler when building the nVHE hyp code.
>
> Use SYM_TYPED_FUNC_START() for __pkvm_init_switch_pgd, as nVHE can't
> call it directly and must use a PA function pointer from C (because it
> is part of the idmap page), which would trigger a KCFI failure if the
> type ID wasn't present.
>
> Signed-off-by: Pierre-Clément Tosi <ptosi@xxxxxxxxxx>
> ---
> arch/arm64/include/asm/esr.h | 6 ++++++
> arch/arm64/kvm/handle_exit.c | 11 +++++++++++
> arch/arm64/kvm/hyp/nvhe/Makefile | 6 +++---
> arch/arm64/kvm/hyp/nvhe/hyp-init.S | 3 ++-
> 4 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index b0c23e7d6595..281e352a4c94 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -397,6 +397,12 @@ static inline bool esr_is_data_abort(unsigned long esr)
> return ec == ESR_ELx_EC_DABT_LOW || ec == ESR_ELx_EC_DABT_CUR;
> }
>
> +static inline bool esr_is_cfi_brk(unsigned long esr)
> +{
> + return ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
> + (esr_comment(esr) & ~CFI_BRK_IMM_MASK) == CFI_BRK_IMM_BASE;
> +}
> +
nit: since there is a single user, please place this helper in handle_exit.c.
> static inline bool esr_fsc_is_translation_fault(unsigned long esr)
> {
> return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index ffa67ac6656c..9b6574e50b13 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -383,6 +383,15 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
> kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
> }
>
> +static void kvm_nvhe_report_cfi_failure(u64 panic_addr)
> +{
> + kvm_err("nVHE hyp CFI failure at: [<%016llx>] %pB!\n", panic_addr,
> + (void *)(panic_addr + kaslr_offset()));
> +
> + if (IS_ENABLED(CONFIG_CFI_PERMISSIVE))
> + kvm_err(" (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n");
> +}
> +
> void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> u64 elr_virt, u64 elr_phys,
> u64 par, uintptr_t vcpu,
> @@ -413,6 +422,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
> else
> kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
> (void *)(panic_addr + kaslr_offset()));
> + } else if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr)) {
It would seem logical to move the IS_ENABLED() into the ESR check helper.
> + kvm_nvhe_report_cfi_failure(panic_addr);
> } else {
> kvm_err("nVHE hyp panic at: [<%016llx>] %pB!\n", panic_addr,
> (void *)(panic_addr + kaslr_offset()));
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index 2250253a6429..2eb915d8943f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -89,9 +89,9 @@ quiet_cmd_hyprel = HYPREL $@
> quiet_cmd_hypcopy = HYPCOPY $@
> cmd_hypcopy = $(OBJCOPY) --prefix-symbols=__kvm_nvhe_ $< $@
>
> -# Remove ftrace, Shadow Call Stack, and CFI CFLAGS.
> -# This is equivalent to the 'notrace', '__noscs', and '__nocfi' annotations.
> -KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS) $(CC_FLAGS_CFI), $(KBUILD_CFLAGS))
> +# Remove ftrace and Shadow Call Stack CFLAGS.
> +# This is equivalent to the 'notrace' and '__noscs' annotations.
> +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS), $(KBUILD_CFLAGS))
> # Starting from 13.0.0 llvm emits SHT_REL section '.llvm.call-graph-profile'
> # when profile optimization is applied. gen-hyprel does not support SHT_REL and
> # causes a build failure. Remove profile optimization flags.
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> index 8958dd761837..ade73fdfaad9 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> @@ -5,6 +5,7 @@
> */
>
> #include <linux/arm-smccc.h>
> +#include <linux/cfi_types.h>
> #include <linux/linkage.h>
>
> #include <asm/alternative.h>
> @@ -265,7 +266,7 @@ alternative_else_nop_endif
>
> SYM_CODE_END(__kvm_handle_stub_hvc)
>
> -SYM_FUNC_START(__pkvm_init_switch_pgd)
> +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
Please put a comment indicating why SYM_TYPED_FUNC_START() is
necessary, because this will otherwise bitrot very quickly.
> /* Load the inputs from the VA pointer before turning the MMU off */
> ldr x5, [x0, #NVHE_INIT_PGD_PA]
> ldr x0, [x0, #NVHE_INIT_STACK_HYP_VA]
>
Another question is how do we test that this still works down the
line? In my experience, these features eventually bitrot because they
have very little functional impact (just like the panic handler broke
the ELR_EL2 handling). We really should have a way to exercise such
failure path.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
