KVM: arm64: Prevent mixed-width VM creation
Subsystems (3)
Marc Zyngier
maintainerPATCH 1/1 KVM: arm64: Prevent mixed-width VM creation
It looks like we have tolerated creating mixed-width VMs since... forever. However, that was never the intention, and we'd rather not have to support that pointless complexity.
Forbid such a setup by making sure all the vcpus have the same register width.
--- arch/arm64/kvm/reset.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
arch/arm64/kvm/reset.c +24 -4
@@ -166,6 +166,25 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)return 0 ; } +static bool vcpu_allowed_register_width (struct kvm_vcpu *vcpu) +{ +struct kvm_vcpu *tmp; +int i; + +/* Check that the vcpus are either all 32bit or all 64bit */ +kvm_for_each_vcpu (i, tmp, vcpu->kvm) { +bool w; + + w =test_bit (KVM_ARM_VCPU_EL1_32BIT , tmp->arch.features); + w ^=test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features); + +if (w) +return false; + } + +return true; +} +/** * kvm_reset_vcpu - sets core registers and sys_regs to reset value * @vcpu: The VCPU pointer
@@ -217,13 +236,14 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) } } +if (!vcpu_allowed_register_width (vcpu)) { + ret = -EINVAL ; + goto out; + } +switch (vcpu->arch.target) {default :if (test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features)) { -if (!cpus_have_const_cap (ARM64_HAS_32BIT_EL1 )) { - ret = -EINVAL ; - goto out; - } pstate =VCPU_RESET_PSTATE_SVC ; }else { pstate =VCPU_RESET_PSTATE_EL1 ;
Signature · 6 lines
2.30.2 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Mark Rutland
reviewerQuoted 27 lines · Marc Zyngier
It looks like we have tolerated creating mixed-width VMs since... forever. However, that was never the intention, and we'd rather not have to support that pointless complexity.
Forbid such a setup by making sure all the vcpus have the same register width.
Reported-by Steven Price <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)arch/arm64/kvm/reset.c +5
-0@@ -166,6 +166,25 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)return 0 ; } +static bool vcpu_allowed_register_width (struct kvm_vcpu *vcpu) +{ +struct kvm_vcpu *tmp; +int i; +
arch/arm64/kvm/reset.c static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu) · line 183
Quoted 10 lines
arch/arm64/kvm/reset.c +10
-0+/* Check that the vcpus are either all 32bit or all 64bit */ +kvm_for_each_vcpu (i, tmp, vcpu->kvm) { +bool w; + + w =test_bit (KVM_ARM_VCPU_EL1_32BIT , tmp->arch.features); + w ^=test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features); + +if (w) +return false; + }
I think this is wrong for a single-cpu VM. In that case, the loop will have a single iteration, and tmp == vcpu, so w must be 0 regardless of the value of arch.features.
IIUC that doesn't prevent KVM_ARM_VCPU_EL1_32BIT being set when we don't have the ARM64_HAS_32BIT_EL1 cap, unless that's checked elsewhere?
How about something like:
| static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
| {
| bool is_32bit = vcpu_features_32bit(vcpu);
| struct kvm_vcpu *tmp;
| int i;
|
| if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is_32bit)
| return false;
|
| kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
| if (is_32bit != vcpu_features_32bit(tmp))
| return false;
| }
|
| return true;
| }... with a helper in <asm/kvm_emulate.h> like:
| static bool vcpu_features_32bit(struct kvm_vcpu *vcpu)
| {
| return test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features);
| }... or
| static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature)
| {
| return test_bit(feature, vcpu->arch.features);
| }... so that we can avoid the line splitting required by the length of the test_bit() expression?
Thanks,
Mark.
Quoted 33 lines
arch/arm64/kvm/reset.c +9
-4+ +return true; +} +/** * kvm_reset_vcpu - sets core registers and sys_regs to reset value * @vcpu: The VCPU pointer @@ -217,13 +236,14 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) } } +if (!vcpu_allowed_register_width (vcpu)) { + ret = -EINVAL ; + goto out; + } +switch (vcpu->arch.target) {default :if (test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features)) { -if (!cpus_have_const_cap (ARM64_HAS_32BIT_EL1 )) { - ret = -EINVAL ; - goto out; - } pstate =VCPU_RESET_PSTATE_SVC ; }else { pstate =VCPU_RESET_PSTATE_EL1 ;Signature · 6 lines
2.30.2 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier
maintainerQuoted 35 lines · Mark Rutland
Quoted 33 lines · Marc Zyngier
It looks like we have tolerated creating mixed-width VMs since... forever. However, that was never the intention, and we'd rather not have to support that pointless complexity.
Forbid such a setup by making sure all the vcpus have the same register width.
Reported-by Steven Price <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)arch/arm64/kvm/reset.c +11
-0@@ -166,6 +166,25 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)return 0 ; } +static bool vcpu_allowed_register_width (struct kvm_vcpu *vcpu) +{ +struct kvm_vcpu *tmp; +int i; + +/* Check that the vcpus are either all 32bit or all 64bit */ +kvm_for_each_vcpu (i, tmp, vcpu->kvm) { +bool w; + + w =test_bit (KVM_ARM_VCPU_EL1_32BIT , tmp->arch.features); + w ^=test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features);
Quoted 8 lines
arch/arm64/kvm/reset.c static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu) · line 183
Quoted 4 lines
arch/arm64/kvm/reset.c +4
-0+ +if (w) +return false; + }I think this is wrong for a single-cpu VM. In that case, the loop will have a single iteration, and tmp == vcpu, so w must be 0 regardless of the value of arch.features.
I don't immediately see what is wrong with a single-cpu VM. 'w' will be zero indeed, and we'll return that this is allowed. After all, each VM starts by being a single-CPU VM.
But of course...
Quoted 2 lines
IIUC that doesn't prevent
KVM_ARM_VCPU_EL1_32BITbeing set when we don't have theARM64_HAS_32BIT_EL1cap, unless that's checked elsewhere?
... I mistakenly removed the check against ARM64_HAS_32BIT_EL1...
Quoted 29 lines
How about something like:
| static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu) | { | bool is_32bit = vcpu_features_32bit(vcpu); | struct kvm_vcpu *tmp; | int i; | | if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is_32bit) | return false; | | kvm_for_each_vcpu(i, tmp, vcpu->kvm) { | if (is_32bit != vcpu_features_32bit(tmp)) | return false; | } | | return true; | }... with a helper in <
asm/kvm_emulate.h> like:| static bool vcpu_features_32bit(struct kvm_vcpu *vcpu) | { | return test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features); | }... or
Quoted 7 lines
| static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature) | { | return test_bit(feature, vcpu->arch.features); | }... so that we can avoid the line splitting required by the length of the
test_bit()expression?
Yup, looks OK to me (with a preference for the latter).
Thanks,
M.
Signature · 5 lines
Without deviation from the norm, progress is not possible. _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Mark Rutland
reviewerQuoted 46 lines · Marc Zyngier
Quoted 35 lines · Mark Rutland
Quoted 33 lines · Marc Zyngier
It looks like we have tolerated creating mixed-width VMs since... forever. However, that was never the intention, and we'd rather not have to support that pointless complexity.
Forbid such a setup by making sure all the vcpus have the same register width.
Reported-by Steven Price <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/reset.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)arch/arm64/kvm/reset.c +11
-0@@ -166,6 +166,25 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)return 0 ; } +static bool vcpu_allowed_register_width (struct kvm_vcpu *vcpu) +{ +struct kvm_vcpu *tmp; +int i; + +/* Check that the vcpus are either all 32bit or all 64bit */ +kvm_for_each_vcpu (i, tmp, vcpu->kvm) { +bool w; + + w =test_bit (KVM_ARM_VCPU_EL1_32BIT , tmp->arch.features); + w ^=test_bit (KVM_ARM_VCPU_EL1_32BIT , vcpu->arch.features);Quoted 8 lines
arch/arm64/kvm/reset.c static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu) · line 183
Quoted 4 lines
arch/arm64/kvm/reset.c +4
-0+ +if (w) +return false; + }I think this is wrong for a single-cpu VM. In that case, the loop will have a single iteration, and tmp == vcpu, so w must be 0 regardless of the value of arch.features.
Quoted 3 lines
I don't immediately see what is wrong with a single-cpu VM. 'w' will be zero indeed, and we'll return that this is allowed. After all, each VM starts by being a single-CPU VM.
Sorry; I should have been clearer. I had assumed that this was trying to rely on a difference across vcpus implicitly providing an equivalent of the removed check for the KVM_ARM_VCPU_EL1_32BIT cap. I guess from the below that was not the case. :)
Thanks,
Mark.
Quoted 41 lines
But of course...
Quoted 2 lines
IIUC that doesn't prevent
KVM_ARM_VCPU_EL1_32BITbeing set when we don't have theARM64_HAS_32BIT_EL1cap, unless that's checked elsewhere?... I mistakenly removed the check against
ARM64_HAS_32BIT_EL1...Quoted 34 lines
How about something like:
| static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu) | { | bool is_32bit = vcpu_features_32bit(vcpu); | struct kvm_vcpu *tmp; | int i; | | if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is_32bit) | return false; | | kvm_for_each_vcpu(i, tmp, vcpu->kvm) { | if (is_32bit != vcpu_features_32bit(tmp)) | return false; | } | | return true; | }... with a helper in <
asm/kvm_emulate.h> like:| static bool vcpu_features_32bit(struct kvm_vcpu *vcpu) | { | return test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features); | }... or
| static inline bool vcpu_has_feature(struct kvm_vcpu *vcpu, int feature) | { | return test_bit(feature, vcpu->arch.features); | }
Quoted 11 lines
Quoted 2 lines
... so that we can avoid the line splitting required by the length of the
test_bit()expression?Yup, looks OK to me (with a preference for the latter).
Thanks,
M.Signature · 1 line
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm