linux-kernel - Re: [PATCH v3 1/4] 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: <Zr_MWkZwJHidWjlQ@google.com>
Date: Fri, 16 Aug 2024 15:02:02 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, Ingo Molnar <mingo@...hat.com>, x86@...nel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Chao Gao <chao.gao@...el.com>
Subject: Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86
 architectural msrs

On Fri, Aug 16, 2024, Sean Christopherson wrote:
> On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> How about this?
> 
> /*
>  * The canonicality checks for MSRs that hold linear addresses, e.g. segment
>  * bases, SYSENTER targets, etc., are static, in the sense that they are based
>  * on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> 
> > + * size of whose depends only on CPU's support for 5-level
> > + * paging, rather than state of CR4.LA57.
> > + *
> > + * In addition to that, some of these MSRS are directly passed
> > + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> > + * doen't have LA57 enabled in its CPUID, for consistency with
> > + * CPUs' ucode, it is better to pivot the check around host
> > + * support for 5 level paging.
> 
> I think we should elaborate on why it's better.  It only takes another line or
> two, and that way we don't forget the edge cases that make properly emulating
> guest CPUID a bad idea.
> 
>  * This creates a virtualization hole where a guest writes to passthrough MSRs
>  * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
>  * (because hardware has no awareness of guest CPUID).  Do not try to plug this
>  * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
>  * depending on whether or not KVM happens to emulate a WRMSR would result in
>  * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
>  * passes through an MSR to L2, and then tries to save+restore L2's value.
>  */
> 
> > +
> > +static u8  max_host_supported_virt_addr_bits(void)
> 
> Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
> Mostly to shorten the name, but also because "supported" suggests there's software
> involvement, e.g. the max supported by the kernel/KVM, which isn't the case.
> 
> If you're ok with the above, I'll fixup when applying.

I take that back, I think we're going to need a v4 (see patch 3).