linux-kernel - Re: [RFC PATCH v3 01/10] KVM: VMX: Virtualize Intel 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: <Zl5X-YHQL-tIUb4h@google.com>
Date: Mon, 3 Jun 2024 16:55:37 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: Jim Mattson <jmattson@...gle.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	daniel.sneddon@...ux.intel.com, pawan.kumar.gupta@...ux.intel.com, 
	Paolo Bonzini <pbonzini@...hat.com>, Jonathan Corbet <corbet@....net>, 
	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>, linux-doc@...r.kernel.org
Subject: Re: [RFC PATCH v3 01/10] KVM: VMX: Virtualize Intel IA32_SPEC_CTRL

On Fri, Apr 12, 2024, Chao Gao wrote:
> On Thu, Apr 11, 2024 at 09:07:31PM -0700, Jim Mattson wrote:
> >On Wed, Apr 10, 2024 at 7:35 AM Chao Gao <chao.gao@...el.com> wrote:
> >>
> >> From: Daniel Sneddon <daniel.sneddon@...ux.intel.com>
> >>
> >> Currently KVM disables interception of IA32_SPEC_CTRL after a non-0 is
> >> written to IA32_SPEC_CTRL by guest. The guest is allowed to write any
> >> value directly to hardware. There is a tertiary control for
> >> IA32_SPEC_CTRL. This control allows for bits in IA32_SPEC_CTRL to be
> >> masked to prevent guests from changing those bits.
> >>
> >> Add controls setting the mask for IA32_SPEC_CTRL and desired value for
> >> masked bits.
> >>
> >> These new controls are especially helpful for protecting guests that
> >> don't know about BHI_DIS_S and that are running on hardware that
> >> supports it. This allows the hypervisor to set BHI_DIS_S to fully
> >> protect the guest.
> >>
> >> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> >> Signed-off-by: Daniel Sneddon <daniel.sneddon@...ux.intel.com>
> >> Signed-off-by: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> >> [ add a new ioctl to report supported bits. Fix the inverted check ]
> >> Signed-off-by: Chao Gao <chao.gao@...el.com>
> >
> >This looks quite Intel-centric. Isn't this feature essentially the
> >same as AMD's V_SPEC_CTRL?

In spirit, yes.  In practice, not really.  The implementations required for each
end up being quite different.  I think the only bit of code that could be reused
by SVM, and isn't already, is the generation of supported_force_spec_ctrl.

+       kvm_caps.supported_force_spec_ctrl = 0;
+
+       if (cpu_has_spec_ctrl_shadow()) {
+               kvm_caps.supported_force_spec_ctrl |= SPEC_CTRL_IBRS;
+
+               if (boot_cpu_has(X86_FEATURE_STIBP))
+                       kvm_caps.supported_force_spec_ctrl |= SPEC_CTRL_STIBP;
+
+               if (boot_cpu_has(X86_FEATURE_SSBD))
+                       kvm_caps.supported_force_spec_ctrl |= SPEC_CTRL_SSBD;
+
+               if (boot_cpu_has(X86_FEATURE_RRSBA_CTRL) &&
+                   (host_arch_capabilities & ARCH_CAP_RRSBA))
+                       kvm_caps.supported_force_spec_ctrl |= SPEC_CTRL_RRSBA_DIS_S;
+
+               if (boot_cpu_has(X86_FEATURE_BHI_CTRL))
+                       kvm_caps.supported_force_spec_ctrl |= SPEC_CTRL_BHI_DIS_S;
+       }

> Yes. they are almost the same. one small difference is intel's version can
> force some bits off though I don't see how forcing bits off can be useful.

Another not-so-small difference is that Intel's version can also force bits *on*,
and force them on only for the guest with minimal overhead.

> >Can't we consolidate the code, rather than
> >having completely independent implementations for AMD and Intel?
> 
> We surely can consolidate the code. I will do this.
> 
> I have a question about V_SPEC_CTRL. w/ V_SPEC_CTRL, the SPEC_CTRL MSR retains
> the host's value on VM-enter:
> 
> .macro RESTORE_GUEST_SPEC_CTRL
>         /* No need to do anything if SPEC_CTRL is unset or V_SPEC_CTRL is set */
>         ALTERNATIVE_2 "", \
>                 "jmp 800f", X86_FEATURE_MSR_SPEC_CTRL, \
>                 "", X86_FEATURE_V_SPEC_CTRL
> 
> Does this mean all mitigations used by the host will be enabled for the guest
> and guests cannot disable them?

Yes.

> Is this intentional? this looks suboptimal. Why not set SPEC_CTRL value to 0 and
> let guest decide which features to enable? On the VMX side, we need host to
> apply certain hardware mitigations (i.e., BHI_DIS_S and RRSBA_DIS_S) for guest
> because BHI's software mitigation may be ineffective. I am not sure why SVM is
> enabling all mitigations used by the host for guests. Wouldn't it be better to
> enable them on an as-needed basis?

AMD's V_SPEC_CTRL doesn't provide a fast context switch of SPEC_CTRL, it performs
a bitwise-OR of the host and guest values.  So to load a subset (or superset) of
the host protections, KVM would need to do an extra WRMSR before VMRUN, and again
after VMRUN.

That said, I have no idea whether or not avoiding WRMSR on AMD is optimal.