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

[Oliver Upton <oupton@google.com>]

On Tue, Mar 15, 2022 at 09:22:10PM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> On Tue, Mar 15, 2022 at 12:48 AM Oliver Upton <oupton@google.com> wrote:
> > 
> > On Mon, Mar 14, 2022 at 11:19 PM Reiji Watanabe <reijiw@google.com> wrote:
> > >
> > > Hi Oliver,
> > >
> > > On 3/14/22 1:22 PM, Oliver Upton wrote:
> > > > On Sun, Mar 13, 2022 at 11:19:58PM -0700, Reiji Watanabe wrote:
> > > >> KVM allows userspace to configure either all EL1 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 KVM_ARCH_FLAG_EL1_32BIT and KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED
> > > >> bits for kvm->arch.flags.  A value of the EL1_32BIT bit indicates that
> > > >> the guest needs to be configured with all 32bit or 64bit vCPUs, and
> > > >> a value of the REG_WIDTH_CONFIGURED bit indicates if a value of the
> > > >> EL1_32BIT bit is valid (already set up). Values in those bits are set at
> > > >> the first KVM_ARM_VCPU_INIT for the guest based on KVM_ARM_VCPU_EL1_32BIT
> > > >> configuration for the vCPU.
> > > >>
> > > >> Check vcpu's register width against those new bits 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>
> > > >
> > > > Hrmph... I hate to be asking this question so late in the game, but...
> > > >
> > > > Are there any bits that we really allow variation per-vCPU besides
> > > > KVM_ARM_VCPU_POWER_OFF? We unintentionally allow for variance with the
> > > > KVM_ARM_VCPU_PSCI_0_2 bit even though that's complete nonsense.
> > > >
> > > > Stated plainly, should we just deny any attempts at asymmetry besides
> > > > POWER_OFF?>
> > > > Besides the nits, I see nothing objectionable with the patch. I'd really
> > > > like to see more generalized constraints on vCPU configuration, but if
> > > > this is the route we take:
> > >
> > > Prohibiting the mixed width configuration is not a new constraint that
> > > this patch creates (this patch fixes a bug that erroneously detects
> > > mixed-width configuration), and enforcing symmetry of other features
> > > among vCPUs is a bit different matter.
> > 
> > Right, I had managed to forget that context for a moment when I
> > replied to you. Then I fully agree with this patch, and the other
> > feature flags can be handled later.
> > 
> > >
> > > Having said that, I like the idea, which will be more consistent with
> > > my ID register series (it can simplify things).  But, I'm not sure
> > > if creating the constraint for those features would be a problem for
> > > existing userspace even if allowing variation per-vCPU for the features
> > > was not our intention.
> > > I would guess having the constraint for KVM_ARM_VCPU_PSCI_0_2 should
> > > be fine.  Do you think that should be fine for PMU, SVE, and PTRAUTH*
> > > as well ?
> > 
> > Personally, yes, but it prompts the question of if we could break
> > userspace by applying restrictions after the fact. The original patch
> > that applied the register width restrictions didn't cause much of a
> > stir, so it seems possible we could get away with it.
> 
> 
> I agree that it's possible we might get away with it, and I can try
> that for the other features besides KVM_ARM_VCPU_POWER_OFF :)
> (I will work it on separately from this series)
> 

Oh, that'd be great! I was just whining publicly :-)

> BTW, if there had been a general interface to configure per-VM feature,
> I would guess that interface might have been chosen for PSCI_0_2.
> Perhaps we should consider creating it the next time per-VM feature
> is introduced.
> 

I believe there is a lot in KVM we could go back and do better if we had
the patience for it ;-) On a more serious note, I do agree that we need
better mechanisms for VM-scoped bits going forward. 

> Thanks,
> Reiji
> 
> 
> > 
> > > >
> > > > Reviewed-by: Oliver Upton <oupton@google.com>
> > > >
> > > >> ---
> > > >>   arch/arm64/include/asm/kvm_emulate.h | 27 ++++++++----
> > > >>   arch/arm64/include/asm/kvm_host.h    |  9 ++++
> > > >>   arch/arm64/kvm/reset.c               | 64 ++++++++++++++++++----------
> > > >>   3 files changed, 70 insertions(+), 30 deletions(-)
> > > >>
> > > >> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > >> index d62405ce3e6d..7496deab025a 100644
> > > >> --- a/arch/arm64/include/asm/kvm_emulate.h
> > > >> +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > >> @@ -43,10 +43,22 @@ void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr);
> > > >>
> > > >>   void kvm_vcpu_wfi(struct kvm_vcpu *vcpu);
> > > >>
> > > >> +#if defined(__KVM_VHE_HYPERVISOR__) || defined(__KVM_NVHE_HYPERVISOR__)
> > > >>   static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > > >>   {
> > > >>      return !(vcpu->arch.hcr_el2 & HCR_RW);
> > > >>   }
> > > >> +#else
> > > >> +static __always_inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu)
> > > >> +{
> > > >> +    struct kvm *kvm = vcpu->kvm;
> > > >> +
> > > >> +    WARN_ON_ONCE(!test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED,
> > > >> +                           &kvm->arch.flags));
> > > >> +
> > > >> +    return test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > > >> +}
> > > >> +#endif
> > > >>
> > > >>   static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >>   {
> > > >> @@ -72,15 +84,14 @@ static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu)
> > > >>              vcpu->arch.hcr_el2 |= HCR_TVM;
> > > >>      }
> > > >>
> > > >> -    if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features))
> > > >> +    if (vcpu_el1_is_32bit(vcpu))
> > > >>              vcpu->arch.hcr_el2 &= ~HCR_RW;
> > > >> -
> > > >> -    /*
> > > >> -     * TID3: trap feature register accesses that we virtualise.
> > > >> -     * For now this is conditional, since no AArch32 feature regs
> > > >> -     * are currently virtualised.
> > > >> -     */
> > > >> -    if (!vcpu_el1_is_32bit(vcpu))
> > > >> +    else
> > > >> +            /*
> > > >> +             * TID3: trap feature register accesses that we virtualise.
> > > >> +             * For now this is conditional, since no AArch32 feature regs
> > > >> +             * are currently virtualised.
> > > >> +             */
> > > >>              vcpu->arch.hcr_el2 |= HCR_TID3;
> > > >>
> > > >>      if (cpus_have_const_cap(ARM64_MISMATCHED_CACHE_TYPE) ||
> > > >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > >> index 11a7ae747ded..22ad977069f5 100644
> > > >> --- a/arch/arm64/include/asm/kvm_host.h
> > > >> +++ b/arch/arm64/include/asm/kvm_host.h
> > > >> @@ -125,6 +125,15 @@ struct kvm_arch {
> > > >>   #define KVM_ARCH_FLAG_RETURN_NISV_IO_ABORT_TO_USER 0
> > > >>      /* Memory Tagging Extension enabled for the guest */
> > > >>   #define KVM_ARCH_FLAG_MTE_ENABLED                  1
> > > >> +    /*
> > > >> +     * The following two bits are used to indicate the guest's EL1
> > > >> +     * register width configuration. A value of KVM_ARCH_FLAG_EL1_32BIT
> > > >> +     * bit is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED is set.
> > > >> +     * Otherwise, the guest's EL1 register width has not yet been
> > > >> +     * determined yet.
> > > >> +     */
> > > >> +#define KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED          2
> > > >> +#define KVM_ARCH_FLAG_EL1_32BIT                             3
> > > >>      unsigned long flags;
> > > >>
> > > >>      /*
> > > >> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
> > > >> index ecc40c8cd6f6..cbeb6216ee25 100644
> > > >> --- a/arch/arm64/kvm/reset.c
> > > >> +++ b/arch/arm64/kvm/reset.c
> > > >> @@ -181,27 +181,45 @@ static int kvm_vcpu_enable_ptrauth(struct kvm_vcpu *vcpu)
> > > >>      return 0;
> > > >>   }
> > > >>
> > > >> -static bool vcpu_allowed_register_width(struct kvm_vcpu *vcpu)
> > > >> +/*
> > > >> + * A guest can have either all EL1 32bit or 64bit vcpus only. It is
> > > >> + * indicated by a value of KVM_ARCH_FLAG_EL1_32BIT bit in kvm->arch.flags,
> > > >> + * which is valid only when KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED in
> > > >> + * kvm->arch.flags is set.
> > > >> + * This function sets the EL1_32BIT bit based on the given @is32bit (and
> > > >> + * sets REG_WIDTH_CONFIGURED bit). When those flags are already set,
> > > >> + * @is32bit must be consistent with the flags.
> > > >> + * Returns 0 on success, or non-zero otherwise.
> > > >> + */
> > > >
> > > > nit: use kerneldoc style:
> > > >
> > > >    https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
> > >
> > > Sure, I can fix the comment to use kerneldoc style.
> > >
> > >
> > > >
> > > >> +static int kvm_set_vm_width(struct kvm *kvm, bool is32bit)
> > > >>   {
> > > >> -    struct kvm_vcpu *tmp;
> > > >> -    bool is32bit;
> > > >> -    unsigned long i;
> > > >> +    bool allowed;
> > > >> +
> > > >> +    lockdep_assert_held(&kvm->lock);
> > > >> +
> > > >> +    if (test_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags)) {
> > > >> +            /*
> > > >> +             * The guest's register width is already configured.
> > > >> +             * Make sure that @is32bit is consistent with it.
> > > >> +             */
> > > >> +            allowed = (is32bit ==
> > > >> +                       test_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags));
> > > >> +            return allowed ? 0 : -EINVAL;
> > > >
> > > > nit: I'd avoid the ternary and just use a boring if/else (though I could
> > > > be in the minority here).
> > >
> > > I agree with you and will fix it.
> > > (The ternary with 'allowed' was just copied from the previous patch,
> > >   and I should have changed that in this patch...)
> > >
> > > Thanks,
> > > Reiji
> > >
> > >
> > > >
> > > >> +    }
> > > >>
> > > >> -    is32bit = vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT);
> > > >>      if (!cpus_have_const_cap(ARM64_HAS_32BIT_EL1) && is32bit)
> > > >> -            return false;
> > > >> +            return -EINVAL;
> > > >>
> > > >>      /* MTE is incompatible with AArch32 */
> > > >> -    if (kvm_has_mte(vcpu->kvm) && is32bit)
> > > >> -            return false;
> > > >> +    if (kvm_has_mte(kvm) && is32bit)
> > > >> +            return -EINVAL;
> > > >>
> > > >> -    /* 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;
> > > >> -    }
> > > >> +    if (is32bit)
> > > >> +            set_bit(KVM_ARCH_FLAG_EL1_32BIT, &kvm->arch.flags);
> > > >>
> > > >> -    return true;
> > > >> +    set_bit(KVM_ARCH_FLAG_REG_WIDTH_CONFIGURED, &kvm->arch.flags);
> > > >> +
> > > >> +    return 0;
> > > >>   }
> > > >>
> > > >>   /**
> > > >> @@ -230,10 +248,17 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >>      u32 pstate;
> > > >>
> > > >>      mutex_lock(&vcpu->kvm->lock);
> > > >> -    reset_state = vcpu->arch.reset_state;
> > > >> -    WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > > >> +    ret = kvm_set_vm_width(vcpu->kvm,
> > > >> +                           vcpu_has_feature(vcpu, KVM_ARM_VCPU_EL1_32BIT));
> > > >> +    if (!ret) {
> > > >> +            reset_state = vcpu->arch.reset_state;
> > > >> +            WRITE_ONCE(vcpu->arch.reset_state.reset, false);
> > > >> +    }
> > > >>      mutex_unlock(&vcpu->kvm->lock);
> > > >>
> > > >> +    if (ret)
> > > >> +            return ret;
> > > >> +
> > > >>      /* Reset PMU outside of the non-preemptible section */
> > > >>      kvm_pmu_vcpu_reset(vcpu);
> > > >>
> > > >> @@ -260,14 +285,9 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
> > > >>              }
> > > >>      }
> > > >>
> > > >> -    if (!vcpu_allowed_register_width(vcpu)) {
> > > >> -            ret = -EINVAL;
> > > >> -            goto out;
> > > >> -    }
> > > >> -
> > > >>      switch (vcpu->arch.target) {
> > > >>      default:
> > > >> -            if (test_bit(KVM_ARM_VCPU_EL1_32BIT, vcpu->arch.features)) {
> > > >> +            if (vcpu_el1_is_32bit(vcpu)) {
> > > >>                      pstate = VCPU_RESET_PSTATE_SVC;
> > > >>              } else {
> > > >>                      pstate = VCPU_RESET_PSTATE_EL1;
> > > >> --
> > > >> 2.35.1.723.g4982287a31-goog
> > > >>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm