Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits

From: Will Deacon <will@kernel.org>
To: Fuad Tabba <tabba@google.com>
Cc: kernel-team@android.com, kvm@vger.kernel.org, maz@kernel.org,
	pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 14/15] KVM: arm64: Handle protected guests at 32 bits
Date: Thu, 12 Aug 2021 10:57:44 +0100	[thread overview]
Message-ID: <20210812095743.GL5912@willie-the-truck> (raw)
In-Reply-To: <20210719160346.609914-15-tabba@google.com>

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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm