linux-kernel - Re: [PATCH v2 24/49] KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to avoid macro collisions

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: <ZoxaOqvXzTH6O64D@google.com>
Date: Mon, 8 Jul 2024 14:29:30 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Hou Wenlong <houwenlong.hwl@...group.com>, 
	Kechen Lu <kechenl@...dia.com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Binbin Wu <binbin.wu@...ux.intel.com>, Yang Weijiang <weijiang.yang@...el.com>, 
	Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 24/49] KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to
 avoid macro collisions

On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> > Undefine SPEC_CTRL_SSBD, which is #defined by msr-index.h to represent the
> > enable flag in MSR_IA32_SPEC_CTRL, to avoid issues with the macro being
> > unpacked into its raw value when passed to KVM's F() macro.  This will
> > allow using multiple layers of macros in F() and friends, e.g. to harden
> > against incorrect usage of F().
> > 
> > No functional change intended (cpuid.c doesn't consume SPEC_CTRL_SSBD).
> > 
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 8efffd48cdf1..a16d6e070c11 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -639,6 +639,12 @@ static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask)
> >  	kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid);
> >  }
> >  
> > +/*
> > + * Undefine the MSR bit macro to avoid token concatenation issues when
> > + * processing X86_FEATURE_SPEC_CTRL_SSBD.
> > + */
> > +#undef SPEC_CTRL_SSBD
> > +
> >  void kvm_set_cpu_caps(void)
> >  {
> >  	memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps));
> 
> Hi,
> 
> Maybe we should instead rename the SPEC_CTRL_SSBD to
> 'MSR_IA32_SPEC_CTRL_SSBD' and together with it, other fields of this msr.  It
> seems that at least some msrs in this file do this.

Yeah, the #undef hack is quite ugly.  But I didn't (and still don't) want to
introduce all the renaming churn in the middle of this already too-big series,
especially since it would require touching quite a bit of code outside of KVM.

I'm also not sure that's the right thing to do; I kinda feel like KVM is the one
that's being silly here.

Aha!  Rather than rename the MSR bits, what if we rename the X86_FEATURE flag,
e.g. to X86_FEATURE_INTEL_SPEC_CTRL_SSBD, X86_FEATURE_MSR_SPEC_CTRL_SSBD, or maybe
even just X86_FEATURE_INTEL_SSBD.  Much less churn, and it would add even more
clarity as to why there's also X86_FEATURE_SSBD and X86_FEATURE_AMD_SSBD.

I'll post a standalone patch to make that change, and maybe see if I can take it
through the KVM tree.