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

From: Will Deacon <will@kernel.org>
To: "Pierre-Clément Tosi" <ptosi@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Vincent Donnefort <vdonnefort@google.com>
Subject: Re: [PATCH v3 09/12] KVM: arm64: VHE: Add test module for hyp kCFI
Date: Mon, 3 Jun 2024 14:10:32 +0100	[thread overview]
Message-ID: <20240603131031.GB18991@willie-the-truck> (raw)
In-Reply-To: <lsb5l4ee4unh76r25x6lx5lgncoxsjfy3q6xeuncv3efhs7jzm@bzdwf6j274et>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel