| Message ID | 20210719160346.609914-15-tabba@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fixed features for protected VMs | expand |
On Mon, Jul 19, 2021 at 9:04 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. > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > AArch32 systems") > > Signed-off-by: Fuad Tabba <tabba@google.com> Would it make sense to document how we handle misbehaved guests, in case a particular VMM wants to clean up the mess afterwards? -- Thanks, Oliver > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 8431f1514280..f09343e15a80 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -23,6 +23,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> > @@ -477,6 +478,29 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR); > } > > + /* > + * 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 doesn't 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. > + */ > + if (unlikely(is_nvhe_hyp_code() && > + kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && > + FIELD_GET(FEATURE(ID_AA64PFR0_EL0), > + PVM_ID_AA64PFR0_ALLOW) < > + ID_AA64PFR0_ELx_32BIT_64BIT && > + vcpu_mode_is_32bit(vcpu))) { > + /* > + * As we have caught the guest red-handed, decide that it isn't > + * fit for purpose anymore by making the vcpu invalid. > + */ > + vcpu->arch.target = -1; > + *exit_code = ARM_EXCEPTION_IL; > + goto exit; > + } > + > /* > * We're using the raw exception code in order to only process > * the trap if no SError is pending. We will come back to the > -- > 2.32.0.402.g57bb445576-goog > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Hi Oliver, On Mon, Jul 19, 2021 at 8:43 PM Oliver Upton <oupton@google.com> wrote: > > On Mon, Jul 19, 2021 at 9:04 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. > > > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > > AArch32 systems") > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > Would it make sense to document how we handle misbehaved guests, in > case a particular VMM wants to clean up the mess afterwards? I agree, especially since with this patch this could happen in more than one place. Thanks, /fuad > -- > Thanks, > Oliver > > > --- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 8431f1514280..f09343e15a80 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -23,6 +23,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> > > @@ -477,6 +478,29 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > > write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR); > > } > > > > + /* > > + * 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 doesn't 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. > > + */ > > + if (unlikely(is_nvhe_hyp_code() && > > + kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && > > + FIELD_GET(FEATURE(ID_AA64PFR0_EL0), > > + PVM_ID_AA64PFR0_ALLOW) < > > + ID_AA64PFR0_ELx_32BIT_64BIT && > > + vcpu_mode_is_32bit(vcpu))) { > > + /* > > + * As we have caught the guest red-handed, decide that it isn't > > + * fit for purpose anymore by making the vcpu invalid. > > + */ > > + vcpu->arch.target = -1; > > + *exit_code = ARM_EXCEPTION_IL; > > + goto exit; > > + } > > + > > /* > > * We're using the raw exception code in order to only process > > * the trap if no SError is pending. We will come back to the > > -- > > 2.32.0.402.g57bb445576-goog > > > > _______________________________________________ > > kvmarm mailing list > > kvmarm@lists.cs.columbia.edu > > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Mon, Jul 19, 2021 at 05:03:45PM +0100, Fuad Tabba 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. > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > AArch32 systems") > > Signed-off-by: Fuad Tabba <tabba@google.com> > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 8431f1514280..f09343e15a80 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -23,6 +23,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> > @@ -477,6 +478,29 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR); > } > > + /* > + * 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 doesn't 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. > + */ > + if (unlikely(is_nvhe_hyp_code() && > + kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && > + FIELD_GET(FEATURE(ID_AA64PFR0_EL0), > + PVM_ID_AA64PFR0_ALLOW) < > + ID_AA64PFR0_ELx_32BIT_64BIT && > + vcpu_mode_is_32bit(vcpu))) { > + /* > + * As we have caught the guest red-handed, decide that it isn't > + * fit for purpose anymore by making the vcpu invalid. > + */ > + vcpu->arch.target = -1; > + *exit_code = ARM_EXCEPTION_IL; > + goto exit; > + } Would this be better off inside the nvhe-specific run loop? Seems like we could elide fixup_guest_exit() altogether if we've detect that we're in AArch32 state when we shouldn't be and it would keep the code off the shared path. Will
Hi Will, On Thu, Aug 12, 2021 at 11:57 AM Will Deacon <will@kernel.org> wrote: > > On Mon, Jul 19, 2021 at 05:03:45PM +0100, Fuad Tabba 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. > > > > Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric > > AArch32 systems") > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > --- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 8431f1514280..f09343e15a80 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -23,6 +23,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> > > @@ -477,6 +478,29 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) > > write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR); > > } > > > > + /* > > + * 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 doesn't 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. > > + */ > > + if (unlikely(is_nvhe_hyp_code() && > > + kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && > > + FIELD_GET(FEATURE(ID_AA64PFR0_EL0), > > + PVM_ID_AA64PFR0_ALLOW) < > > + ID_AA64PFR0_ELx_32BIT_64BIT && > > + vcpu_mode_is_32bit(vcpu))) { > > + /* > > + * As we have caught the guest red-handed, decide that it isn't > > + * fit for purpose anymore by making the vcpu invalid. > > + */ > > + vcpu->arch.target = -1; > > + *exit_code = ARM_EXCEPTION_IL; > > + goto exit; > > + } > > Would this be better off inside the nvhe-specific run loop? Seems like we > could elide fixup_guest_exit() altogether if we've detect that we're in > AArch32 state when we shouldn't be and it would keep the code off the shared > path. Yes, it makes more sense and would result in cleaner code to have it there, especially in the future where there's likely to be a separate run loop for protected VMs. I'll move it. Thanks, /fuad > Will
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 8431f1514280..f09343e15a80 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -23,6 +23,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> @@ -477,6 +478,29 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code) write_sysreg_el2(read_sysreg_el2(SYS_ELR) - 4, SYS_ELR); } + /* + * 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 doesn't 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. + */ + if (unlikely(is_nvhe_hyp_code() && + kvm_vm_is_protected(kern_hyp_va(vcpu->kvm)) && + FIELD_GET(FEATURE(ID_AA64PFR0_EL0), + PVM_ID_AA64PFR0_ALLOW) < + ID_AA64PFR0_ELx_32BIT_64BIT && + vcpu_mode_is_32bit(vcpu))) { + /* + * As we have caught the guest red-handed, decide that it isn't + * fit for purpose anymore by making the vcpu invalid. + */ + vcpu->arch.target = -1; + *exit_code = ARM_EXCEPTION_IL; + goto exit; + } + /* * We're using the raw exception code in order to only process * the trap if no SError is pending. We will come back to the
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. Adapted from commit 22f553842b14 ("KVM: arm64: Handle Asymmetric AArch32 systems") Signed-off-by: Fuad Tabba <tabba@google.com> --- arch/arm64/kvm/hyp/include/hyp/switch.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)