| Message ID | 87885c41627a033d9772dd368049e7f8f5fd4ef7.1710446682.git.ptosi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for hypervisor kCFI | expand |
On Thu, 14 Mar 2024 20:25:43 +0000, Pierre-Clément Tosi <ptosi@google.com> 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@google.com> > --- > 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.
Hi Marc, On Sun, Mar 17, 2024 at 01:09:01PM +0000, Marc Zyngier wrote: > On Thu, 14 Mar 2024 20:25:43 +0000, > Pierre-Clément Tosi <ptosi@google.com> wrote: > > > > 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. I've placed this here as I'm introducing a second user in a following patch of this series (in the VHE code) and wanted to avoid adding code then immediately moving it around. I've therefore kept this part unchanged in v2 but let me know if you prefer the commits to add-then-move and I'll update that for v3. > > 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. I suppose it makes sense for a static function but, given that I've kept the helper in a shared header and as it essentially is a straightforward shift-mask-compare (like the existing helpers in <asm/esr.h>), wouldn't it be confusing for its result to depend on a Kconfig flag? Anyway, same as above; left unchanged in v2 but happy to update this in v3.
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; +} + 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)) { + 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) /* 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]
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. Therefore, teach hyp_panic() to detect KCFI errors from the ESR and 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@google.com> --- 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(-)