Re: [PATCH v2 1/2] KVM: x86: relax canonical check for some x86 architectural msrs

[Chao Gao <chao.gao@intel.com>]

>Checking kvm_cpu_cap_has() is wrong.  What the _host_ supports is irrelevant,
>what matters is what the guest CPU supports, i.e. this should check guest CPUID.
>Ah, but for safety, KVM also needs to check kvm_cpu_cap_has() to prevent faulting
>on a bad load into hardware.  Which means adding a "governed" feature until my
>CPUID rework lands.
>
>And I'm pretty sure this fix is incomplete, as nVMX's consistency checks on MSRs
>that are loaded via dedicated VMCS fields likely need the same treatment, e.g.
>presumably these checks should follow the MSR handling.
>
>	if (CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_eip, vcpu)))
>		return -EINVAL;
>
>
>	    (CC(is_noncanonical_address(vmcs12->guest_bndcfgs & PAGE_MASK, vcpu)) ||
>
>So I think we probably need a dedicated helper for MSRs.

+1

>
>Hmm, and I suspect these are wrong too, but in a different way.  Toggling host

Actually they are wrong in the _same_ way because ...

>LA57 on VM-Exit is legal[*], so logically, KVM should use CR4.LA57 from
>vmcs12->host_cr4, not the vCPU's current CR4 value.  Which makes me _really_
>curious if Intel CPUs actually get that right.

... according to Section 3.1 in 5-level paging whitepaper[1], on VM entry, the
canonicality checking about the host/guest state (except guest RIP) is also
based on whether the CPU supports 5-level paging, i.e., host or guest CR4.LA57
isn't checked.

[1]: https://cdrdv2.intel.com/v1/dl/getContent/671442?fileName=5-level-paging-white-paper.pdf

>
>	if (CC(is_noncanonical_address(vmcs12->host_fs_base, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_gs_base, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_gdtr_base, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_idtr_base, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_tr_base, vcpu)) ||
>	    CC(is_noncanonical_address(vmcs12->host_rip, vcpu)))

not sure how host_rip should be handled because it isn't documented in that
spec.

>		return -EINVAL;
>
>[*] https://lore.kernel.org/all/20210622211124.3698119-1-seanjc@google.com