Re: [PATCH v2 1/2] KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs

[Marc Zyngier <maz@kernel.org>]

On Tue, 18 Jan 2022 04:19:22 +0000,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> KVM allows userspace to configure either all 32bit or 64bit vCPUs
> for a guest.  At vCPU reset, vcpu_allowed_register_width() checks
> if the vcpu's register width is consistent with all other vCPUs'.
> Since the checking is done even against vCPUs that are not initialized
> (KVM_ARM_VCPU_INIT has not been done) yet, the uninitialized vCPUs
> are erroneously treated as 64bit vCPU, which causes the function to
> incorrectly detect a mixed-width VM.
> 
> Introduce a new flag (el1_reg_width) in kvm_arch to indicates that
> the guest needs to be configured with all 32bit or 64bit vCPUs,
> and initialize it at the first KVM_ARM_VCPU_INIT for the guest.
> Check vcpu's register width against the flag at the vcpu's
> KVM_ARM_VCPU_INIT (instead of against other vCPUs' register width).
> 
> Fixes: 66e94d5cafd4 ("KVM: arm64: Prevent mixed-width VM creation")
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 13 +++++++++++++
>  arch/arm64/kvm/arm.c              | 30 ++++++++++++++++++++++++++++++
>  arch/arm64/kvm/reset.c            |  8 --------
>  3 files changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 2a5f7f38006f..c02b7caf2c82 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -102,6 +102,12 @@ struct kvm_s2_mmu {
>  struct kvm_arch_memory_slot {
>  };
>  
> +enum kvm_el1_reg_width {
> +	EL1_WIDTH_UNINITIALIZED = 0,
> +	EL1_32BIT,
> +	EL1_64BIT,
> +};
> +
>  struct kvm_arch {
>  	struct kvm_s2_mmu mmu;
>  
> @@ -137,6 +143,13 @@ struct kvm_arch {
>  
>  	/* Memory Tagging Extension enabled for the guest */
>  	bool mte_enabled;
> +
> +	/*
> +	 * EL1 register width for the guest.
> +	 * This is set at the first KVM_ARM_VCPU_INIT for the guest based
> +	 * on whether the vcpu has KVM_ARM_VCPU_EL1_32BIT or not.
> +	 */
> +	enum kvm_el1_reg_width el1_reg_width;

I really don't like that we need to keep track of yet another bit of
state on top of the existing one. Duplicating state is a source of
bugs, because you always end up checking the wrong one at the wrong
time (and I have scars to prove it).

>  };
>  
>  struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e4727dc771bf..54ae8bf9d187 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1058,6 +1058,32 @@ int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
>  	return -EINVAL;
>  }
>  
> +/*
> + * A guest can have either all 32bit or 64bit vcpus only.

That's not strictly true. All we are enforcing is that EL1 is either
32 or 64bit.

> + * Either one the guest has is indicated in kvm->arch.el1_reg_width.
> + * Check if the vcpu's register width is consistent with
> + * kvm->arch.el1_reg_width.  If kvm->arch.el1_reg_width is not set yet,
> + * set it based on the vcpu's KVM_ARM_VCPU_EL1_32BIT configuration.
> + */
> +static int kvm_register_width_check_or_init(struct kvm_vcpu *vcpu)
> +{
> +	bool is32bit;
> +	bool allowed = true;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> +
> +	mutex_lock(&kvm->lock);
> +
> +	if (kvm->arch.el1_reg_width == EL1_WIDTH_UNINITIALIZED)
> +		kvm->arch.el1_reg_width = is32bit ? EL1_32BIT : EL1_64BIT;
> +	else
> +		allowed = (is32bit == (kvm->arch.el1_reg_width == EL1_32BIT));
> +
> +	mutex_unlock(&kvm->lock);
> +	return allowed ? 0 : -EINVAL;
> +}
> +
>  static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  			       const struct kvm_vcpu_init *init)
>  {
> @@ -1097,6 +1123,10 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>  
>  	/* Now we know what it is, we can reset it. */
>  	ret = kvm_reset_vcpu(vcpu);
> +
> +	if (!ret)
> +		ret = kvm_register_width_check_or_init(vcpu);
> +
>  	if (ret) {
>  		vcpu->arch.target = -1;
>  		bitmap_zero(vcpu->arch.features, KVM_VCPU_MAX_FEATURES);
> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> index 426bd7fbc3fd..dbf2939a6a96 100644
> --- a/arch/arm64/kvm/reset.c
> +++ b/arch/arm64/kvm/reset.c
> @@ -168,9 +168,7 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
>  
>  static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_vcpu *tmp;
>  	bool is32bit;
> -	int i;
>  
>  	is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
>  	if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> @@ -180,12 +178,6 @@ static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
>  	if (kvm_has_mte(vcpu->kvm) && is32bit)
>  		return false;
>  
> -	/* Check that the vcpus are either all 32bit or all 64bit */
> -	kvm_for_each_vcpu(i, tmp, vcpu->kvm) {
> -		if (vcpu_has_feature(tmp, KVM_ARM_VCPU_EL1_32BIT) != is32bit)
> -			return false;
> -	}
> -

In [1], I suggested another approach that didn't require extra state,
and moved the existing checks under the kvm lock. What was wrong with
that approach?

Thanks,

	M.

[1] https://lore.kernel.org/r/875yqqtn5q.wl-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.