[PATCH v3 09/12] KVM: arm64: VHE: Add test module for hyp kCFI

[Will Deacon]

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 at 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 at 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