linux-kernel - Re: [PATCH v2] KVM: SVM: let alternatives handle the cases when RSB filling is required

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: <ZpTeuJHgwz9u8d_k@t470s.drde.home.arpa>
Date: Mon, 15 Jul 2024 10:35:26 +0200
From: Amit Shah <amit@...nel.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: David Kaplan <David.Kaplan@....com>, Jim Mattson <jmattson@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"bp@...en8.de" <bp@...en8.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"hpa@...or.com" <hpa@...or.com>,
	Kim Phillips <kim.phillips@....com>
Subject: Re: [PATCH v2] KVM: SVM: let alternatives handle the cases when RSB
 filling is required

On (Mon) 08 Jul 2024 [11:59:45], Sean Christopherson wrote:
> On Mon, Jul 01, 2024, David Kaplan wrote:
> > > > >        /*
> > > > >         * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the
> > > > > Intel feature
> > > > >         * flag and protect from vendor-specific bugs via the
> > > > > whitelist.
> > > > >         *
> > > > >         * Don't use AutoIBRS when SNP is enabled because it degrades
> > > > > host
> > > > >         * userspace indirect branch performance.
> > > > >         */
> > > > >        if ((x86_arch_cap_msr & ARCH_CAP_IBRS_ALL) ||
> > > > >            (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
> > > > >             !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
> > > > >                setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
> > > > >                if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB)
> > > > > &&
> > > > >                    !(x86_arch_cap_msr & ARCH_CAP_PBRSB_NO))
> > > > >                        setup_force_cpu_bug(X86_BUG_EIBRS_PBRSB);
> > > > >        }
> > > >
> > > > Families 0FH through 12H don't have EIBRS or AutoIBRS, so there's no
> > > > cpu_vuln_whitelist[] lookup. Hence, no need to set the NO_EIBRS_PBRSB
> > > > bit, even if it is accurate.
> > >
> > > The commit that adds the RSB_VMEXIT_LITE feature flag does describe the
> > > bug in a good amount of detail:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i
> > > d=2b1299322016731d56807aa49254a5ea3080b6b3
> > >
> > > I've not seen any indication this is required for AMD CPUs.
> > >
> > > David, do you agree we don't need this?
> > >
> > 
> > It's not required, as AMD CPUs don't have the PBRSB issue with AutoIBRS.
> > Although I think Sean was talking about being extra paranoid
> 
> Ya.  I'm asking if there's a reason not to tack on X86_FEATURE_RSB_VMEXIT_LITE,
> beyond it effectively being dead code.  There's no runtime cost, and so assuming
> it doesn't get spuriously enabled, I don't see a downside.

Ah - I get it now.  You want to add this code for parity with
vmenter.S so that a future bug like this doesn't happen.

I disagree, though.  It's not really dead code - it does get patched
at runtime.  If a future AMD CPU has a bug that Intel doesn't, we'll
have to introduce a new ALTERNATIVE just for that condition - leading
to more complexity than is actually required.

Also - reviewers of code will get confused, wondering why this code
for AMD exists when the CPU vuln does not.

I get that we want to write defensive code, but this was a very
special condition that is unlikely to happen in this part of the code,
and also this was missed by the devs and the reviewers.

The good thing here is that missing this only leads to suboptimal
code, not a security bug.  So given all this, I vote for the
simplicity of code, rather than tacking on something.

Sound OK?


		Amit
--