| Message ID | 20220223041844.3984439-4-oupton@google.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | KVM: arm64: Implement PSCI SYSTEM_SUSPEND | expand |
On Tue, Feb 22, 2022 at 8:19 PM Oliver Upton <oupton@google.com> 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@google.com> > Signed-off-by: Oliver Upton <oupton@google.com> Reviewed-by: Reiji Watanabe <reijiw@google.com>
On Wed, 23 Feb 2022 04:18:28 +0000, Oliver Upton <oupton@google.com> 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@google.com> > Signed-off-by: Oliver Upton <oupton@google.com> > --- > 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. 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! ;-) Or have you identified something that makes it a requirement to handle this case (and possibly others) in the hypervisor? Thanks, M.
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@google.com> 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@google.com> > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > 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@gator.home -- Oliver
On Thu, 24 Feb 2022 19:21:50 +0000, Oliver Upton <oupton@google.com> 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@google.com> 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@google.com> > > > Signed-off-by: Oliver Upton <oupton@google.com> > > > --- > > > 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.
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; + } + vcpu = kvm_mpidr_to_vcpu(kvm, cpu_id); /* @@ -93,7 +113,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) reset_state = &vcpu->arch.reset_state; - reset_state->pc = smccc_get_arg2(source_vcpu); + reset_state->pc = entry_addr; /* Propagate caller endianness */ reset_state->be = kvm_vcpu_is_be(source_vcpu);
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@google.com> Signed-off-by: Oliver Upton <oupton@google.com> --- arch/arm64/kvm/psci.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)