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: <ZpbFvTUeB3gMIKiU@google.com>
Date: Tue, 16 Jul 2024 12:10:53 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Amit Shah <amit@...nel.org>
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, Jul 15, 2024, Amit Shah wrote:
> 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.

Eh, we're splitting hairs over what's dead code where.

> 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.

If and only if the bug was mitigated by FILL_RETURN_BUFFER.  And if we needed to
extend FILL_RETURN_BUFFER, then we'd need a new alternative regardless of whether
or not KVM SVM honored RSB_VMEXIT_LITE.

If the hypothetical AMD bug is fixed by a single stuffed return, then the kernel
would simply force set RSB_VMEXIT_LITE as appropriate.  If the bug requires a
mitigation somewhere between RSB_CLEAR_LOOPS and 1, we'd need to add more code
somewhere.

> 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.

Defensive code is only part of it, and a minor part at that.  The main "issue" is
having divergent VM-Enter/VM-Exit code for Intel vs. AMD.  To those of us that
care primarily about virtualization and are only passingly familiar with the myriad
speculation bugs and mitigations, omitting RSB_VMEXIT_LITE _looks_ wrong.

To know that the omission is correct, one has to suss out that it's (supposed to
be) impossible for RSB_VMEXIT_LITE to be set on AMD.  And as a KVM person, that's
a detail I don't want to care about.

FWIW, I feel the same way about all the other post-VM-Exit mitigations, they just
don't stand out in the same way because the entire mitigation sequence is absent
on one vendor the other, i.e. they don't look wrong at first glance.  But if KVM
could have a mostly unified VM-Enter => VM-Exit assembly code, I would happliy eat
a dead NOP/JMP or three.  Now that I look at it, that actually seems very doable...

> The good thing here is that missing this only leads to suboptimal
> code, not a security bug.

I don't think we can guarantee that.  Obviously this is all speculative (lolz),
but AFAICT, X86_FEATURE_RSB_VMEXIT_LITE doesn't imply X86_FEATURE_RSB_VMEXIT.

> So given all this, I vote for the simplicity of code, rather than tacking on
> something.
> 
> Sound OK?
> 
> 
> 		Amit
> --