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: <ZrEAXVhH3w6Q0tIy@google.com>
Date: Mon, 5 Aug 2024 09:39:57 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: mlevitsk@...hat.com
Cc: 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>, Chao Gao <chao.gao@...el.com>
Subject: Re: [PATCH v2 1/2] KVM: x86: relax canonical check for some x86
 architectural msrs

On Mon, Aug 05, 2024, mlevitsk@...hat.com wrote:
> У пт, 2024-08-02 у 08:53 -0700, Sean Christopherson пише:
> > > > > > > > > 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.
> 
> Well the problem is that we passthrough these MSRs, and that means that the guest
> can modify them at will, and only ucode can prevent it from doing so.
> 
> So even if the 5 level paging is disabled in the guest's CPUID, but host supports it,
> nothing will prevent the guest to write non canonical value to one of those MSRs, 
> and later KVM during migration or just KVM_SET_SREGS will fail.
 
Ahh, and now I recall the discussions around the virtualization holes with LA57.

> Thus I used kvm_cpu_cap_has on purpose to make KVM follow the actual ucode
> behavior.

I'm leaning towards having KVM do the right thing when emulation happens to be
triggered.  If KVM checks kvm_cpu_cap_has() instead of guest_cpu_cap_has() (looking
at the future), then KVM will extend the virtualization hole to MSRs that are
never passed through, and also to the nested VMX checks.  Or I suppose we could
add separate helpers for passthrough MSRs vs. non-passthrough, but that seems
like it'd add very little value and a lot of maintenance burden.

Practically speaking, outside of tests, I can't imagine the guest will ever care
if there is inconsistent behavior with respect to loading non-canonical values
into MSRs.