| Message ID | 20240529121251.1993135-11-ptosi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for hypervisor kCFI | expand |
On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi 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 in nVHE, it panics and triggers and exception return to EL1. > > Therefore, teach nvhe_hyp_panic_handler() to detect kCFI errors from the > ESR and report them. If necessary, remind the user that EL2 kCFI is not > affected by CONFIG_CFI_PERMISSIVE. > > 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/kvm/handle_exit.c | 10 ++++++++++ > arch/arm64/kvm/hyp/nvhe/Makefile | 6 +++--- > arch/arm64/kvm/hyp/nvhe/hyp-init.S | 6 +++++- > 3 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index b3d6657a259d..69b08ac7322d 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -417,6 +417,14 @@ static void print_nvhe_hyp_panic(const char *name, u64 panic_addr) > (void *)(panic_addr + kaslr_offset())); > } > > +static void kvm_nvhe_report_cfi_failure(u64 panic_addr) > +{ > + print_nvhe_hyp_panic("CFI failure", panic_addr); > + > + 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, > @@ -446,6 +454,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr, > kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line); > else > print_nvhe_hyp_panic("BUG", panic_addr); > + } else if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr)) { > + kvm_nvhe_report_cfi_failure(panic_addr); > } else { > print_nvhe_hyp_panic("panic", panic_addr); > } > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile > index 50fa0ffb6b7e..782b34b004be 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 d859c4de06b6..b1c8977e2812 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> > @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc) > > /* > * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void)); > + * > + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly > + * using a physical pointer without triggering a kCFI failure. > */ > -SYM_FUNC_START(__pkvm_init_switch_pgd) > +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) > /* Turn the MMU off */ > pre_disable_mmu_workaround > mrs x9, sctlr_el2 I still think this last hunk should be merged with the earlier patch fixing up the prototype of __pkvm_init_switch_pgd(). With that: Acked-by: Will Deacon <will@kernel.org> Will
On Mon, Jun 03, 2024 at 03:45:30PM +0100, Will Deacon wrote: > On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi wrote: > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S > > index d859c4de06b6..b1c8977e2812 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> > > @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc) > > > > /* > > * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void)); > > + * > > + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly > > + * using a physical pointer without triggering a kCFI failure. > > */ > > -SYM_FUNC_START(__pkvm_init_switch_pgd) > > +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) > > /* Turn the MMU off */ > > pre_disable_mmu_workaround > > mrs x9, sctlr_el2 > > I still think this last hunk should be merged with the earlier patch > fixing up the prototype of __pkvm_init_switch_pgd(). Unfortunately, this is not possible because SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) makes the assembler generate an unresolved symbol for the function type, which the compiler only generates (from the C declaration) if it compiles with kCFI so that moving this hunk to an earlier patch results in a linker error: ld.lld: error: undefined symbol: __kvm_nvhe___kcfi_typeid___pkvm_init_switch_pgd OTOH, moving it to a later patch triggers a kCFI (runtime) panic. As a result, this hunk *must* be part of this patch. > > With that: > > Acked-by: Will Deacon <will@kernel.org> As I haven't followed your suggestion, I'll ignore this.
On Tue, Jun 04, 2024 at 05:04:40PM +0100, Pierre-Clément Tosi wrote: > On Mon, Jun 03, 2024 at 03:45:30PM +0100, Will Deacon wrote: > > On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi wrote: > > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S > > > index d859c4de06b6..b1c8977e2812 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> > > > @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc) > > > > > > /* > > > * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void)); > > > + * > > > + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly > > > + * using a physical pointer without triggering a kCFI failure. > > > */ > > > -SYM_FUNC_START(__pkvm_init_switch_pgd) > > > +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) > > > /* Turn the MMU off */ > > > pre_disable_mmu_workaround > > > mrs x9, sctlr_el2 > > > > I still think this last hunk should be merged with the earlier patch > > fixing up the prototype of __pkvm_init_switch_pgd(). > > Unfortunately, this is not possible because > > SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) > > makes the assembler generate an unresolved symbol for the function type, which > the compiler only generates (from the C declaration) if it compiles with kCFI so > that moving this hunk to an earlier patch results in a linker error: > > ld.lld: error: undefined symbol: __kvm_nvhe___kcfi_typeid___pkvm_init_switch_pgd > > OTOH, moving it to a later patch triggers a kCFI (runtime) panic. > > As a result, this hunk *must* be part of this patch. Argh, thanks for the explanation. I thought CONFIG_CFI_CLANG would save us here, but that's already enabled for the rest of the kernel so now I understand what you mean. > > With that: > > > > Acked-by: Will Deacon <will@kernel.org> > > As I haven't followed your suggestion, I'll ignore this. You can keep the Ack. Cheers, Will
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index b3d6657a259d..69b08ac7322d 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -417,6 +417,14 @@ static void print_nvhe_hyp_panic(const char *name, u64 panic_addr) (void *)(panic_addr + kaslr_offset())); } +static void kvm_nvhe_report_cfi_failure(u64 panic_addr) +{ + print_nvhe_hyp_panic("CFI failure", panic_addr); + + 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, @@ -446,6 +454,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr, kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line); else print_nvhe_hyp_panic("BUG", panic_addr); + } else if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr)) { + kvm_nvhe_report_cfi_failure(panic_addr); } else { print_nvhe_hyp_panic("panic", panic_addr); } diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile index 50fa0ffb6b7e..782b34b004be 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 d859c4de06b6..b1c8977e2812 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> @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc) /* * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void)); + * + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly + * using a physical pointer without triggering a kCFI failure. */ -SYM_FUNC_START(__pkvm_init_switch_pgd) +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd) /* Turn the MMU off */ pre_disable_mmu_workaround mrs x9, sctlr_el2
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 in nVHE, it panics and triggers and exception return to EL1. Therefore, teach nvhe_hyp_panic_handler() to detect kCFI errors from the ESR and report them. If necessary, remind the user that EL2 kCFI is not affected by CONFIG_CFI_PERMISSIVE. 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/kvm/handle_exit.c | 10 ++++++++++ arch/arm64/kvm/hyp/nvhe/Makefile | 6 +++--- arch/arm64/kvm/hyp/nvhe/hyp-init.S | 6 +++++- 3 files changed, 18 insertions(+), 4 deletions(-)