Re: [PATCH 3/5] KVM: arm64: Propagate errors from __pkvm_prot_finalize hypercall

From: Quentin Perret <qperret@google.com>
To: Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	Marc Zyngier <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH 3/5] KVM: arm64: Propagate errors from __pkvm_prot_finalize hypercall
Date: Wed, 29 Sep 2021 14:36:47 +0100	[thread overview]
Message-ID: <YVRr76aI+5zhq3nY@google.com> (raw)
In-Reply-To: <20210923112256.15767-4-will@kernel.org>

On Thursday 23 Sep 2021 at 12:22:54 (+0100), Will Deacon wrote:
> If the __pkvm_prot_finalize hypercall returns an error, we WARN but fail
> to propagate the failure code back to kvm_arch_init().
> 
> Pass a pointer to a zero-initialised return variable so that failure
> to finalise the pKVM protections on a host CPU can be reported back to
> KVM.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kvm/arm.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 9506cf88fa0e..13bbf35896cd 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1986,9 +1986,25 @@ static int init_hyp_mode(void)
>  	return err;
>  }
>  
> -static void _kvm_host_prot_finalize(void *discard)
> +static void _kvm_host_prot_finalize(void *arg)
>  {
> -	WARN_ON(kvm_call_hyp_nvhe(__pkvm_prot_finalize));
> +	int *err = arg;
> +
> +	if (WARN_ON(kvm_call_hyp_nvhe(__pkvm_prot_finalize)))
> +		WRITE_ONCE(*err, -EINVAL);
> +}

I was going to suggest to propagate the hypercall's error code directly,
but this becomes very racy so n/m...

But this got me thinking about what we should do when the hyp init fails
while the protected mode has been explicitly enabled on the kernel
cmdline. That is, if we continue and boot the kernel w/o KVM support,
then I don't know how e.g. EL3 can know that it shouldn't give keys to
VMs because the kernel (and EL2) can't be trusted. It feels like it is
the kernel's responsibility to do something while it _is_ still
trustworthy.

I guess we could make any error code fatal in kvm_arch_init() when
is_protected_kvm_enabled() is on, or something along those lines? Maybe
dependent on CONFIG_NVHE_EL2_DEBUG=n?

It's probably a bit theoretical because there really shouldn't be any
reason to fail hyp init in production when using a signed kernel image
etc etc, but then if that is the case the additional check I'm
suggesting shouldn't hurt and will give us some peace of mind. Thoughts?

Thanks,
Quentin

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