Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 7 May 2021 17:58:09 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Venkatesh Srinivas <venkateshs@chromium.org>
Cc:     Jon Kohler <jon@nutanix.com>, Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: x86: use X86_FEATURE_RSB_CTXSW for RSB stuffing in
 vmexit
Message-ID: <YJV/sZvgA8uN/23k@google.com>
References: <20210507150636.94389-1-jon@nutanix.com>
 <CAA0tLEoyy_ogDc11r_1T907Rp5CwgM64hFwRt5SX40THp2+C3A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAA0tLEoyy_ogDc11r_1T907Rp5CwgM64hFwRt5SX40THp2+C3A@mail.gmail.com>
Precedence: bulk

On Fri, May 07, 2021, Venkatesh Srinivas wrote:
> On Fri, May 7, 2021 at 8:08 AM Jon Kohler <jon@nutanix.com> wrote:
> >
> > cpufeatures.h defines X86_FEATURE_RSB_CTXSW as "Fill RSB on context
> > switches" which seems more accurate than using X86_FEATURE_RETPOLINE
> > in the vmxexit path for RSB stuffing.
> >
> > X86_FEATURE_RSB_CTXSW is used for FILL_RETURN_BUFFER in
> > arch/x86/entry/entry_{32|64}.S. This change makes KVM vmx and svm
> > follow that same pattern. This pairs up nicely with the language in
> > bugs.c, where this cpu_cap is enabled, which indicates that RSB
> > stuffing should be unconditional with spectrev2 enabled.
> >         /*
> >          * If spectre v2 protection has been enabled, unconditionally fill
> >          * RSB during a context switch; this protects against two independent
> >          * issues:
> >          *
> >          *      - RSB underflow (and switch to BTB) on Skylake+
> >          *      - SpectreRSB variant of spectre v2 on X86_BUG_SPECTRE_V2 CPUs
> >          */
> >         setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
> >
> > Furthermore, on X86_FEATURE_IBRS_ENHANCED CPUs && SPECTRE_V2_CMD_AUTO,
> > we're bypassing setting X86_FEATURE_RETPOLINE, where as far as I could
> > find, we should still be doing RSB stuffing no matter what when
> > CONFIG_RETPOLINE is enabled and spectrev2 is set to auto.
> 
> If I'm reading https://software.intel.com/security-software-guidance/deep-dives/deep-dive-indirect-branch-restricted-speculation
> correctly, I don't think an RSB fill sequence is required on VMExit on
> processors w/ Enhanced IBRS. Specifically:
> """
> On processors with enhanced IBRS, an RSB overwrite sequence may not
> suffice to prevent the predicted target of a near return from using an
> RSB entry created in a less privileged predictor mode.  Software can
> prevent this by enabling SMEP (for transitions from user mode to
> supervisor mode) and by having IA32_SPEC_CTRL.IBRS set during VM exits
> """
> On Enhanced IBRS processors, it looks like SPEC_CTRL.IBRS is set
> across all #VMExits via x86_virt_spec_ctrl in kvm.
> 
> So is this patch needed?

Venkatesh belatedly pointed out (off list) that KVM VMX stops intercepting
MSR_IA32_SPEC_CTRL after the first (successful) write by the guest.  But, I 
believe that's a non-issue for ENHANCED_IBRS because of this blurb in Intel's
documentation[*]:

  Processors with enhanced IBRS still support the usage model where IBRS is set
  only in the OS/VMM for OSes that enable SMEP. To do this, such processors will
  ensure that guest behavior cannot control the RSB after a VM exit once IBRS is
  set, even if IBRS was not set at the time of the VM exit.

The code and changelog for commit 706d51681d63 ("x86/speculation: Support
Enhanced IBRS on future CPUs") is more than a little confusing:

  spectre_v2_select_mitigation():
	if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED)) {
		mode = SPECTRE_V2_IBRS_ENHANCED;
		/* Force it so VMEXIT will restore correctly */
		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
		goto specv2_set_mode;
	}


  changelog:
	Kernel also has to make sure that IBRS bit remains set after
	VMEXIT because the guest might have cleared the bit. This is already
	covered by the existing x86_spec_ctrl_set_guest() and
	x86_spec_ctrl_restore_host() speculation control functions.

but I _think_ that is simply saying that MSR_IA32_SPEC_CTRL.IBRS needs to be
restored in order to keep the mitigations active in the host.   I don't think it
contradicts the documentation that says VM-Exit is automagically mitigated if
IBRS has _ever_ been set.

[*] https://software.intel.com/security-software-guidance/deep-dives/deep-dive-indirect-branch-restricted-speculation