diff mbox series

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

Message ID 20210923112256.15767-4-will@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Restrict host hypercalls when pKVM is enabled | expand

Commit Message

Will Deacon Sept. 23, 2021, 11:22 a.m. UTC
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(-)

Comments

Quentin Perret Sept. 29, 2021, 1:36 p.m. UTC | #1
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
Will Deacon Oct. 5, 2021, 11:30 a.m. UTC | #2
Hey Quentin,

On Wed, Sep 29, 2021 at 02:36:47PM +0100, Quentin Perret wrote:
> 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?

It's an interesting one.

I'm not hugely keen on crashing the system if we fail to deprivilege the
host (which I think is effectively what is happening in the case you
describe), but you're right that we need to disable pKVM somehow in this
case. I think the best thing would be to wipe the pvmfw memory; that would
mean that the host can do whatever it likes at EL2, as the keys will no
longer be available.

I'll make a note about this, since I've parked the pvmfw patches until
we've got more of the pKVM infrastructure up and running.

Will
diff mbox series

Patch

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);
+}
+
+static int pkvm_drop_host_privileges(void)
+{
+	int ret = 0;
+
+	/*
+	 * Flip the static key upfront as that may no longer be possible
+	 * once the host stage 2 is installed.
+	 */
+	static_branch_enable(&kvm_protected_mode_initialized);
+	on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
+	return ret;
 }
 
 static int finalize_hyp_mode(void)
@@ -2002,15 +2018,7 @@  static int finalize_hyp_mode(void)
 	 * None of other sections should ever be introspected.
 	 */
 	kmemleak_free_part(__hyp_bss_start, __hyp_bss_end - __hyp_bss_start);
-
-	/*
-	 * Flip the static key upfront as that may no longer be possible
-	 * once the host stage 2 is installed.
-	 */
-	static_branch_enable(&kvm_protected_mode_initialized);
-	on_each_cpu(_kvm_host_prot_finalize, NULL, 1);
-
-	return 0;
+	return pkvm_drop_host_privileges();
 }
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)