Hi Marc,
On Thu, Feb 24, 2022 at 12:30:49PM +0000, Marc Zyngier wrote:
> On Wed, 23 Feb 2022 04:18:28 +0000,
> Oliver Upton <oupton@xxxxxxxxxx> wrote:
> >
> > DEN0022D.b 5.6.2 "Caller responsibilities" states that a PSCI
> > implementation may return INVALID_ADDRESS for the CPU_ON call if the
> > provided entry address is known to be invalid. There is an additional
> > caveat to this rule. Prior to PSCI v1.0, the INVALID_PARAMETERS error
> > is returned instead. Check the guest's PSCI version and return the
> > appropriate error if the IPA is invalid.
> >
> > Reported-by: Reiji Watanabe <reijiw@xxxxxxxxxx>
> > Signed-off-by: Oliver Upton <oupton@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/psci.c | 24 ++++++++++++++++++++++--
> > 1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> > index a0c10c11f40e..de1cf554929d 100644
> > --- a/arch/arm64/kvm/psci.c
> > +++ b/arch/arm64/kvm/psci.c
> > @@ -12,6 +12,7 @@
> >
> > #include <asm/cputype.h>
> > #include <asm/kvm_emulate.h>
> > +#include <asm/kvm_mmu.h>
> >
> > #include <kvm/arm_psci.h>
> > #include <kvm/arm_hypercalls.h>
> > @@ -70,12 +71,31 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
> > struct vcpu_reset_state *reset_state;
> > struct kvm *kvm = source_vcpu->kvm;
> > struct kvm_vcpu *vcpu = NULL;
> > - unsigned long cpu_id;
> > + unsigned long cpu_id, entry_addr;
> >
> > cpu_id = smccc_get_arg1(source_vcpu);
> > if (!kvm_psci_valid_affinity(source_vcpu, cpu_id))
> > return PSCI_RET_INVALID_PARAMS;
> >
> > + /*
> > + * Basic sanity check: ensure the requested entry address actually
> > + * exists within the guest's address space.
> > + */
> > + entry_addr = smccc_get_arg2(source_vcpu);
> > + if (!kvm_ipa_valid(kvm, entry_addr)) {
> > +
> > + /*
> > + * Before PSCI v1.0, the INVALID_PARAMETERS error is returned
> > + * instead of INVALID_ADDRESS.
> > + *
> > + * For more details, see ARM DEN0022D.b 5.6 "CPU_ON".
> > + */
> > + if (kvm_psci_version(source_vcpu) < KVM_ARM_PSCI_1_0)
> > + return PSCI_RET_INVALID_PARAMS;
> > + else
> > + return PSCI_RET_INVALID_ADDRESS;
> > + }
> > +
>
> If you're concerned with this, should you also check for the PC
> alignment, or the presence of a memslot covering the address you are
> branching to? Le latter is particularly hard to implement reliably.
Andrew, Reiji and I had a conversation regarding exactly this on the
last run of this series, and concluded that checking against the IPA is
probably the best KVM can do [1]. That said, alignment is also an easy
thing to check.
> So far, my position has been that the guest is free to shoot itself in
> the foot if that's what it wants to do, and that babysitting it was a
> waste of useful bits! ;-)
>
Agreed -- there are plenty of spectacular/hilarious ways in which the
guest can mess up :-)
> Or have you identified something that makes it a requirement to handle
> this case (and possibly others) in the hypervisor?
It is a lot easier to tell a guest that their software is broken if they
get an error back from the hypercall, whereas a vCPU off in the weeds
might need to be looked at before concluding there's a guest issue.
[1]: http://lore.kernel.org/r/20211005190153.dc2befzcisvznxq5@xxxxxxxxxx
--
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
