* [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Christoffer recently pointed out that we don't narrow the arguments to SMC32 PSCI functions called by a 64bit guest. This could result in a guest failing to boot its secondary CPUs if it had junk in the upper 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. Whist I was looking at this, it became apparent that we allow a 32bit guest to call 64bit functions, which the spec explicitly forbids. Oh well, another patch. This has been lightly tested, but I feel that we could do with a new set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Marc Zyngier (2): KVM: arm64: PSCI: Narrow input registers when using 32bit functions KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) -- 2.25.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Christoffer recently pointed out that we don't narrow the arguments to SMC32 PSCI functions called by a 64bit guest. This could result in a guest failing to boot its secondary CPUs if it had junk in the upper 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. Whist I was looking at this, it became apparent that we allow a 32bit guest to call 64bit functions, which the spec explicitly forbids. Oh well, another patch. This has been lightly tested, but I feel that we could do with a new set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Marc Zyngier (2): KVM: arm64: PSCI: Narrow input registers when using 32bit functions KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) -- 2.25.0 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose Christoffer recently pointed out that we don't narrow the arguments to SMC32 PSCI functions called by a 64bit guest. This could result in a guest failing to boot its secondary CPUs if it had junk in the upper 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. Whist I was looking at this, it became apparent that we allow a 32bit guest to call 64bit functions, which the spec explicitly forbids. Oh well, another patch. This has been lightly tested, but I feel that we could do with a new set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Marc Zyngier (2): KVM: arm64: PSCI: Narrow input registers when using 32bit functions KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) -- 2.25.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-01 16:58 ` Marc Zyngier -1 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose, Christoffer Dall 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-02 13:47 ` Christoffer Dall -1 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:47 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, linux-arm-kernel, kvmarm 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> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-02 13:47 ` Christoffer Dall 0 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:47 UTC (permalink / raw) To: Marc Zyngier Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Julien Thierry, Suzuki K Poulose 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> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-02 13:47 ` Christoffer Dall 0 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:47 UTC (permalink / raw) To: Marc Zyngier Cc: kvm, Suzuki K Poulose, James Morse, linux-arm-kernel, kvmarm, Julien Thierry 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> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-03 14:02 ` Alexandru Elisei -1 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-03 14:02 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose 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 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions @ 2020-04-03 14:02 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-01 16:58 ` Marc Zyngier -1 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Implementing (and even advertising) 64bit PSCI functions to 32bit guests is at least a bit odd, if not altogether violating the spec which says ("5.2.1 Register usage in arguments and return values"): "Adherence to the SMC Calling Conventions implies that any AArch32 caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). This matches the NOT_SUPPORTED error code used in PSCI" Tighten the implementation by pretending these functions are not there for 32bit guests. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 69ff4a51ceb5..122795cdd984 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); } +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) +{ + switch(fn) { + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_ON: + case PSCI_0_2_FN64_AFFINITY_INFO: + /* Disallow these functions for 32bit guests */ + if (vcpu_mode_is_32bit(vcpu)) + return PSCI_RET_NOT_SUPPORTED; + break; + } + + return 0; +} + static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) unsigned long val; int ret = 1; + val = kvm_psci_check_allowed_function(vcpu, psci_fn); + if (val) + goto out; + switch (psci_fn) { case PSCI_0_2_FN_PSCI_VERSION: /* @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) break; } +out: smccc_set_retval(vcpu, val, 0, 0, 0); return ret; } @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) break; case PSCI_1_0_FN_PSCI_FEATURES: feature = smccc_get_arg1(vcpu); + val = kvm_psci_check_allowed_function(vcpu, feature); + if (val) + break; + switch(feature) { case PSCI_0_2_FN_PSCI_VERSION: case PSCI_0_2_FN_CPU_SUSPEND: -- 2.25.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Implementing (and even advertising) 64bit PSCI functions to 32bit guests is at least a bit odd, if not altogether violating the spec which says ("5.2.1 Register usage in arguments and return values"): "Adherence to the SMC Calling Conventions implies that any AArch32 caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). This matches the NOT_SUPPORTED error code used in PSCI" Tighten the implementation by pretending these functions are not there for 32bit guests. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 69ff4a51ceb5..122795cdd984 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); } +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) +{ + switch(fn) { + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_ON: + case PSCI_0_2_FN64_AFFINITY_INFO: + /* Disallow these functions for 32bit guests */ + if (vcpu_mode_is_32bit(vcpu)) + return PSCI_RET_NOT_SUPPORTED; + break; + } + + return 0; +} + static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) unsigned long val; int ret = 1; + val = kvm_psci_check_allowed_function(vcpu, psci_fn); + if (val) + goto out; + switch (psci_fn) { case PSCI_0_2_FN_PSCI_VERSION: /* @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) break; } +out: smccc_set_retval(vcpu, val, 0, 0, 0); return ret; } @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) break; case PSCI_1_0_FN_PSCI_FEATURES: feature = smccc_get_arg1(vcpu); + val = kvm_psci_check_allowed_function(vcpu, feature); + if (val) + break; + switch(feature) { case PSCI_0_2_FN_PSCI_VERSION: case PSCI_0_2_FN_CPU_SUSPEND: -- 2.25.0 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-01 16:58 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-01 16:58 UTC (permalink / raw) To: linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose Implementing (and even advertising) 64bit PSCI functions to 32bit guests is at least a bit odd, if not altogether violating the spec which says ("5.2.1 Register usage in arguments and return values"): "Adherence to the SMC Calling Conventions implies that any AArch32 caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). This matches the NOT_SUPPORTED error code used in PSCI" Tighten the implementation by pretending these functions are not there for 32bit guests. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 69ff4a51ceb5..122795cdd984 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); } +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) +{ + switch(fn) { + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_ON: + case PSCI_0_2_FN64_AFFINITY_INFO: + /* Disallow these functions for 32bit guests */ + if (vcpu_mode_is_32bit(vcpu)) + return PSCI_RET_NOT_SUPPORTED; + break; + } + + return 0; +} + static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) unsigned long val; int ret = 1; + val = kvm_psci_check_allowed_function(vcpu, psci_fn); + if (val) + goto out; + switch (psci_fn) { case PSCI_0_2_FN_PSCI_VERSION: /* @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) break; } +out: smccc_set_retval(vcpu, val, 0, 0, 0); return ret; } @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) break; case PSCI_1_0_FN_PSCI_FEATURES: feature = smccc_get_arg1(vcpu); + val = kvm_psci_check_allowed_function(vcpu, feature); + if (val) + break; + switch(feature) { case PSCI_0_2_FN_PSCI_VERSION: case PSCI_0_2_FN_CPU_SUSPEND: -- 2.25.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-02 13:48 ` Christoffer Dall -1 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:48 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, linux-arm-kernel, kvmarm On Wed, Apr 01, 2020 at 05:58:16PM +0100, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: > -- > 2.25.0 > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-02 13:48 ` Christoffer Dall 0 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:48 UTC (permalink / raw) To: Marc Zyngier Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Julien Thierry, Suzuki K Poulose On Wed, Apr 01, 2020 at 05:58:16PM +0100, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: > -- > 2.25.0 > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-02 13:48 ` Christoffer Dall 0 siblings, 0 replies; 30+ messages in thread From: Christoffer Dall @ 2020-04-02 13:48 UTC (permalink / raw) To: Marc Zyngier Cc: kvm, Suzuki K Poulose, James Morse, linux-arm-kernel, kvmarm, Julien Thierry On Wed, Apr 01, 2020 at 05:58:16PM +0100, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: > -- > 2.25.0 > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-03 14:02 ` Alexandru Elisei -1 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Hello, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: I checked in ARM DEN 0022D, those are indeed the only 3 functions that KVM implements and have a different function ID based on the calling convention. > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: The patch makes sense to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-03 14:02 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Hello, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: I checked in ARM DEN 0022D, those are indeed the only 3 functions that KVM implements and have a different function ID based on the calling convention. > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: The patch makes sense to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests @ 2020-04-03 14:02 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:02 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose Hello, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: I checked in ARM DEN 0022D, those are indeed the only 3 functions that KVM implements and have a different function ID based on the calling convention. > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: The patch makes sense to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes 2020-04-01 16:58 ` Marc Zyngier (?) @ 2020-04-03 10:35 ` Alexandru Elisei -1 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 10:35 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Hi, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Christoffer recently pointed out that we don't narrow the arguments to > SMC32 PSCI functions called by a 64bit guest. This could result in a > guest failing to boot its secondary CPUs if it had junk in the upper > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > Whist I was looking at this, it became apparent that we allow a 32bit > guest to call 64bit functions, which the spec explicitly forbids. Oh > well, another patch. > > This has been lightly tested, but I feel that we could do with a new > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for Paolo to merge the pull request from Drew, which contains some fixes for the current tests. > > Marc Zyngier (2): > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > I started reviewing the patches and I have a question. I'm probably missing something, but why make the changes to the PSCI code instead of making them in the kvm_hvc_call_handler function? From my understanding of the code, making the changes there would benefit all firmware interface that use SMCCC as the communication protocol, not just PSCI. Thanks, Alex _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 10:35 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 10:35 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Hi, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Christoffer recently pointed out that we don't narrow the arguments to > SMC32 PSCI functions called by a 64bit guest. This could result in a > guest failing to boot its secondary CPUs if it had junk in the upper > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > Whist I was looking at this, it became apparent that we allow a 32bit > guest to call 64bit functions, which the spec explicitly forbids. Oh > well, another patch. > > This has been lightly tested, but I feel that we could do with a new > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for Paolo to merge the pull request from Drew, which contains some fixes for the current tests. > > Marc Zyngier (2): > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > I started reviewing the patches and I have a question. I'm probably missing something, but why make the changes to the PSCI code instead of making them in the kvm_hvc_call_handler function? From my understanding of the code, making the changes there would benefit all firmware interface that use SMCCC as the communication protocol, not just PSCI. Thanks, Alex ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 10:35 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 10:35 UTC (permalink / raw) To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm Cc: James Morse, Christoffer Dall, Julien Thierry, Suzuki K Poulose Hi, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Christoffer recently pointed out that we don't narrow the arguments to > SMC32 PSCI functions called by a 64bit guest. This could result in a > guest failing to boot its secondary CPUs if it had junk in the upper > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > Whist I was looking at this, it became apparent that we allow a 32bit > guest to call 64bit functions, which the spec explicitly forbids. Oh > well, another patch. > > This has been lightly tested, but I feel that we could do with a new > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for Paolo to merge the pull request from Drew, which contains some fixes for the current tests. > > Marc Zyngier (2): > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > I started reviewing the patches and I have a question. I'm probably missing something, but why make the changes to the PSCI code instead of making them in the kvm_hvc_call_handler function? From my understanding of the code, making the changes there would benefit all firmware interface that use SMCCC as the communication protocol, not just PSCI. Thanks, Alex _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes 2020-04-03 10:35 ` Alexandru Elisei (?) @ 2020-04-03 11:20 ` Marc Zyngier -1 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-03 11:20 UTC (permalink / raw) To: Alexandru Elisei; +Cc: kvm, linux-arm-kernel, kvmarm Hi Alexandru, On Fri, 3 Apr 2020 11:35:00 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi, > > On 4/1/20 5:58 PM, Marc Zyngier wrote: > > Christoffer recently pointed out that we don't narrow the arguments to > > SMC32 PSCI functions called by a 64bit guest. This could result in a > > guest failing to boot its secondary CPUs if it had junk in the upper > > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > > > Whist I was looking at this, it became apparent that we allow a 32bit > > guest to call 64bit functions, which the spec explicitly forbids. Oh > > well, another patch. > > > > This has been lightly tested, but I feel that we could do with a new > > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). > > Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for > Paolo to merge the pull request from Drew, which contains some fixes for the > current tests. > > > > > Marc Zyngier (2): > > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > I started reviewing the patches and I have a question. I'm probably missing > something, but why make the changes to the PSCI code instead of making them in the > kvm_hvc_call_handler function? From my understanding of the code, making the > changes there would benefit all firmware interface that use SMCCC as the > communication protocol, not just PSCI. The problem is that it is not obvious whether other functions have similar requirements. For example, the old PSCI 0.1 functions are completely outside of the SMCCC scope (there is no split between 32 and 64bit functions, for example), and there is no generic way to discover the number of arguments that you would want to narrow. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 11:20 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-03 11:20 UTC (permalink / raw) To: Alexandru Elisei Cc: linux-arm-kernel, kvmarm, kvm, Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Hi Alexandru, On Fri, 3 Apr 2020 11:35:00 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi, > > On 4/1/20 5:58 PM, Marc Zyngier wrote: > > Christoffer recently pointed out that we don't narrow the arguments to > > SMC32 PSCI functions called by a 64bit guest. This could result in a > > guest failing to boot its secondary CPUs if it had junk in the upper > > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > > > Whist I was looking at this, it became apparent that we allow a 32bit > > guest to call 64bit functions, which the spec explicitly forbids. Oh > > well, another patch. > > > > This has been lightly tested, but I feel that we could do with a new > > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). > > Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for > Paolo to merge the pull request from Drew, which contains some fixes for the > current tests. > > > > > Marc Zyngier (2): > > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > I started reviewing the patches and I have a question. I'm probably missing > something, but why make the changes to the PSCI code instead of making them in the > kvm_hvc_call_handler function? From my understanding of the code, making the > changes there would benefit all firmware interface that use SMCCC as the > communication protocol, not just PSCI. The problem is that it is not obvious whether other functions have similar requirements. For example, the old PSCI 0.1 functions are completely outside of the SMCCC scope (there is no split between 32 and 64bit functions, for example), and there is no generic way to discover the number of arguments that you would want to narrow. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 11:20 ` Marc Zyngier 0 siblings, 0 replies; 30+ messages in thread From: Marc Zyngier @ 2020-04-03 11:20 UTC (permalink / raw) To: Alexandru Elisei Cc: kvm, Suzuki K Poulose, Christoffer Dall, James Morse, linux-arm-kernel, kvmarm, Julien Thierry Hi Alexandru, On Fri, 3 Apr 2020 11:35:00 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Hi, > > On 4/1/20 5:58 PM, Marc Zyngier wrote: > > Christoffer recently pointed out that we don't narrow the arguments to > > SMC32 PSCI functions called by a 64bit guest. This could result in a > > guest failing to boot its secondary CPUs if it had junk in the upper > > 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. > > > > Whist I was looking at this, it became apparent that we allow a 32bit > > guest to call 64bit functions, which the spec explicitly forbids. Oh > > well, another patch. > > > > This has been lightly tested, but I feel that we could do with a new > > set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). > > Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for > Paolo to merge the pull request from Drew, which contains some fixes for the > current tests. > > > > > Marc Zyngier (2): > > KVM: arm64: PSCI: Narrow input registers when using 32bit functions > > KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests > > > > virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > I started reviewing the patches and I have a question. I'm probably missing > something, but why make the changes to the PSCI code instead of making them in the > kvm_hvc_call_handler function? From my understanding of the code, making the > changes there would benefit all firmware interface that use SMCCC as the > communication protocol, not just PSCI. The problem is that it is not obvious whether other functions have similar requirements. For example, the old PSCI 0.1 functions are completely outside of the SMCCC scope (there is no split between 32 and 64bit functions, for example), and there is no generic way to discover the number of arguments that you would want to narrow. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes 2020-04-03 11:20 ` Marc Zyngier (?) @ 2020-04-03 14:01 ` Alexandru Elisei -1 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:01 UTC (permalink / raw) To: Marc Zyngier; +Cc: kvm, linux-arm-kernel, kvmarm Hi, On 4/3/20 12:20 PM, Marc Zyngier wrote: > Hi Alexandru, > > On Fri, 3 Apr 2020 11:35:00 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi, >> >> On 4/1/20 5:58 PM, Marc Zyngier wrote: >>> Christoffer recently pointed out that we don't narrow the arguments to >>> SMC32 PSCI functions called by a 64bit guest. This could result in a >>> guest failing to boot its secondary CPUs if it had junk in the upper >>> 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. >>> >>> Whist I was looking at this, it became apparent that we allow a 32bit >>> guest to call 64bit functions, which the spec explicitly forbids. Oh >>> well, another patch. >>> >>> This has been lightly tested, but I feel that we could do with a new >>> set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). >> Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for >> Paolo to merge the pull request from Drew, which contains some fixes for the >> current tests. >> >>> Marc Zyngier (2): >>> KVM: arm64: PSCI: Narrow input registers when using 32bit functions >>> KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests >>> >>> virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 40 insertions(+) >>> >> I started reviewing the patches and I have a question. I'm probably missing >> something, but why make the changes to the PSCI code instead of making them in the >> kvm_hvc_call_handler function? From my understanding of the code, making the >> changes there would benefit all firmware interface that use SMCCC as the >> communication protocol, not just PSCI. > The problem is that it is not obvious whether other functions have > similar requirements. For example, the old PSCI 0.1 functions are > completely outside of the SMCCC scope (there is no split between 32 and > 64bit functions, for example), and there is no generic way to discover > the number of arguments that you would want to narrow. You're right, there's really no way to tell if the guest is using SMC32 or SMC64 other than looking at the function IDs, so having the PSCI code do the checking is the right thing to do. Thanks, Alex > > Thanks, > > M. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 14:01 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:01 UTC (permalink / raw) To: Marc Zyngier Cc: linux-arm-kernel, kvmarm, kvm, Christoffer Dall, James Morse, Julien Thierry, Suzuki K Poulose Hi, On 4/3/20 12:20 PM, Marc Zyngier wrote: > Hi Alexandru, > > On Fri, 3 Apr 2020 11:35:00 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi, >> >> On 4/1/20 5:58 PM, Marc Zyngier wrote: >>> Christoffer recently pointed out that we don't narrow the arguments to >>> SMC32 PSCI functions called by a 64bit guest. This could result in a >>> guest failing to boot its secondary CPUs if it had junk in the upper >>> 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. >>> >>> Whist I was looking at this, it became apparent that we allow a 32bit >>> guest to call 64bit functions, which the spec explicitly forbids. Oh >>> well, another patch. >>> >>> This has been lightly tested, but I feel that we could do with a new >>> set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). >> Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for >> Paolo to merge the pull request from Drew, which contains some fixes for the >> current tests. >> >>> Marc Zyngier (2): >>> KVM: arm64: PSCI: Narrow input registers when using 32bit functions >>> KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests >>> >>> virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 40 insertions(+) >>> >> I started reviewing the patches and I have a question. I'm probably missing >> something, but why make the changes to the PSCI code instead of making them in the >> kvm_hvc_call_handler function? From my understanding of the code, making the >> changes there would benefit all firmware interface that use SMCCC as the >> communication protocol, not just PSCI. > The problem is that it is not obvious whether other functions have > similar requirements. For example, the old PSCI 0.1 functions are > completely outside of the SMCCC scope (there is no split between 32 and > 64bit functions, for example), and there is no generic way to discover > the number of arguments that you would want to narrow. You're right, there's really no way to tell if the guest is using SMC32 or SMC64 other than looking at the function IDs, so having the PSCI code do the checking is the right thing to do. Thanks, Alex > > Thanks, > > M. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/2] KVM: arm64: PSCI fixes @ 2020-04-03 14:01 ` Alexandru Elisei 0 siblings, 0 replies; 30+ messages in thread From: Alexandru Elisei @ 2020-04-03 14:01 UTC (permalink / raw) To: Marc Zyngier Cc: kvm, Suzuki K Poulose, Christoffer Dall, James Morse, linux-arm-kernel, kvmarm, Julien Thierry Hi, On 4/3/20 12:20 PM, Marc Zyngier wrote: > Hi Alexandru, > > On Fri, 3 Apr 2020 11:35:00 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > >> Hi, >> >> On 4/1/20 5:58 PM, Marc Zyngier wrote: >>> Christoffer recently pointed out that we don't narrow the arguments to >>> SMC32 PSCI functions called by a 64bit guest. This could result in a >>> guest failing to boot its secondary CPUs if it had junk in the upper >>> 32bits. Yes, this is silly, but the guest is allowed to do that. Duh. >>> >>> Whist I was looking at this, it became apparent that we allow a 32bit >>> guest to call 64bit functions, which the spec explicitly forbids. Oh >>> well, another patch. >>> >>> This has been lightly tested, but I feel that we could do with a new >>> set of PSCI corner cases in KVM-unit-tests (hint, nudge... ;-). >> Good idea. I was already planning to add new PSCI and timer tests, I'm waiting for >> Paolo to merge the pull request from Drew, which contains some fixes for the >> current tests. >> >>> Marc Zyngier (2): >>> KVM: arm64: PSCI: Narrow input registers when using 32bit functions >>> KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests >>> >>> virt/kvm/arm/psci.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 40 insertions(+) >>> >> I started reviewing the patches and I have a question. I'm probably missing >> something, but why make the changes to the PSCI code instead of making them in the >> kvm_hvc_call_handler function? From my understanding of the code, making the >> changes there would benefit all firmware interface that use SMCCC as the >> communication protocol, not just PSCI. > The problem is that it is not obvious whether other functions have > similar requirements. For example, the old PSCI 0.1 functions are > completely outside of the SMCCC scope (there is no split between 32 and > 64bit functions, for example), and there is no generic way to discover > the number of arguments that you would want to narrow. You're right, there's really no way to tell if the guest is using SMC32 or SMC64 other than looking at the function IDs, so having the PSCI code do the checking is the right thing to do. Thanks, Alex > > Thanks, > > M. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-04-03 14:02 UTC | newest] Thread overview: 30+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-01 16:58 [PATCH 0/2] KVM: arm64: PSCI fixes Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-01 16:58 ` [PATCH 1/2] KVM: arm64: PSCI: Narrow input registers when using 32bit functions Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-02 13:47 ` Christoffer Dall 2020-04-02 13:47 ` Christoffer Dall 2020-04-02 13:47 ` Christoffer Dall 2020-04-03 14:02 ` Alexandru Elisei 2020-04-03 14:02 ` Alexandru Elisei 2020-04-03 14:02 ` Alexandru Elisei 2020-04-01 16:58 ` [PATCH 2/2] KVM: arm64: PSCI: Forbid 64bit functions for 32bit guests Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-01 16:58 ` Marc Zyngier 2020-04-02 13:48 ` Christoffer Dall 2020-04-02 13:48 ` Christoffer Dall 2020-04-02 13:48 ` Christoffer Dall 2020-04-03 14:02 ` Alexandru Elisei 2020-04-03 14:02 ` Alexandru Elisei 2020-04-03 14:02 ` Alexandru Elisei 2020-04-03 10:35 ` [PATCH 0/2] KVM: arm64: PSCI fixes Alexandru Elisei 2020-04-03 10:35 ` Alexandru Elisei 2020-04-03 10:35 ` Alexandru Elisei 2020-04-03 11:20 ` Marc Zyngier 2020-04-03 11:20 ` Marc Zyngier 2020-04-03 11:20 ` Marc Zyngier 2020-04-03 14:01 ` Alexandru Elisei 2020-04-03 14:01 ` Alexandru Elisei 2020-04-03 14:01 ` Alexandru Elisei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.