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

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 02, 2024, Maxim Levitsky wrote:
> Several architectural msrs (e.g MSR_KERNEL_GS_BASE) must contain
> a canonical address, and according to Intel PRM, this is enforced
> by a #GP canonical check during MSR write.
> 
> However as it turns out, the supported address width
> used for this canonical check is determined only
> by host cpu model:

Please try to wrap consistently and sanely, this is unnecessarily hard to read
because every paragraph manages to wrap at a different column.

> if CPU *supports* 5 level paging, the width will be 57
> regardless of the state of CR4.LA57.
> 
> Experemental tests on a Sapphire Rapids CPU and on a Zen4 CPU
> confirm this behavior.
> 
> In addition to that, the Intel ISA extension manual mentions that this might
> be the architectural behavior:
> 
> Architecture Instruction Set Extensions and Future Features Programming Reference [1].
> Chapter 6.4:
> 
> "CANONICALITY CHECKING FOR DATA ADDRESSES WRITTEN TO CONTROL REGISTERS AND
> MSRS"
> 
> "In Processors that support LAM continue to require the addresses written to
> control registers or MSRs to be 57-bit canonical if the processor _supports_
> 5-level paging or 48-bit canonical if it supports only 4-level paging"
> 
> [1]: https://cdrdv2.intel.com/v1/dl/getContent/671368
> 
> Suggested-by: Chao Gao <chao.gao@intel.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a6968eadd418..3582f0bb7644 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1844,7 +1844,16 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
>  	case MSR_KERNEL_GS_BASE:
>  	case MSR_CSTAR:
>  	case MSR_LSTAR:
> -		if (is_noncanonical_address(data, vcpu))
> +
> +		/*
> +		 * Both AMD and Intel cpus allow values which
> +		 * are canonical in the 5 level paging mode but are not
> +		 * canonical in the 4 level paging mode to be written
> +		 * to the above MSRs, as long as the host CPU supports
> +		 * 5 level paging, regardless of the state of the CR4.LA57.
> +		 */
> +		if (!__is_canonical_address(data,
> +			kvm_cpu_cap_has(X86_FEATURE_LA57) ? 57 : 48))

Please align indentation.

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.

Hmm, and I suspect these are wrong too, but in a different way.  Toggling host
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.

	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)))
		return -EINVAL;

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