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_JX1z8xWNAxHmz@google.com>
Date: Fri, 16 Aug 2024 14:49:19 -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 Thu, Aug 15, 2024, Maxim Levitsky wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ce7c00894f32..2e83f7d74591 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
>  		       sizeof(kvm_vcpu_stats_desc),
>  };
>  
> +
> +/*
> + * Most x86 arch MSR values which contain linear addresses like

Is it most, or all?  I'm guessing all?

> + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> + * have static canonicality checks,

Weird and early line breaks.

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.