| Message ID | 20210817081134.2918285-16-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fixed features for protected VMs | expand |
Hi Fuad, On Tue, Aug 17, 2021 at 1:12 AM Fuad Tabba <tabba@google.com> wrote: > > Protected KVM does not support protected AArch32 guests. However, > it is possible for the guest to force run AArch32, potentially > causing problems. Add an extra check so that if the hypervisor > catches the guest doing that, it can prevent the guest from > running again by resetting vcpu->arch.target and returning > ARM_EXCEPTION_IL. > > If this were to happen, The VMM can try and fix it by re- > initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is > likely not possible for protected VMs. > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > AArch32 systems") > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > index 398e62098898..0c24b7f473bf 100644 > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > @@ -20,6 +20,7 @@ > #include <asm/kprobes.h> > #include <asm/kvm_asm.h> > #include <asm/kvm_emulate.h> > +#include <asm/kvm_fixed_config.h> > #include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > #include <asm/fpsimd.h> > @@ -195,6 +196,39 @@ exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu) > return NULL; > } > > +/* > + * Some guests (e.g., protected VMs) might not be allowed to run in AArch32. The > + * check below is based on the one in kvm_arch_vcpu_ioctl_run(). > + * The ARMv8 architecture does not give the hypervisor a mechanism to prevent a > + * guest from dropping to AArch32 EL0 if implemented by the CPU. If the > + * hypervisor spots a guest in such a state ensure it is handled, and don't > + * trust the host to spot or fix it. > + * > + * Returns true if the check passed and the guest run loop can continue, or > + * false if the guest should exit to the host. > + */ > +static bool check_aarch32_guest(struct kvm_vcpu *vcpu, u64 *exit_code) This does a bit more than just check & return, so maybe call it handle_aarch32_guest()? > +{ > + if (kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && maybe initialize a local with a hyp pointer to the kvm structure. > + vcpu_mode_is_32bit(vcpu) && > + FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL0), > + PVM_ID_AA64PFR0_RESTRICT_UNSIGNED) < > + ID_AA64PFR0_ELx_32BIT_64BIT) { It may be more readable to initialize a local variable with this feature check, i.e: bool aarch32_allowed = FIELD_GET(...) == ID_AA64PFR0_ELx_32BIT_64BIT; and then: if (kvm_vm_is_protected(kvm) && vcpu_mode_is_32bit(vcpu) && !aarch32_allowed) { > + /* > + * As we have caught the guest red-handed, decide that it isn't > + * fit for purpose anymore by making the vcpu invalid. The VMM > + * can try and fix it by re-initializing the vcpu with > + * KVM_ARM_VCPU_INIT, however, this is likely not possible for > + * protected VMs. > + */ > + vcpu->arch.target = -1; > + *exit_code = ARM_EXCEPTION_IL; > + return false; > + } > + > + return true; > +} > + > /* Switch to the guest for legacy non-VHE systems */ > int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > { > @@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > /* Jump in the fire! */ > exit_code = __guest_enter(vcpu); > > + if (unlikely(!check_aarch32_guest(vcpu, &exit_code))) > + break; > + > /* And we're baaack! */ > } while (fixup_guest_exit(vcpu, &exit_code)); > > -- > 2.33.0.rc1.237.g0d66db33f3-goog >
Hi Oliver, On Thu, Aug 19, 2021 at 9:10 AM Oliver Upton <oupton@google.com> wrote: > > Hi Fuad, > > On Tue, Aug 17, 2021 at 1:12 AM Fuad Tabba <tabba@google.com> wrote: > > > > Protected KVM does not support protected AArch32 guests. However, > > it is possible for the guest to force run AArch32, potentially > > causing problems. Add an extra check so that if the hypervisor > > catches the guest doing that, it can prevent the guest from > > running again by resetting vcpu->arch.target and returning > > ARM_EXCEPTION_IL. > > > > If this were to happen, The VMM can try and fix it by re- > > initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is > > likely not possible for protected VMs. > > > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > > AArch32 systems") > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c > > index 398e62098898..0c24b7f473bf 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/switch.c > > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c > > @@ -20,6 +20,7 @@ > > #include <asm/kprobes.h> > > #include <asm/kvm_asm.h> > > #include <asm/kvm_emulate.h> > > +#include <asm/kvm_fixed_config.h> > > #include <asm/kvm_hyp.h> > > #include <asm/kvm_mmu.h> > > #include <asm/fpsimd.h> > > @@ -195,6 +196,39 @@ exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu) > > return NULL; > > } > > > > +/* > > + * Some guests (e.g., protected VMs) might not be allowed to run in AArch32. The > > + * check below is based on the one in kvm_arch_vcpu_ioctl_run(). > > + * The ARMv8 architecture does not give the hypervisor a mechanism to prevent a > > + * guest from dropping to AArch32 EL0 if implemented by the CPU. If the > > + * hypervisor spots a guest in such a state ensure it is handled, and don't > > + * trust the host to spot or fix it. > > + * > > + * Returns true if the check passed and the guest run loop can continue, or > > + * false if the guest should exit to the host. > > + */ > > +static bool check_aarch32_guest(struct kvm_vcpu *vcpu, u64 *exit_code) > > This does a bit more than just check & return, so maybe call it > handle_aarch32_guest()? > > > +{ > > + if (kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && > > maybe initialize a local with a hyp pointer to the kvm structure. Will do. > > + vcpu_mode_is_32bit(vcpu) && > > + FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL0), > > + PVM_ID_AA64PFR0_RESTRICT_UNSIGNED) < > > + ID_AA64PFR0_ELx_32BIT_64BIT) { > > It may be more readable to initialize a local variable with this > feature check, i.e: > > bool aarch32_allowed = FIELD_GET(...) == ID_AA64PFR0_ELx_32BIT_64BIT; > > and then: > > if (kvm_vm_is_protected(kvm) && vcpu_mode_is_32bit(vcpu) && > !aarch32_allowed) { I agree. Thanks, /fuad > > + /* > > + * As we have caught the guest red-handed, decide that it isn't > > + * fit for purpose anymore by making the vcpu invalid. The VMM > > + * can try and fix it by re-initializing the vcpu with > > + * KVM_ARM_VCPU_INIT, however, this is likely not possible for > > + * protected VMs. > > + */ > > + vcpu->arch.target = -1; > > + *exit_code = ARM_EXCEPTION_IL; > > + return false; > > + } > > + > > + return true; > > +} > > + > > /* Switch to the guest for legacy non-VHE systems */ > > int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > > { > > @@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) > > /* Jump in the fire! */ > > exit_code = __guest_enter(vcpu); > > > > + if (unlikely(!check_aarch32_guest(vcpu, &exit_code))) > > + break; > > + > > /* And we're baaack! */ > > } while (fixup_guest_exit(vcpu, &exit_code)); > > > > -- > > 2.33.0.rc1.237.g0d66db33f3-goog > >
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c index 398e62098898..0c24b7f473bf 100644 --- a/arch/arm64/kvm/hyp/nvhe/switch.c +++ b/arch/arm64/kvm/hyp/nvhe/switch.c @@ -20,6 +20,7 @@ #include <asm/kprobes.h> #include <asm/kvm_asm.h> #include <asm/kvm_emulate.h> +#include <asm/kvm_fixed_config.h> #include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> #include <asm/fpsimd.h> @@ -195,6 +196,39 @@ exit_handle_fn kvm_get_nvhe_exit_handler(struct kvm_vcpu *vcpu) return NULL; } +/* + * Some guests (e.g., protected VMs) might not be allowed to run in AArch32. The + * check below is based on the one in kvm_arch_vcpu_ioctl_run(). + * The ARMv8 architecture does not give the hypervisor a mechanism to prevent a + * guest from dropping to AArch32 EL0 if implemented by the CPU. If the + * hypervisor spots a guest in such a state ensure it is handled, and don't + * trust the host to spot or fix it. + * + * Returns true if the check passed and the guest run loop can continue, or + * false if the guest should exit to the host. + */ +static bool check_aarch32_guest(struct kvm_vcpu *vcpu, u64 *exit_code) +{ + if (kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && + vcpu_mode_is_32bit(vcpu) && + FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL0), + PVM_ID_AA64PFR0_RESTRICT_UNSIGNED) < + ID_AA64PFR0_ELx_32BIT_64BIT) { + /* + * As we have caught the guest red-handed, decide that it isn't + * fit for purpose anymore by making the vcpu invalid. The VMM + * can try and fix it by re-initializing the vcpu with + * KVM_ARM_VCPU_INIT, however, this is likely not possible for + * protected VMs. + */ + vcpu->arch.target = -1; + *exit_code = ARM_EXCEPTION_IL; + return false; + } + + return true; +} + /* Switch to the guest for legacy non-VHE systems */ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) { @@ -255,6 +289,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu) /* Jump in the fire! */ exit_code = __guest_enter(vcpu); + if (unlikely(!check_aarch32_guest(vcpu, &exit_code))) + break; + /* And we're baaack! */ } while (fixup_guest_exit(vcpu, &exit_code));
Protected KVM does not support protected AArch32 guests. However, it is possible for the guest to force run AArch32, potentially causing problems. Add an extra check so that if the hypervisor catches the guest doing that, it can prevent the guest from running again by resetting vcpu->arch.target and returning ARM_EXCEPTION_IL. If this were to happen, The VMM can try and fix it by re- initializing the vcpu with KVM_ARM_VCPU_INIT, however, this is likely not possible for protected VMs. Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric AArch32 systems") Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/nvhe/switch.c | 37 ++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)