| 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 |
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
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 --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)
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(-)