linux-kernel - Re: [PATCH v3] KVM: VMX: do not disable interception for MSR_IA32_SPEC

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: <Yo5hmcdRvE1UrI4y@google.com>
Date:   Wed, 25 May 2022 17:04:25 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Jon Kohler <jon@...anix.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Waiman Long <longman@...hat.com>,
        Kees Cook <keescook@...omium.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] KVM: VMX: do not disable interception for
 MSR_IA32_SPEC_CTRL on eIBRS

On Fri, May 20, 2022, Jon Kohler wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 610355b9ccce..1c725d17d984 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2057,20 +2057,32 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			return 1;
> 
>  		vmx->spec_ctrl = data;
> -		if (!data)
> +
> +		/*
> +		 * Disable interception on the first non-zero write, unless the
> +		 * guest is hosted on an eIBRS system and setting only

The "unless guest is hosted on an eIBRS system" blurb is wrong and doesn't match
the code.  Again, it's all about whether eIBRS is advertised to the guest.  With
some other minor tweaking to wrangle the comment to 80 chars...

		/*
                 * Disable interception on the first non-zero write, except if
		 * eIBRS is advertised to the guest and the guest is enabling
		 * _only_ IBRS.  On eIBRS systems, kernels typically set IBRS
		 * once at boot and never touch it post-boot.  All other bits,
		 * and IBRS on non-eIBRS systems, are often set on a per-task
		 * basis, i.e. change frequently, so the benefit of avoiding
		 * VM-exits during guest context switches outweighs the cost of
		 * RDMSR on every VM-Exit to save the guest's value.
		 */

> +		 * SPEC_CTRL_IBRS, which is typically set once at boot and never

Uber nit, when it makes sense, avoid regurgitating the code verbatim, e.g. refer
to "setting SPEC_CTRL_IBRS" as "enabling IBRS".  That little bit of abstraction
can sometimes help unfamiliar readers understand the effect of the code, whereas
copy+pasting bits of the code doesn't provide any additional context.

> +		 * touched again.  All other bits are often set on a per-task
> +		 * basis, i.e. may change frequently, so the benefit of avoiding
> +		 * VM-exits during guest context switches outweighs the cost of
> +		 * RDMSR on every VM-Exit to save the guest's value.
> +		 */
> +		if (!data ||
> +			(data == SPEC_CTRL_IBRS &&
> +			 (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL)))

Align the two halves of the logical-OR, i.e.

		if (!data ||
		    (data == SPEC_CTRL_IBRS &&
		     (vcpu->arch.arch_capabilities & ARCH_CAP_IBRS_ALL)))
			break;