Re: [PATCH 1/1] kvm: nVMX: Checks "VMCS shadowing" with VMCS link pointer for non-root mode VM{READ,WRITE}

[Yao Yuan]

On Fri, Aug 12, 2022 at 12:33:05PM -0700, Jim Mattson wrote:
> On Thu, Aug 11, 2022 at 7:02 PM Yuan Yao <yuan.yao@xxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Aug 12, 2022 at 09:47:06AM +0800, Yuan Yao wrote:
> > > Add checking to VMCS12's "VMCS shadowing", make sure the checking of
> > > VMCS12's vmcs_link_pointer for non-root mode VM{READ,WRITE} happens
> > > only if VMCS12's "VMCS shadowing" is 1.
> > >
> > > SDM says that for non-root mode the VMCS's "VMCS shadowing" must be 1
> > > (and the corresponding bits in VMREAD/VMWRITE bitmap must be 0) when
> > > condition checking of [B] is reached(please refer [A]), which means
> > > checking to VMCS link pointer for non-root mode VM{READ,WRITE} should
> > > happen only when "VMCS shadowing" = 1.
> > >
> > > Description from SDM Vol3(April 2022) Chapter 30.3 VMREAD/VMWRITE:
> > >
> > > IF (not in VMX operation)
> > >    or (CR0.PE = 0)
> > >    or (RFLAGS.VM = 1)
> > >    or (IA32_EFER.LMA = 1 and CS.L = 0)
> > > THEN #UD;
> > > ELSIF in VMX non-root operation
> > >       AND (“VMCS shadowing” is 0 OR
> > >            source operand sets bits in range 63:15 OR
> > >            VMREAD bit corresponding to bits 14:0 of source
> > >            operand is 1)  <------[A]
> > > THEN VMexit;
> > > ELSIF CPL > 0
> > > THEN #GP(0);
> > > ELSIF (in VMX root operation AND current-VMCS pointer is not valid) OR
> > >       (in VMX non-root operation AND VMCS link pointer is not valid)
> > > THEN VMfailInvalid;  <------ [B]
> > > ...
> > >
> > > Fixes: dd2d6042b7f4 ("kvm: nVMX: VMWRITE checks VMCS-link pointer before VMCS field")
> > > Signed-off-by: Yuan Yao <yuan.yao@xxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/vmx/nested.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index ddd4367d4826..30685be54c5d 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -5123,6 +5123,7 @@ static int handle_vmread(struct kvm_vcpu *vcpu)
> > >                */
> > >               if (vmx->nested.current_vmptr == INVALID_GPA ||
> > >                   (is_guest_mode(vcpu) &&
> > > +                  nested_cpu_has_shadow_vmcs(vcpu) &&
> >
> > Oops, should be "nested_cpu_has_shadow_vmcs(get_vmcs12(vcpu))".
> >
> > >                    get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > >                       return nested_vmx_failInvalid(vcpu);
> > >
> > > @@ -5233,6 +5234,7 @@ static int handle_vmwrite(struct kvm_vcpu *vcpu)
> > >        */
> > >       if (vmx->nested.current_vmptr == INVALID_GPA ||
> > >           (is_guest_mode(vcpu) &&
> > > +          nested_cpu_has_shadow_vmcs(vcpu) &&
> >
> > Ditto.
> >
> > >            get_vmcs12(vcpu)->vmcs_link_pointer == INVALID_GPA))
> > >               return nested_vmx_failInvalid(vcpu);
> > >
> > > --
>
> These checks are redundant, aren't they?
>
> That is, nested_vmx_exit_handled_vmcs_access() has already checked
> nested_cpu_has_shadow_vmcs(vmcs12).

Ah, you're right it does there.

That means in L0 we handle this for vmcs12 which has shadow VMCS
setting and the corresponding bit in the bitmap is 0(so no vmexit to
L1 and the read/write should from/to vmcs12's shadow vmcs, we handle
this here to emulate this), so we don't need to check the shdaow VMCS
setting here again. Is this the right understanding ?