Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4

[Paolo Bonzini]

On Sat, Mar 11, 2023 at 1:17 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > @@ -3047,6 +3047,19 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> >                                          vmcs12->guest_ia32_perf_global_ctrl)))
> >               return -EINVAL;
> >
> > +     if (CC((vmcs12->guest_cr0 & (X86_CR0_PG | X86_CR0_PE)) == X86_CR0_PG))
> > +             return -EINVAL;
> > +
> > +#ifdef CONFIG_X86_64
>
> The #ifdef isn't necessary, attempting to set for a 32-bit host should be rejected
> by nested_vmx_check_controls() since nested_vmx_setup_ctls_msrs() clears the bit.
> Ditto for the host logic related to VM_EXIT_HOST_ADDR_SPACE_SIZE, which looks
> suspiciously similar ;-)

Yeah, I noticed that too and decided that the idea could have been to
allow some dead code elimination on 32-bit kernels, so I copied what
the host state checks were doing. But if you prefer the more compact
way I am absolutely not going to complain.

> > +     if (CC(ia32e &&
> > +            (!(vmcs12->guest_cr4 & X86_CR4_PAE) ||
> > +             !(vmcs12->guest_cr0 & X86_CR0_PG))))
> > +             return -EINVAL;
>
> This is a lot easier to read IMO, and has the advantage of more precisely
> identifying the failure in the tracepoint.
>
>         if (CC(ia32e && !(vmcs12->guest_cr4 & X86_CR4_PAE)) ||
>             CC(ia32e && !(vmcs12->guest_cr4 & X86_CR0_PG)))
>                 return -EINVAL;

Looks good.  I squashed everything in.

Paolo