| Message ID | 20240510112645.3625702-10-ptosi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for hypervisor kCFI | expand |
On Fri, May 10, 2024 at 12:26:38PM +0100, Pierre-Clément Tosi wrote: > In order to easily periodically (and potentially automatically) validate > that the hypervisor kCFI feature doesn't bitrot, introduce a way to > trigger hypervisor kCFI faults from userspace on test builds of KVM. > > Add hooks in the hypervisor code to call registered callbacks (intended > to trigger kCFI faults either for the callback call itself of from > within the callback function) when running with guest or host VBAR_EL2. > As the calls are issued from the KVM_RUN ioctl handling path, userspace > gains control over when the actual triggering of the fault happens > without needing to modify the KVM uAPI. > > Export kernel functions to register these callbacks from modules and > introduce a kernel module intended to contain any testing logic. By > limiting the changes to the core kernel to a strict minimum, this > architectural split allows tests to be updated (within the module) > without the need to redeploy (or recompile) the kernel (hyp) under test. > > Use the module parameters as the uAPI for configuring the fault > condition being tested (i.e. either at insertion or post-insertion > using /sys/module/.../parameters), which naturally makes it impossible > for userspace to test kCFI without the module (and, inversely, makes > the module only - not KVM - responsible for exposing said uAPI). > > As kCFI is implemented with a caller-side check of a callee-side value, > make the module support 4 tests based on the location of the caller and > callee (built-in or in-module), for each of the 2 hypervisor contexts > (host & guest), selected by userspace using the 'guest' or 'host' module > parameter. For this purpose, export symbols which the module can use to > configure the callbacks for in-kernel and module-to-built-in kCFI > faulting calls. > > Define the module-to-kernel API to allow the module to detect that it > was loaded on a kernel built with support for it but which is running > without a hypervisor (-ENXIO) or with one that doesn't use the VHE CPU > feature (-EOPNOTSUPP), which is currently the only mode for which KVM > supports hypervisor kCFI. > > Allow kernel build configs to set CONFIG_HYP_CFI_TEST to only support > the in-kernel hooks (=y) or also build the test module (=m). Use > intermediate internal Kconfig flags (CONFIG_HYP_SUPPORTS_CFI_TEST and > CONFIG_HYP_CFI_TEST_MODULE) to simplify the Makefiles and #ifdefs. As > the symbols for callback registration are only exported to modules when > CONFIG_HYP_CFI_TEST != n, it is impossible for the test module to be > non-forcefully inserted on a kernel that doesn't support it. > > Note that this feature must NOT result in any noticeable change > (behavioral or binary size) when HYP_CFI_TEST_MODULE = n. > > CONFIG_HYP_CFI_TEST is intentionally independent of CONFIG_CFI_CLANG, to > avoid arbitrarily limiting the number of flag combinations that can be > tested with the module. > > Also note that, as VHE aliases VBAR_EL1 to VBAR_EL2 for the host, > testing hypervisor kCFI in VHE and in host context is equivalent to > testing kCFI support of the kernel itself i.e. EL1 in non-VHE and/or in > non-virtualized environments. For this reason, CONFIG_CFI_PERMISSIVE > **will** prevent the test module from triggering a hyp panic (although a > warning still gets printed) in that context. > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > --- > arch/arm64/include/asm/kvm_cfi.h | 36 ++++++++ > arch/arm64/kvm/Kconfig | 22 +++++ > arch/arm64/kvm/Makefile | 3 + > arch/arm64/kvm/hyp/include/hyp/cfi.h | 47 ++++++++++ > arch/arm64/kvm/hyp/vhe/Makefile | 1 + > arch/arm64/kvm/hyp/vhe/cfi.c | 37 ++++++++ > arch/arm64/kvm/hyp/vhe/switch.c | 7 ++ > arch/arm64/kvm/hyp_cfi_test.c | 43 +++++++++ > arch/arm64/kvm/hyp_cfi_test_module.c | 133 +++++++++++++++++++++++++++ > 9 files changed, 329 insertions(+) > create mode 100644 arch/arm64/include/asm/kvm_cfi.h > create mode 100644 arch/arm64/kvm/hyp/include/hyp/cfi.h > create mode 100644 arch/arm64/kvm/hyp/vhe/cfi.c > create mode 100644 arch/arm64/kvm/hyp_cfi_test.c > create mode 100644 arch/arm64/kvm/hyp_cfi_test_module.c > > diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h > new file mode 100644 > index 000000000000..13cc7b19d838 > --- /dev/null > +++ b/arch/arm64/include/asm/kvm_cfi.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2024 - Google Inc > + * Author: Pierre-Clément Tosi <ptosi@google.com> > + */ > + > +#ifndef __ARM64_KVM_CFI_H__ > +#define __ARM64_KVM_CFI_H__ > + > +#include <asm/kvm_asm.h> > +#include <linux/errno.h> > + > +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST > + > +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)); > +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)); Hmm, I tend to think this indirection is a little over the top for a test module that only registers a small handful of handlers: > +static int set_param_mode(const char *val, const struct kernel_param *kp, > + int (*register_cb)(void (*)(void))) > +{ > + unsigned int *mode = kp->arg; > + int err; > + > + err = param_set_uint(val, kp); > + if (err) > + return err; > + > + switch (*mode) { > + case 0: > + return register_cb(NULL); > + case 1: > + return register_cb(hyp_trigger_builtin_cfi_fault); > + case 2: > + return register_cb((void *)hyp_cfi_builtin2module_test_target); > + case 3: > + return register_cb(trigger_module2builtin_cfi_fault); > + case 4: > + return register_cb(trigger_module2module_cfi_fault); > + default: > + return -EINVAL; > + } > +} Why not just have a hyp selftest that runs through all of this behind a static key? I think it would simplify the code quite a bit, and you could move the registration and indirection logic. Will
Hi Will, Thanks for the review! I've addressed all your comments in v4, except for the one below. On Mon, May 13, 2024 at 06:21:21PM +0100, Will Deacon wrote: > On Fri, May 10, 2024 at 12:26:38PM +0100, Pierre-Clément Tosi wrote: > > In order to easily periodically (and potentially automatically) validate > > that the hypervisor kCFI feature doesn't bitrot, introduce a way to > > trigger hypervisor kCFI faults from userspace on test builds of KVM. > > > > Add hooks in the hypervisor code to call registered callbacks (intended > > to trigger kCFI faults either for the callback call itself of from > > within the callback function) when running with guest or host VBAR_EL2. > > As the calls are issued from the KVM_RUN ioctl handling path, userspace > > gains control over when the actual triggering of the fault happens > > without needing to modify the KVM uAPI. > > > > Export kernel functions to register these callbacks from modules and > > introduce a kernel module intended to contain any testing logic. By > > limiting the changes to the core kernel to a strict minimum, this > > architectural split allows tests to be updated (within the module) > > without the need to redeploy (or recompile) the kernel (hyp) under test. > > > > Use the module parameters as the uAPI for configuring the fault > > condition being tested (i.e. either at insertion or post-insertion > > using /sys/module/.../parameters), which naturally makes it impossible > > for userspace to test kCFI without the module (and, inversely, makes > > the module only - not KVM - responsible for exposing said uAPI). > > > > As kCFI is implemented with a caller-side check of a callee-side value, > > make the module support 4 tests based on the location of the caller and > > callee (built-in or in-module), for each of the 2 hypervisor contexts > > (host & guest), selected by userspace using the 'guest' or 'host' module > > parameter. For this purpose, export symbols which the module can use to > > configure the callbacks for in-kernel and module-to-built-in kCFI > > faulting calls. > > > > Define the module-to-kernel API to allow the module to detect that it > > was loaded on a kernel built with support for it but which is running > > without a hypervisor (-ENXIO) or with one that doesn't use the VHE CPU > > feature (-EOPNOTSUPP), which is currently the only mode for which KVM > > supports hypervisor kCFI. > > > > Allow kernel build configs to set CONFIG_HYP_CFI_TEST to only support > > the in-kernel hooks (=y) or also build the test module (=m). Use > > intermediate internal Kconfig flags (CONFIG_HYP_SUPPORTS_CFI_TEST and > > CONFIG_HYP_CFI_TEST_MODULE) to simplify the Makefiles and #ifdefs. As > > the symbols for callback registration are only exported to modules when > > CONFIG_HYP_CFI_TEST != n, it is impossible for the test module to be > > non-forcefully inserted on a kernel that doesn't support it. > > > > Note that this feature must NOT result in any noticeable change > > (behavioral or binary size) when HYP_CFI_TEST_MODULE = n. > > > > CONFIG_HYP_CFI_TEST is intentionally independent of CONFIG_CFI_CLANG, to > > avoid arbitrarily limiting the number of flag combinations that can be > > tested with the module. > > > > Also note that, as VHE aliases VBAR_EL1 to VBAR_EL2 for the host, > > testing hypervisor kCFI in VHE and in host context is equivalent to > > testing kCFI support of the kernel itself i.e. EL1 in non-VHE and/or in > > non-virtualized environments. For this reason, CONFIG_CFI_PERMISSIVE > > **will** prevent the test module from triggering a hyp panic (although a > > warning still gets printed) in that context. > > > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> > > --- > > arch/arm64/include/asm/kvm_cfi.h | 36 ++++++++ > > arch/arm64/kvm/Kconfig | 22 +++++ > > arch/arm64/kvm/Makefile | 3 + > > arch/arm64/kvm/hyp/include/hyp/cfi.h | 47 ++++++++++ > > arch/arm64/kvm/hyp/vhe/Makefile | 1 + > > arch/arm64/kvm/hyp/vhe/cfi.c | 37 ++++++++ > > arch/arm64/kvm/hyp/vhe/switch.c | 7 ++ > > arch/arm64/kvm/hyp_cfi_test.c | 43 +++++++++ > > arch/arm64/kvm/hyp_cfi_test_module.c | 133 +++++++++++++++++++++++++++ > > 9 files changed, 329 insertions(+) > > create mode 100644 arch/arm64/include/asm/kvm_cfi.h > > create mode 100644 arch/arm64/kvm/hyp/include/hyp/cfi.h > > create mode 100644 arch/arm64/kvm/hyp/vhe/cfi.c > > create mode 100644 arch/arm64/kvm/hyp_cfi_test.c > > create mode 100644 arch/arm64/kvm/hyp_cfi_test_module.c > > > > diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h > > new file mode 100644 > > index 000000000000..13cc7b19d838 > > --- /dev/null > > +++ b/arch/arm64/include/asm/kvm_cfi.h > > @@ -0,0 +1,36 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2024 - Google Inc > > + * Author: Pierre-Clément Tosi <ptosi@google.com> > > + */ > > + > > +#ifndef __ARM64_KVM_CFI_H__ > > +#define __ARM64_KVM_CFI_H__ > > + > > +#include <asm/kvm_asm.h> > > +#include <linux/errno.h> > > + > > +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST > > + > > +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)); > > +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)); > > Hmm, I tend to think this indirection is a little over the top for a test > module that only registers a small handful of handlers: > > > +static int set_param_mode(const char *val, const struct kernel_param *kp, > > + int (*register_cb)(void (*)(void))) > > +{ > > + unsigned int *mode = kp->arg; > > + int err; > > + > > + err = param_set_uint(val, kp); > > + if (err) > > + return err; > > + > > + switch (*mode) { > > + case 0: > > + return register_cb(NULL); > > + case 1: > > + return register_cb(hyp_trigger_builtin_cfi_fault); > > + case 2: > > + return register_cb((void *)hyp_cfi_builtin2module_test_target); > > + case 3: > > + return register_cb(trigger_module2builtin_cfi_fault); > > + case 4: > > + return register_cb(trigger_module2module_cfi_fault); > > + default: > > + return -EINVAL; > > + } > > +} > > Why not just have a hyp selftest that runs through all of this behind a > static key? I think it would simplify the code quite a bit, and you could > move the registration and indirection logic. I agree that the code would be simpler but note that the resulting tests would have a more limited coverage compared to what this currently implements. In particular, they would likely miss issues with the failure path itself (e.g. [1]) as the synchronous exception would need to be "handled" to let the selftest complete. OTOH, that would have the benefit of not triggering a kernel panic, making the test easier to integrate into existing CI suites. However, as the original request for those tests [2] was specifically about testing the failure path, I've held off from modifying the test module (in v4) until I get confirmation that Marc would be happy with your suggestion. [1]: https://lore.kernel.org/kvmarm/20240529121251.1993135-2-ptosi@google.com/ [2]: https://lore.kernel.org/kvmarm/867ci10zv6.wl-maz@kernel.org/ Thanks, Pierre > > Will
On Wed, May 29, 2024 at 01:26:31PM +0100, Pierre-Clément Tosi wrote: > On Mon, May 13, 2024 at 06:21:21PM +0100, Will Deacon wrote: > > On Fri, May 10, 2024 at 12:26:38PM +0100, Pierre-Clément Tosi wrote: > > > diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h > > > new file mode 100644 > > > index 000000000000..13cc7b19d838 > > > --- /dev/null > > > +++ b/arch/arm64/include/asm/kvm_cfi.h > > > @@ -0,0 +1,36 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > +/* > > > + * Copyright (C) 2024 - Google Inc > > > + * Author: Pierre-Clément Tosi <ptosi@google.com> > > > + */ > > > + > > > +#ifndef __ARM64_KVM_CFI_H__ > > > +#define __ARM64_KVM_CFI_H__ > > > + > > > +#include <asm/kvm_asm.h> > > > +#include <linux/errno.h> > > > + > > > +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST > > > + > > > +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)); > > > +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)); > > > > Hmm, I tend to think this indirection is a little over the top for a test > > module that only registers a small handful of handlers: > > > > > +static int set_param_mode(const char *val, const struct kernel_param *kp, > > > + int (*register_cb)(void (*)(void))) > > > +{ > > > + unsigned int *mode = kp->arg; > > > + int err; > > > + > > > + err = param_set_uint(val, kp); > > > + if (err) > > > + return err; > > > + > > > + switch (*mode) { > > > + case 0: > > > + return register_cb(NULL); > > > + case 1: > > > + return register_cb(hyp_trigger_builtin_cfi_fault); > > > + case 2: > > > + return register_cb((void *)hyp_cfi_builtin2module_test_target); > > > + case 3: > > > + return register_cb(trigger_module2builtin_cfi_fault); > > > + case 4: > > > + return register_cb(trigger_module2module_cfi_fault); > > > + default: > > > + return -EINVAL; > > > + } > > > +} > > > > Why not just have a hyp selftest that runs through all of this behind a > > static key? I think it would simplify the code quite a bit, and you could > > move the registration and indirection logic. > > I agree that the code would be simpler but note that the resulting tests would > have a more limited coverage compared to what this currently implements. In > particular, they would likely miss issues with the failure path itself (e.g. > [1]) as the synchronous exception would need to be "handled" to let the selftest > complete. OTOH, that would have the benefit of not triggering a kernel panic, > making the test easier to integrate into existing CI suites. > > However, as the original request for those tests [2] was specifically about > testing the failure path, I've held off from modifying the test module (in v4) > until I get confirmation that Marc would be happy with your suggestion. > > [1]: https://lore.kernel.org/kvmarm/20240529121251.1993135-2-ptosi@google.com/ > [2]: https://lore.kernel.org/kvmarm/867ci10zv6.wl-maz@kernel.org/ In which case, I think I'd drop the tests for now because I think the cure is worse than the disease with the current implementation. Will
diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h new file mode 100644 index 000000000000..13cc7b19d838 --- /dev/null +++ b/arch/arm64/include/asm/kvm_cfi.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 - Google Inc + * Author: Pierre-Clément Tosi <ptosi@google.com> + */ + +#ifndef __ARM64_KVM_CFI_H__ +#define __ARM64_KVM_CFI_H__ + +#include <asm/kvm_asm.h> +#include <linux/errno.h> + +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST + +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)); +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)); + +#else + +static inline int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)) +{ + return -EOPNOTSUPP; +} + +static inline int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)) +{ + return -EOPNOTSUPP; +} + +#endif /* CONFIG_HYP_SUPPORTS_CFI_TEST */ + +/* Symbols which the host can register as hyp callbacks; see <hyp/cfi.h>. */ +void hyp_trigger_builtin_cfi_fault(void); +void hyp_builtin_cfi_fault_target(int unused); + +#endif /* __ARM64_KVM_CFI_H__ */ diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index 58f09370d17e..5daa8079a120 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -65,4 +65,26 @@ config PROTECTED_NVHE_STACKTRACE If unsure, or not using protected nVHE (pKVM), say N. +config HYP_CFI_TEST + tristate "KVM hypervisor kCFI test support" + depends on KVM + help + Say Y or M here to build KVM with test hooks to support intentionally + triggering hypervisor kCFI faults in guest or host context. + + Say M here to also build a module which registers callbacks triggering + faults and selected by userspace through its parameters. + + Note that this feature is currently only supported in VHE mode. + + If unsure, say N. + +config HYP_SUPPORTS_CFI_TEST + def_bool y + depends on HYP_CFI_TEST + +config HYP_CFI_TEST_MODULE + def_tristate m if HYP_CFI_TEST = m + depends on HYP_CFI_TEST + endif # VIRTUALIZATION diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile index c0c050e53157..d42540ae3ea7 100644 --- a/arch/arm64/kvm/Makefile +++ b/arch/arm64/kvm/Makefile @@ -22,6 +22,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \ vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \ vgic/vgic-its.o vgic/vgic-debug.o +kvm-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += hyp_cfi_test.o kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o always-y := hyp_constants.h hyp-constants.s @@ -39,3 +40,5 @@ $(obj)/hyp_constants.h: $(obj)/hyp-constants.s FORCE obj-kvm := $(addprefix $(obj)/, $(kvm-y)) $(obj-kvm): $(obj)/hyp_constants.h + +obj-$(CONFIG_HYP_CFI_TEST_MODULE) += hyp_cfi_test_module.o diff --git a/arch/arm64/kvm/hyp/include/hyp/cfi.h b/arch/arm64/kvm/hyp/include/hyp/cfi.h new file mode 100644 index 000000000000..c6536040bc06 --- /dev/null +++ b/arch/arm64/kvm/hyp/include/hyp/cfi.h @@ -0,0 +1,47 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2024 - Google Inc + * Author: Pierre-Clément Tosi <ptosi@google.com> + */ + +#ifndef __ARM64_KVM_HYP_CFI_H__ +#define __ARM64_KVM_HYP_CFI_H__ + +#include <asm/bug.h> +#include <asm/errno.h> + +#include <linux/compiler.h> + +#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST + +int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt); + +extern void (*hyp_test_host_ctxt_cfi)(void); +extern void (*hyp_test_guest_ctxt_cfi)(void); + +/* Hypervisor callbacks for the host to register. */ +void hyp_trigger_builtin_cfi_fault(void); +void hyp_builtin_cfi_fault_target(int unused); + +#else + +static inline +int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt) +{ + return -EOPNOTSUPP; +} + +#define hyp_test_host_ctxt_cfi ((void(*)(void))(NULL)) +#define hyp_test_guest_ctxt_cfi ((void(*)(void))(NULL)) + +static inline void hyp_trigger_builtin_cfi_fault(void) +{ +} + +static inline void hyp_builtin_cfi_fault_target(int __always_unused unused) +{ +} + +#endif /* CONFIG_HYP_SUPPORTS_CFI_TEST */ + +#endif /* __ARM64_KVM_HYP_CFI_H__ */ diff --git a/arch/arm64/kvm/hyp/vhe/Makefile b/arch/arm64/kvm/hyp/vhe/Makefile index 3b9e5464b5b3..19ca584cc21e 100644 --- a/arch/arm64/kvm/hyp/vhe/Makefile +++ b/arch/arm64/kvm/hyp/vhe/Makefile @@ -9,3 +9,4 @@ ccflags-y := -D__KVM_VHE_HYPERVISOR__ obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \ ../fpsimd.o ../hyp-entry.o ../exception.o +obj-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += cfi.o diff --git a/arch/arm64/kvm/hyp/vhe/cfi.c b/arch/arm64/kvm/hyp/vhe/cfi.c new file mode 100644 index 000000000000..5849f239e27f --- /dev/null +++ b/arch/arm64/kvm/hyp/vhe/cfi.c @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 - Google Inc + * Author: Pierre-Clément Tosi <ptosi@google.com> + */ +#include <asm/rwonce.h> + +#include <hyp/cfi.h> + +void (*hyp_test_host_ctxt_cfi)(void); +void (*hyp_test_guest_ctxt_cfi)(void); + +int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt) +{ + if (in_host_ctxt) + hyp_test_host_ctxt_cfi = cb; + else + hyp_test_guest_ctxt_cfi = cb; + + return 0; +} + +void hyp_builtin_cfi_fault_target(int __always_unused unused) +{ +} + +void hyp_trigger_builtin_cfi_fault(void) +{ + /* Intentional UB cast & dereference, to trigger a kCFI fault. */ + void (*target)(void) = (void *)&hyp_builtin_cfi_fault_target; + + /* + * READ_ONCE() prevents this indirect call from being optimized out, + * forcing the compiler to generate the kCFI check before the branch. + */ + READ_ONCE(target)(); +} diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index 9db04a286398..b3268933b093 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c @@ -4,6 +4,7 @@ * Author: Marc Zyngier <marc.zyngier@arm.com> */ +#include <hyp/cfi.h> #include <hyp/switch.h> #include <linux/arm-smccc.h> @@ -221,6 +222,9 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) struct kvm_cpu_context *guest_ctxt; u64 exit_code; + if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_host_ctxt_cfi)) + hyp_test_host_ctxt_cfi(); + host_ctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt; host_ctxt->__hyp_running_vcpu = vcpu; guest_ctxt = &vcpu->arch.ctxt; @@ -245,6 +249,9 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) else vcpu_clear_flag(vcpu, VCPU_HYP_CONTEXT); + if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_guest_ctxt_cfi)) + hyp_test_guest_ctxt_cfi(); + do { /* Jump in the fire! */ exit_code = __guest_enter(vcpu); diff --git a/arch/arm64/kvm/hyp_cfi_test.c b/arch/arm64/kvm/hyp_cfi_test.c new file mode 100644 index 000000000000..da7b25ca1b1f --- /dev/null +++ b/arch/arm64/kvm/hyp_cfi_test.c @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 - Google Inc + * Author: Pierre-Clément Tosi <ptosi@google.com> + */ +#include <asm/kvm_asm.h> +#include <asm/kvm_cfi.h> +#include <asm/kvm_host.h> +#include <asm/virt.h> + +#include <linux/export.h> +#include <linux/stddef.h> +#include <linux/types.h> + +/* For calling directly into the VHE hypervisor; see <hyp/cfi.h>. */ +int __kvm_register_cfi_test_cb(void (*)(void), bool); + +static int kvm_register_cfi_test_cb(void (*vhe_cb)(void), bool in_host_ctxt) +{ + if (!is_hyp_mode_available()) + return -ENXIO; + + if (is_hyp_nvhe()) + return -EOPNOTSUPP; + + return __kvm_register_cfi_test_cb(vhe_cb, in_host_ctxt); +} + +int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void)) +{ + return kvm_register_cfi_test_cb(cb, true); +} +EXPORT_SYMBOL(kvm_cfi_test_register_host_ctxt_cb); + +int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void)) +{ + return kvm_register_cfi_test_cb(cb, false); +} +EXPORT_SYMBOL(kvm_cfi_test_register_guest_ctxt_cb); + +/* Hypervisor callbacks for the test module to register. */ +EXPORT_SYMBOL(hyp_trigger_builtin_cfi_fault); +EXPORT_SYMBOL(hyp_builtin_cfi_fault_target); diff --git a/arch/arm64/kvm/hyp_cfi_test_module.c b/arch/arm64/kvm/hyp_cfi_test_module.c new file mode 100644 index 000000000000..eeda4be4d3ef --- /dev/null +++ b/arch/arm64/kvm/hyp_cfi_test_module.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2024 - Google Inc + * Author: Pierre-Clément Tosi <ptosi@google.com> + */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <asm/kvm_asm.h> +#include <asm/kvm_cfi.h> +#include <asm/rwonce.h> + +#include <linux/init.h> +#include <linux/kstrtox.h> +#include <linux/module.h> +#include <linux/printk.h> + +static int set_host_mode(const char *val, const struct kernel_param *kp); +static int set_guest_mode(const char *val, const struct kernel_param *kp); + +#define M_DESC \ + "\n\t0: none" \ + "\n\t1: built-in caller & built-in callee" \ + "\n\t2: built-in caller & module callee" \ + "\n\t3: module caller & built-in callee" \ + "\n\t4: module caller & module callee" + +static unsigned int host_mode; +module_param_call(host, set_host_mode, param_get_uint, &host_mode, 0644); +MODULE_PARM_DESC(host, + "Hypervisor kCFI fault test case in host context:" M_DESC); + +static unsigned int guest_mode; +module_param_call(guest, set_guest_mode, param_get_uint, &guest_mode, 0644); +MODULE_PARM_DESC(guest, + "Hypervisor kCFI fault test case in guest context:" M_DESC); + +static void trigger_module2module_cfi_fault(void); +static void trigger_module2builtin_cfi_fault(void); +static void hyp_cfi_module2module_test_target(int); +static void hyp_cfi_builtin2module_test_target(int); + +static int set_param_mode(const char *val, const struct kernel_param *kp, + int (*register_cb)(void (*)(void))) +{ + unsigned int *mode = kp->arg; + int err; + + err = param_set_uint(val, kp); + if (err) + return err; + + switch (*mode) { + case 0: + return register_cb(NULL); + case 1: + return register_cb(hyp_trigger_builtin_cfi_fault); + case 2: + return register_cb((void *)hyp_cfi_builtin2module_test_target); + case 3: + return register_cb(trigger_module2builtin_cfi_fault); + case 4: + return register_cb(trigger_module2module_cfi_fault); + default: + return -EINVAL; + } +} + +static int set_host_mode(const char *val, const struct kernel_param *kp) +{ + return set_param_mode(val, kp, kvm_cfi_test_register_host_ctxt_cb); +} + +static int set_guest_mode(const char *val, const struct kernel_param *kp) +{ + return set_param_mode(val, kp, kvm_cfi_test_register_guest_ctxt_cb); +} + +static void __exit exit_hyp_cfi_test(void) +{ + int err; + + err = kvm_cfi_test_register_host_ctxt_cb(NULL); + if (err) + pr_err("Failed to unregister host context trigger: %d\n", err); + + err = kvm_cfi_test_register_guest_ctxt_cb(NULL); + if (err) + pr_err("Failed to unregister guest context trigger: %d\n", err); +} +module_exit(exit_hyp_cfi_test); + +static void trigger_module2builtin_cfi_fault(void) +{ + /* Intentional UB cast & dereference, to trigger a kCFI fault. */ + void (*target)(void) = (void *)&hyp_builtin_cfi_fault_target; + + /* + * READ_ONCE() prevents this indirect call from being optimized out, + * forcing the compiler to generate the kCFI check before the branch. + */ + READ_ONCE(target)(); + + pr_err_ratelimited("%s: Survived a kCFI violation\n", __func__); +} + +static void trigger_module2module_cfi_fault(void) +{ + /* Intentional UB cast & dereference, to trigger a kCFI fault. */ + void (*target)(void) = (void *)&hyp_cfi_module2module_test_target; + + /* + * READ_ONCE() prevents this indirect call from being optimized out, + * forcing the compiler to generate the kCFI check before the branch. + */ + READ_ONCE(target)(); + + pr_err_ratelimited("%s: Survived a kCFI violation\n", __func__); +} + +/* Use different functions, for clearer symbols in kCFI panic reports. */ +static noinline +void hyp_cfi_module2module_test_target(int __always_unused unused) +{ +} + +static noinline +void hyp_cfi_builtin2module_test_target(int __always_unused unused) +{ +} + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Pierre-Clément Tosi <ptosi@google.com>"); +MODULE_DESCRIPTION("KVM hypervisor kCFI test module");
In order to easily periodically (and potentially automatically) validate that the hypervisor kCFI feature doesn't bitrot, introduce a way to trigger hypervisor kCFI faults from userspace on test builds of KVM. Add hooks in the hypervisor code to call registered callbacks (intended to trigger kCFI faults either for the callback call itself of from within the callback function) when running with guest or host VBAR_EL2. As the calls are issued from the KVM_RUN ioctl handling path, userspace gains control over when the actual triggering of the fault happens without needing to modify the KVM uAPI. Export kernel functions to register these callbacks from modules and introduce a kernel module intended to contain any testing logic. By limiting the changes to the core kernel to a strict minimum, this architectural split allows tests to be updated (within the module) without the need to redeploy (or recompile) the kernel (hyp) under test. Use the module parameters as the uAPI for configuring the fault condition being tested (i.e. either at insertion or post-insertion using /sys/module/.../parameters), which naturally makes it impossible for userspace to test kCFI without the module (and, inversely, makes the module only - not KVM - responsible for exposing said uAPI). As kCFI is implemented with a caller-side check of a callee-side value, make the module support 4 tests based on the location of the caller and callee (built-in or in-module), for each of the 2 hypervisor contexts (host & guest), selected by userspace using the 'guest' or 'host' module parameter. For this purpose, export symbols which the module can use to configure the callbacks for in-kernel and module-to-built-in kCFI faulting calls. Define the module-to-kernel API to allow the module to detect that it was loaded on a kernel built with support for it but which is running without a hypervisor (-ENXIO) or with one that doesn't use the VHE CPU feature (-EOPNOTSUPP), which is currently the only mode for which KVM supports hypervisor kCFI. Allow kernel build configs to set CONFIG_HYP_CFI_TEST to only support the in-kernel hooks (=y) or also build the test module (=m). Use intermediate internal Kconfig flags (CONFIG_HYP_SUPPORTS_CFI_TEST and CONFIG_HYP_CFI_TEST_MODULE) to simplify the Makefiles and #ifdefs. As the symbols for callback registration are only exported to modules when CONFIG_HYP_CFI_TEST != n, it is impossible for the test module to be non-forcefully inserted on a kernel that doesn't support it. Note that this feature must NOT result in any noticeable change (behavioral or binary size) when HYP_CFI_TEST_MODULE = n. CONFIG_HYP_CFI_TEST is intentionally independent of CONFIG_CFI_CLANG, to avoid arbitrarily limiting the number of flag combinations that can be tested with the module. Also note that, as VHE aliases VBAR_EL1 to VBAR_EL2 for the host, testing hypervisor kCFI in VHE and in host context is equivalent to testing kCFI support of the kernel itself i.e. EL1 in non-VHE and/or in non-virtualized environments. For this reason, CONFIG_CFI_PERMISSIVE **will** prevent the test module from triggering a hyp panic (although a warning still gets printed) in that context. Signed-off-by: Pierre-Clément Tosi <ptosi@google.com> --- arch/arm64/include/asm/kvm_cfi.h | 36 ++++++++ arch/arm64/kvm/Kconfig | 22 +++++ arch/arm64/kvm/Makefile | 3 + arch/arm64/kvm/hyp/include/hyp/cfi.h | 47 ++++++++++ arch/arm64/kvm/hyp/vhe/Makefile | 1 + arch/arm64/kvm/hyp/vhe/cfi.c | 37 ++++++++ arch/arm64/kvm/hyp/vhe/switch.c | 7 ++ arch/arm64/kvm/hyp_cfi_test.c | 43 +++++++++ arch/arm64/kvm/hyp_cfi_test_module.c | 133 +++++++++++++++++++++++++++ 9 files changed, 329 insertions(+) create mode 100644 arch/arm64/include/asm/kvm_cfi.h create mode 100644 arch/arm64/kvm/hyp/include/hyp/cfi.h create mode 100644 arch/arm64/kvm/hyp/vhe/cfi.c create mode 100644 arch/arm64/kvm/hyp_cfi_test.c create mode 100644 arch/arm64/kvm/hyp_cfi_test_module.c