| Message ID | 20200401165816.530281-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PSCI fixes | expand |
On Wed, Apr 01, 2020 at 05:58:15PM +0100, Marc Zyngier wrote: > When a guest delibarately uses an SSMC32 function number (which is allowed), > we should make sure we drop the top 32bits from the input arguments, as they > could legitimately be junk. > > Reported-by: Christoffer Dall <christoffer.dall@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 17e2bdd4b76f..69ff4a51ceb5 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -187,6 +187,18 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu) > kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET); > } > > +static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > +{ > + int i; > + > + /* > + * Zero the input registers' upper 32 bits. They will be fully > + * zeroed on exit, so we're fine changing them in place. > + */ > + for (i = 1; i < 4; i++) > + vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -211,12 +223,16 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > val = PSCI_RET_SUCCESS; > break; > case PSCI_0_2_FN_CPU_ON: > + kvm_psci_narrow_to_32bit(vcpu); > + fallthrough; > case PSCI_0_2_FN64_CPU_ON: > mutex_lock(&kvm->lock); > val = kvm_psci_vcpu_on(vcpu); > mutex_unlock(&kvm->lock); > break; > case PSCI_0_2_FN_AFFINITY_INFO: > + kvm_psci_narrow_to_32bit(vcpu); > + fallthrough; > case PSCI_0_2_FN64_AFFINITY_INFO: > val = kvm_psci_vcpu_affinity_info(vcpu); > break; > -- > 2.25.0 > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
Hi, On 4/1/20 5:58 PM, Marc Zyngier wrote: > When a guest delibarately uses an SSMC32 function number (which is allowed), s/SSMC32/SMC32 > we should make sure we drop the top 32bits from the input arguments, as they > could legitimately be junk. > > Reported-by: Christoffer Dall <christoffer.dall@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 17e2bdd4b76f..69ff4a51ceb5 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -187,6 +187,18 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu) > kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET); > } > > +static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > +{ > + int i; > + > + /* > + * Zero the input registers' upper 32 bits. They will be fully > + * zeroed on exit, so we're fine changing them in place. > + */ > + for (i = 1; i < 4; i++) > + vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); One minor suggestion, it could be lower_32_bits instead, but that's down to personal preference and entirely up to you. > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -211,12 +223,16 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > val = PSCI_RET_SUCCESS; > break; > case PSCI_0_2_FN_CPU_ON: > + kvm_psci_narrow_to_32bit(vcpu); > + fallthrough; > case PSCI_0_2_FN64_CPU_ON: > mutex_lock(&kvm->lock); > val = kvm_psci_vcpu_on(vcpu); > mutex_unlock(&kvm->lock); > break; > case PSCI_0_2_FN_AFFINITY_INFO: > + kvm_psci_narrow_to_32bit(vcpu); > + fallthrough; > case PSCI_0_2_FN64_AFFINITY_INFO: > val = kvm_psci_vcpu_affinity_info(vcpu); > break; From ARM DEN 0022D, those are indeed the only functions with ids that differ from SMC32 to SMC64, and have arguments that KVM doesn't ignore (like it does with CPU_SUSPEND). I also had a look at smccc_get_arg{1,2,3}, because they read the register values and return an unsigned long. smccc_get_arg1 is called after the registers have been narrowed, or the result is cast into an u32 when called before that. smccc_get_arg{2,3} are always called as part of the individual PSCI function implementations, which come after the arguments have been narrowed. With that: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 17e2bdd4b76f..69ff4a51ceb5 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -187,6 +187,18 @@ static void kvm_psci_system_reset(struct kvm_vcpu *vcpu) kvm_prepare_system_event(vcpu, KVM_SYSTEM_EVENT_RESET); } +static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) +{ + int i; + + /* + * Zero the input registers' upper 32 bits. They will be fully + * zeroed on exit, so we're fine changing them in place. + */ + for (i = 1; i < 4; i++) + vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); +} + static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -211,12 +223,16 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) val = PSCI_RET_SUCCESS; break; case PSCI_0_2_FN_CPU_ON: + kvm_psci_narrow_to_32bit(vcpu); + fallthrough; case PSCI_0_2_FN64_CPU_ON: mutex_lock(&kvm->lock); val = kvm_psci_vcpu_on(vcpu); mutex_unlock(&kvm->lock); break; case PSCI_0_2_FN_AFFINITY_INFO: + kvm_psci_narrow_to_32bit(vcpu); + fallthrough; case PSCI_0_2_FN64_AFFINITY_INFO: val = kvm_psci_vcpu_affinity_info(vcpu); break;
When a guest delibarately uses an SSMC32 function number (which is allowed), we should make sure we drop the top 32bits from the input arguments, as they could legitimately be junk. Reported-by: Christoffer Dall <christoffer.dall@arm.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/psci.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)