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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> > + */
> > +
> > +#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@xxxxxxxxxx/
[2]: https://lore.kernel.org/kvmarm/867ci10zv6.wl-maz@xxxxxxxxxx/
Thanks,
Pierre
>
> Will
