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

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: <ZrBtOAzZurwvWKZM@chao-email>
Date: Mon, 5 Aug 2024 14:12:08 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Maxim Levitsky <mlevitsk@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Borislav Petkov <bp@...en8.de>, "H. Peter
 Anvin" <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, Ingo Molnar
	<mingo@...hat.com>, <x86@...nel.org>, Thomas Gleixner <tglx@...utronix.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v2 1/2] KVM: x86: relax canonical check for some x86
 architectural msrs

>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