linux-kernel - Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALMp9eRzPXg2WS6-Yy6U90+B8wXm=zhVSkmAym4Y924m7FM-7g@mail.gmail.com>
Date:   Mon, 11 Oct 2021 13:51:20 -0700
From:   Jim Mattson <jmattson@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Vipin Sharma <vipinsh@...gle.com>, pbonzini@...hat.com,
        dmatlack@...gle.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID
 type

On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Mon, Oct 11, 2021, Vipin Sharma wrote:
> > Add a common helper function to read invalidation type specified by a
> > trapped INVPCID/INVEPT/INVVPID instruction.
> >
> > Add a symbol constant for max INVPCID type.
> >
> > No functional change intended.
> >
> > Signed-off-by: Vipin Sharma <vipinsh@...gle.com>
> > ---
> >  arch/x86/include/asm/invpcid.h |  1 +
> >  arch/x86/kvm/vmx/nested.c      |  4 ++--
> >  arch/x86/kvm/vmx/vmx.c         |  4 ++--
> >  arch/x86/kvm/vmx/vmx.h         | 12 ++++++++++++
> >  4 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
> > index 734482afbf81..b5ac26784c1b 100644
> > --- a/arch/x86/include/asm/invpcid.h
> > +++ b/arch/x86/include/asm/invpcid.h
> > @@ -21,6 +21,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
> >  #define INVPCID_TYPE_SINGLE_CTXT     1
> >  #define INVPCID_TYPE_ALL_INCL_GLOBAL 2
> >  #define INVPCID_TYPE_ALL_NON_GLOBAL  3
> > +#define INVPCID_TYPE_MAX             3
>
> ...
>
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 1c8b2b6e7ed9..77f72a41dde3 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -5502,9 +5502,9 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
> >       }
> >
> >       vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> > -     type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
> > +     type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
>
> I would prefer to keep the register read visibile in this code so that it's
> obvious what exactly is being read.  With this approach, it's not clear that KVM
> is reading a GPR vs. VMCS vs. simply extracting "type" from "vmx_instruction_info".
>
> And it's not just the INV* instruction, VMREAD and VMWRITE use the same encoding.
>
> The hardest part is coming up with a name :-)  Maybe do the usually-ill-advised
> approach of following the SDM verbatim?  Reg2 is common to all flavors, so this?
>
>         gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info);
>         type = kvm_register_read(vcpu, gpr_index);
>
> >
> > -     if (type > 3) {
> > +     if (type > INVPCID_TYPE_MAX) {
>
> Hrm, I don't love this because it's not auto-updating in the unlikely chance that
> a new type is added.  I definitely don't like open coding '3' either.  What about
> going with a verbose option of
>
>         if (type != INVPCID_TYPE_INDIV_ADDR &&
>             type != INVPCID_TYPE_SINGLE_CTXT &&
>             type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
>             type != INVPCID_TYPE_ALL_NON_GLOBAL) {
>                 kvm_inject_gp(vcpu, 0);
>                 return 1;
>         }

Better, perhaps, to introduce a new function, valid_invpcid_type(),
and squirrel away the ugliness there?

> It's kind of gross, but gcc10 is smart enought to coalesce those all into a single
> CMP <reg>, 3; JA <#GP>, i.e. the resulting binary is identical.
>
> >               kvm_inject_gp(vcpu, 0);
> >               return 1;
> >       }
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 592217fd7d92..eeafcce57df7 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -522,4 +522,16 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
> >
> >  void dump_vmcs(struct kvm_vcpu *vcpu);
> >
> > +/*
> > + * When handling a VM-exit for one of INVPCID, INVEPT or INVVPID, read the type
> > + * of invalidation specified by the instruction.
> > + */
> > +static inline unsigned long vmx_read_invalidation_type(struct kvm_vcpu *vcpu,
> > +                                                    u32 vmx_instr_info)
> > +{
> > +     u32 vmx_instr_reg2 = (vmx_instr_info >> 28) & 0xf;
> > +
> > +     return kvm_register_read(vcpu, vmx_instr_reg2);
> > +}
> > +
> >  #endif /* __KVM_X86_VMX_H */
> > --
> > 2.33.0.882.g93a45727a2-goog
> >