On Thu, 24 Feb 2022 19:21:50 +0000,
Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> 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.
Until you look at Thumb-2 ;-)
>
> > 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.
Fair enough. I'm not fundamentally against this patch. It is just a
bit out of context in this series.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
