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

From: Will Deacon <will@kernel.org>
To: Quentin Perret <qperret@google.com>
Cc: Marc Zyngier <maz@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/5] KVM: arm64: Propagate errors from __pkvm_prot_finalize hypercall
Date: Tue, 5 Oct 2021 12:30:41 +0100	[thread overview]
Message-ID: <20211005113040.GA29208@willie-the-truck> (raw)
In-Reply-To: <YVRr76aI+5zhq3nY@google.com>

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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm