linux-kernel - Re: [PATCH v3 13/35] x86/bugs: Restructure spectre_v2

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: <20250212213526.2ztowz3z4r3lxplk@jpoimboe>
Date: Wed, 12 Feb 2025 13:35:26 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: "Kaplan, David" <David.Kaplan@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 13/35] x86/bugs: Restructure spectre_v2_user mitigation

On Wed, Feb 12, 2025 at 03:59:39PM +0000, Kaplan, David wrote:
> > On Wed, Jan 08, 2025 at 02:24:53PM -0600, David Kaplan wrote:
> > > -     if (retbleed_mitigation == RETBLEED_MITIGATION_UNRET ||
> > > -         retbleed_mitigation == RETBLEED_MITIGATION_IBPB) {
> > > -             if (mode != SPECTRE_V2_USER_STRICT &&
> > > -                 mode != SPECTRE_V2_USER_STRICT_PREFERRED)
> > > +     if (spectre_v2_user_stibp != SPECTRE_V2_USER_NONE &&
> > > +         (retbleed_mitigation == RETBLEED_MITIGATION_UNRET ||
> > > +         retbleed_mitigation == RETBLEED_MITIGATION_IBPB)) {
> >
> > This adds a hidden dependency on retbleed_update_mitigation()?
> 
> Yeah I guess it does.  I'm not sure of a way to cleanly avoid this if
> the logic is kept as-is, do you think it's ok just to document this
> dependency explicitly?

Yeah, if the dependencies can't be cleanly unwound, at least they should
be explicitly documented with comments at the call sites, similar to
what we attempt to do today.

> The only case I think where this matters is if 'stuff' is selected for
> retbleed, and then retbleed_update_mitigation decides you can't do
> that and it has to re-select and may end up with unret or ibpb.  That
> case doesn't even make much sense since 'retbleed=stuff' isn't a
> mitigation for AMD.

True, though generally we should treat such things as hard dependencies.

It would be really easy for a future person to come along and introduce
a bug when they see the 'retbleed_mitigation' reference and assume the
dependency has already been handled.

So basically any "update" function which references the output of
another "update" function should be treated as a dependency.  Because
even it's not technically a dependency, that could easily change in the
future without being noticed, with a patch to *either* of the functions.

> One idea, which would involve changing the logic vs upstream, is that
> 'retbleed=stuff' should only be allowed on Intel and it should be
> converted to AUTO on AMD.  If that's the case, then there isn't really
> a hidden dependency anymore since the retbleed mitigation will never
> change to unret/ibpb during retbleed_update_mitigation().  Thoughts?

Yeah, I'm strongly in favor of any such simplification.  We spend *way*
too much maintenance effort on all these weird options and combinations
which don't make sense.

-- 
Josh