Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
Hi Will,
On Thu, Aug 12, 2021 at 11:57 AM Will Deacon 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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
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
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
Hi Oliver,
On Mon, Jul 19, 2021 at 8:43 PM Oliver Upton wrote:
>
> On Mon, Jul 19, 2021 at 9:04 AM 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
>
> 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
On Mon, Jul 19, 2021 at 9:04 AM 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
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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
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
---
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
#include
#include
+#include
#include
#include
#include
@@ -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
